Drive the force=1 flow through the driver core. There are two main reasons to do this:
1) To enable tpm_tis for OF environments requires a platform_device anyhow, so
the force_device needs to be re-used for them.
2) Recent changes in the core code break the assumption that a driver will be
'attached' to things created through platform_device_register_simple,
which causes the tpm core to blow up.
To make force probing reliable this also fixes both tpm_tis and tpm_crb to
properly use request_region to lock the TPM iomemory against multiple access.
v3:
- Fix some bugs in getting the struct resource for tpm_tis (Martin Wilck)
- Include tpm_crb in the request_resource cleanup as well, tpm_tis and tpm_crb
tend to use the same address ranges so both should have locking for safety
- ACPI and endianness cleanups in both drivers
v2:
- Make sure we request the mem resource in tpm_tis to avoid double-loading
the driver
- Re-order the init sequence so that a forced platform device gets first crack at
loading, and excludes the other mechanisms via the above
- Checkpatch clean
- Gotos renamed
Jason Gunthorpe (7):
tpm_crb: Use the common ACPI definition of struct acpi_tpm2
tpm_tis: Disable interrupt auto probing on a per-device basis
tpm_tis: Do not fall back to a hardcoded address for TPM2
tpm_tis: Use devm_ioremap_resource
tpm_tis: Clean up the force=1 module parameter
tpm_crb: Drop le32_to_cpu(ioread32(..))
tpm_crb: Use devm_ioremap_resource
drivers/char/tpm/tpm.h | 7 --
drivers/char/tpm/tpm_crb.c | 196 +++++++++++++++++++++-------------
drivers/char/tpm/tpm_tis.c | 254 +++++++++++++++++++++++++--------------------
3 files changed, 264 insertions(+), 193 deletions(-)
--
2.1.4
include/acpi/actbl2.h is the proper place for these definitions
and the needed TPM2 ones have been there since
commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
This also drops a couple of le32_to_cpu's for members of this table,
the existing swapping was not done consistently, and the definitions
in actbl2.h do not have endianness annotations, declaring that no swap
is required. Note that the TPM ACPI spec defines all of these
values to be little endian, both in crb2 and ppi.
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Wilck, Martin <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm.h | 7 -------
drivers/char/tpm/tpm_crb.c | 31 +++++++++----------------------
drivers/char/tpm/tpm_tis.c | 2 +-
3 files changed, 10 insertions(+), 30 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 542a80cbfd9c..28b477e8da6a 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -128,13 +128,6 @@ enum tpm2_startup_types {
TPM2_SU_STATE = 0x0001,
};
-enum tpm2_start_method {
- TPM2_START_ACPI = 2,
- TPM2_START_FIFO = 6,
- TPM2_START_CRB = 7,
- TPM2_START_CRB_WITH_ACPI = 8,
-};
-
struct tpm_chip;
struct tpm_vendor_specific {
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8342cf51ffdc..8dd70696ebe8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,14 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
};
-struct acpi_tpm2 {
- struct acpi_table_header hdr;
- u16 platform_class;
- u16 reserved;
- u64 control_area_pa;
- u32 start_method;
-} __packed;
-
enum crb_ca_request {
CRB_CA_REQ_GO_IDLE = BIT(0),
CRB_CA_REQ_CMD_READY = BIT(1),
@@ -207,7 +199,7 @@ static const struct tpm_class_ops tpm_crb = {
static int crb_acpi_add(struct acpi_device *device)
{
struct tpm_chip *chip;
- struct acpi_tpm2 *buf;
+ struct acpi_table_tpm2 *buf;
struct crb_priv *priv;
struct device *dev = &device->dev;
acpi_status status;
@@ -217,13 +209,14 @@ static int crb_acpi_add(struct acpi_device *device)
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) &buf);
- if (ACPI_FAILURE(status)) {
- dev_err(dev, "failed to get TPM2 ACPI table\n");
+ if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
+ dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
return -ENODEV;
}
/* Should the FIFO driver handle this? */
- if (buf->start_method == TPM2_START_FIFO)
+ sm = buf->start_method;
+ if (sm == ACPI_TPM2_MEMORY_MAPPED)
return -ENODEV;
chip = tpmm_chip_alloc(dev, &tpm_crb);
@@ -232,11 +225,6 @@ static int crb_acpi_add(struct acpi_device *device)
chip->flags = TPM_CHIP_FLAG_TPM2;
- if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
- dev_err(dev, "TPM2 ACPI table has wrong size");
- return -EINVAL;
- }
-
priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
GFP_KERNEL);
if (!priv) {
@@ -244,21 +232,20 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENOMEM;
}
- sm = le32_to_cpu(buf->start_method);
-
/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
- if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
+ if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
- if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
+ if (sm == ACPI_TPM2_START_METHOD ||
+ sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
priv->flags |= CRB_FL_ACPI_START;
priv->cca = (struct crb_control_area __iomem *)
- devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
+ devm_ioremap_nocache(dev, buf->control_address, 0x1000);
if (!priv->cca) {
dev_err(dev, "ioremap of the control area failed\n");
return -ENOMEM;
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..304323bdcaaa 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -135,7 +135,7 @@ static inline int is_fifo(struct acpi_device *dev)
return 0;
}
- if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+ if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
return 0;
/* TPM 2.0 FIFO */
--
2.1.4
Instead of clearing the global interrupts flag when any device
does not have an interrupt just pass -1 through tpm_info.irq.
The only thing that asks for autoprobing is the force=1 path.
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Wilck, Martin <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 304323bdcaaa..fecd27b45fd1 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -69,7 +69,11 @@ enum tis_defaults {
struct tpm_info {
unsigned long start;
unsigned long len;
- unsigned int irq;
+ /* irq > 0 means: use irq $irq;
+ * irq = 0 means: autoprobe for an irq;
+ * irq = -1 means: no irq support
+ */
+ int irq;
};
static struct tpm_info tis_default_info = {
@@ -807,7 +811,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
/* INTERRUPT Setup */
init_waitqueue_head(&chip->vendor.read_queue);
init_waitqueue_head(&chip->vendor.int_queue);
- if (interrupts) {
+ if (interrupts && tpm_info->irq != -1) {
if (tpm_info->irq) {
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
tpm_info->irq);
@@ -895,9 +899,9 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
#ifdef CONFIG_PNP
static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
- const struct pnp_device_id *pnp_id)
+ const struct pnp_device_id *pnp_id)
{
- struct tpm_info tpm_info = tis_default_info;
+ struct tpm_info tpm_info = {};
acpi_handle acpi_dev_handle = NULL;
tpm_info.start = pnp_mem_start(pnp_dev, 0);
@@ -906,7 +910,7 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
if (pnp_irq_valid(pnp_dev, 0))
tpm_info.irq = pnp_irq(pnp_dev, 0);
else
- interrupts = false;
+ tpm_info.irq = -1;
#ifdef CONFIG_ACPI
if (pnp_acpi_device(pnp_dev)) {
@@ -984,6 +988,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
return -ENODEV;
INIT_LIST_HEAD(&resources);
+ tpm_info.irq = -1;
ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
&tpm_info);
if (ret < 0)
@@ -991,9 +996,6 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
acpi_dev_free_resource_list(&resources);
- if (!tpm_info.irq)
- interrupts = false;
-
if (is_itpm(acpi_dev))
itpm = true;
--
2.1.4
If the ACPI tables do not declare a memory resource for the TPM2
then do not just fall back to the x86 default base address.
Also be stricter when checking the ancillary TPM2 ACPI data and error
out if any of this data is wrong rather than blindly assuming TPM1.
Fixes: 399235dc6e95 ("tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0")
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Wilck, Martin <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis.c | 48 +++++++++++++++++-----------------------------
1 file changed, 18 insertions(+), 30 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index fecd27b45fd1..b2b31f5418ca 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -122,39 +122,11 @@ static inline int is_itpm(struct acpi_device *dev)
{
return has_hid(dev, "INTC0102");
}
-
-static inline int is_fifo(struct acpi_device *dev)
-{
- struct acpi_table_tpm2 *tbl;
- acpi_status st;
-
- /* TPM 1.2 FIFO */
- if (!has_hid(dev, "MSFT0101"))
- return 1;
-
- st = acpi_get_table(ACPI_SIG_TPM2, 1,
- (struct acpi_table_header **) &tbl);
- if (ACPI_FAILURE(st)) {
- dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
- return 0;
- }
-
- if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
- return 0;
-
- /* TPM 2.0 FIFO */
- return 1;
-}
#else
static inline int is_itpm(struct acpi_device *dev)
{
return 0;
}
-
-static inline int is_fifo(struct acpi_device *dev)
-{
- return 1;
-}
#endif
/* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -980,11 +952,21 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
{
+ struct acpi_table_tpm2 *tbl;
+ acpi_status st;
struct list_head resources;
- struct tpm_info tpm_info = tis_default_info;
+ struct tpm_info tpm_info = {};
int ret;
- if (!is_fifo(acpi_dev))
+ st = acpi_get_table(ACPI_SIG_TPM2, 1,
+ (struct acpi_table_header **) &tbl);
+ if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
+ dev_err(&acpi_dev->dev,
+ FW_BUG "failed to get TPM2 ACPI table\n");
+ return -EINVAL;
+ }
+
+ if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
return -ENODEV;
INIT_LIST_HEAD(&resources);
@@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
acpi_dev_free_resource_list(&resources);
+ if (tpm_info.start == 0 && tpm_info.len == 0) {
+ dev_err(&acpi_dev->dev,
+ FW_BUG "TPM2 ACPI table does not define a memory resource\n");
+ return -EINVAL;
+ }
+
if (is_itpm(acpi_dev))
itpm = true;
--
2.1.4
This does a request_resource under the covers which means tis holds a
lock on the memory range it is using so other drivers cannot grab it.
When doing probing it is important to ensure that other drivers are
not using the same range before tis starts touching it.
To do this flow the actual struct resource from the device right
through to devm_ioremap_resource. This ensures all the proper resource
meta-data is carried down.
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Wilck, Martin <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index b2b31f5418ca..856fb35e574c 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -67,8 +67,7 @@ enum tis_defaults {
};
struct tpm_info {
- unsigned long start;
- unsigned long len;
+ struct resource res;
/* irq > 0 means: use irq $irq;
* irq = 0 means: autoprobe for an irq;
* irq = -1 means: no irq support
@@ -77,8 +76,11 @@ struct tpm_info {
};
static struct tpm_info tis_default_info = {
- .start = TIS_MEM_BASE,
- .len = TIS_MEM_LEN,
+ .res = {
+ .start = TIS_MEM_BASE,
+ .end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
+ .flags = IORESOURCE_MEM,
+ },
.irq = 0,
};
@@ -692,7 +694,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
- chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
+ chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
if (!chip->vendor.iobase)
return -EIO;
@@ -875,9 +877,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
{
struct tpm_info tpm_info = {};
acpi_handle acpi_dev_handle = NULL;
+ struct resource *res;
- tpm_info.start = pnp_mem_start(pnp_dev, 0);
- tpm_info.len = pnp_mem_len(pnp_dev, 0);
+ res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+ tpm_info.res = *res;
if (pnp_irq_valid(pnp_dev, 0))
tpm_info.irq = pnp_irq(pnp_dev, 0);
@@ -940,12 +945,10 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
struct tpm_info *tpm_info = (struct tpm_info *) data;
struct resource res;
- if (acpi_dev_resource_interrupt(ares, 0, &res)) {
+ if (acpi_dev_resource_interrupt(ares, 0, &res))
tpm_info->irq = res.start;
- } else if (acpi_dev_resource_memory(ares, &res)) {
- tpm_info->start = res.start;
- tpm_info->len = resource_size(&res);
- }
+ else if (acpi_dev_resource_memory(ares, &res))
+ tpm_info->res = res;
return 1;
}
@@ -978,7 +981,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
acpi_dev_free_resource_list(&resources);
- if (tpm_info.start == 0 && tpm_info.len == 0) {
+ if (resource_type(&tpm_info.res) != IORESOURCE_MEM) {
dev_err(&acpi_dev->dev,
FW_BUG "TPM2 ACPI table does not define a memory resource\n");
return -EINVAL;
--
2.1.4
The TPM core has long assumed that every device has a driver attached,
however the force path was attaching the TPM core outside of a driver
context. This isn't generally reliable as the user could detatch the
driver using sysfs or something, but commit b8b2c7d845d5 ("base/platform:
assert that dev_pm_domain callbacks are called unconditionally")
forced the issue by leaving the driver pointer NULL if there is
no probe.
Rework the TPM setup to create a platform device with resources and
then allow the driver core to naturally bind and probe it through the
normal mechanisms. All this structure is needed anyhow to enable TPM
for OF environments.
Finally, since the entire flow is changing convert the init/exit to use
the modern ifdef-less coding style when possible
Reported-by: "Wilck, Martin" <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Wilck, Martin <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_tis.c | 173 +++++++++++++++++++++++++++------------------
1 file changed, 106 insertions(+), 67 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 856fb35e574c..12aa96a69b6c 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -60,7 +60,6 @@ enum tis_int_flags {
};
enum tis_defaults {
- TIS_MEM_BASE = 0xFED40000,
TIS_MEM_LEN = 0x5000,
TIS_SHORT_TIMEOUT = 750, /* ms */
TIS_LONG_TIMEOUT = 2000, /* 2 sec */
@@ -75,15 +74,6 @@ struct tpm_info {
int irq;
};
-static struct tpm_info tis_default_info = {
- .res = {
- .start = TIS_MEM_BASE,
- .end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
- .flags = IORESOURCE_MEM,
- },
- .irq = 0,
-};
-
/* Some timeout values are needed before it is known whether the chip is
* TPM 1.0 or TPM 2.0.
*/
@@ -695,8 +685,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
#endif
chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
- if (!chip->vendor.iobase)
- return -EIO;
+ if (IS_ERR(chip->vendor.iobase))
+ return PTR_ERR(chip->vendor.iobase);
/* Maximum timeouts */
chip->vendor.timeout_a = TIS_TIMEOUT_A_MAX;
@@ -825,7 +815,6 @@ out_err:
return rc;
}
-#ifdef CONFIG_PM_SLEEP
static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
{
u32 intmask;
@@ -867,11 +856,9 @@ static int tpm_tis_resume(struct device *dev)
return 0;
}
-#endif
static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
-#ifdef CONFIG_PNP
static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
const struct pnp_device_id *pnp_id)
{
@@ -889,14 +876,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
else
tpm_info.irq = -1;
-#ifdef CONFIG_ACPI
if (pnp_acpi_device(pnp_dev)) {
if (is_itpm(pnp_acpi_device(pnp_dev)))
itpm = true;
- acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
+ acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
}
-#endif
return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
}
@@ -937,7 +922,6 @@ static struct pnp_driver tis_pnp_driver = {
module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
-#endif
#ifdef CONFIG_ACPI
static int tpm_check_resource(struct acpi_resource *ares, void *data)
@@ -1024,80 +1008,135 @@ static struct acpi_driver tis_acpi_driver = {
};
#endif
+static struct platform_device *force_pdev;
+
+static int tpm_tis_plat_probe(struct platform_device *pdev)
+{
+ struct tpm_info tpm_info = {};
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(&pdev->dev, "no memory resource defined\n");
+ return -ENODEV;
+ }
+ tpm_info.res = *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (res) {
+ tpm_info.irq = res->start;
+ } else {
+ if (pdev == force_pdev)
+ tpm_info.irq = -1;
+ else
+ /* When forcing auto probe the IRQ */
+ tpm_info.irq = 0;
+ }
+
+ return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
+}
+
+static int tpm_tis_plat_remove(struct platform_device *pdev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
+
+ tpm_chip_unregister(chip);
+ tpm_tis_remove(chip);
+
+ return 0;
+}
+
static struct platform_driver tis_drv = {
+ .probe = tpm_tis_plat_probe,
+ .remove = tpm_tis_plat_remove,
.driver = {
.name = "tpm_tis",
.pm = &tpm_tis_pm,
},
};
-static struct platform_device *pdev;
-
static bool force;
+#ifdef CONFIG_X86
module_param(force, bool, 0444);
MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
+#endif
+
+static int tpm_tis_force_device(void)
+{
+ struct platform_device *pdev;
+ static const struct resource x86_resources[] = {
+ {
+ .start = 0xFED40000,
+ .end = 0xFED40000 + TIS_MEM_LEN - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ };
+
+ if (!force)
+ return 0;
+
+ /* The driver core will match the name tpm_tis of the device to
+ * the tpm_tis platform driver and complete the setup via
+ * tpm_tis_plat_probe
+ */
+ pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
+ ARRAY_SIZE(x86_resources));
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
+ force_pdev = pdev;
+
+ return 0;
+}
+
static int __init init_tis(void)
{
int rc;
-#ifdef CONFIG_PNP
- if (!force) {
- rc = pnp_register_driver(&tis_pnp_driver);
- if (rc)
- return rc;
- }
-#endif
+
+ rc = tpm_tis_force_device();
+ if (rc)
+ goto err_force;
+
+ rc = platform_driver_register(&tis_drv);
+ if (rc)
+ goto err_platform;
+
#ifdef CONFIG_ACPI
- if (!force) {
- rc = acpi_bus_register_driver(&tis_acpi_driver);
- if (rc) {
-#ifdef CONFIG_PNP
- pnp_unregister_driver(&tis_pnp_driver);
-#endif
- return rc;
- }
- }
+ rc = acpi_bus_register_driver(&tis_acpi_driver);
+ if (rc)
+ goto err_acpi;
#endif
- if (!force)
- return 0;
- rc = platform_driver_register(&tis_drv);
- if (rc < 0)
- return rc;
- pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
- if (IS_ERR(pdev)) {
- rc = PTR_ERR(pdev);
- goto err_dev;
+ if (IS_ENABLED(CONFIG_PNP)) {
+ rc = pnp_register_driver(&tis_pnp_driver);
+ if (rc)
+ goto err_pnp;
}
- rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
- if (rc)
- goto err_init;
+
return 0;
-err_init:
- platform_device_unregister(pdev);
-err_dev:
- platform_driver_unregister(&tis_drv);
+
+err_pnp:
+#ifdef CONFIG_ACPI
+ acpi_bus_unregister_driver(&tis_acpi_driver);
+err_acpi:
+#endif
+ platform_device_unregister(force_pdev);
+err_platform:
+ if (force_pdev)
+ platform_device_unregister(force_pdev);
+err_force:
return rc;
}
static void __exit cleanup_tis(void)
{
- struct tpm_chip *chip;
-#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
- if (!force) {
+ pnp_unregister_driver(&tis_pnp_driver);
#ifdef CONFIG_ACPI
- acpi_bus_unregister_driver(&tis_acpi_driver);
-#endif
-#ifdef CONFIG_PNP
- pnp_unregister_driver(&tis_pnp_driver);
-#endif
- return;
- }
+ acpi_bus_unregister_driver(&tis_acpi_driver);
#endif
- chip = dev_get_drvdata(&pdev->dev);
- tpm_chip_unregister(chip);
- tpm_tis_remove(chip);
- platform_device_unregister(pdev);
platform_driver_unregister(&tis_drv);
+
+ if (force_pdev)
+ platform_device_unregister(force_pdev);
}
module_init(init_tis);
--
2.1.4
ioread32 and readl are defined to read from PCI style memory, ie little
endian and return the result in host order. On platforms where a
swap is required ioread32/readl do the swap internally (eg see ppc).
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_crb.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8dd70696ebe8..0237006dc4d8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -89,7 +89,7 @@ static u8 crb_status(struct tpm_chip *chip)
struct crb_priv *priv = chip->vendor.priv;
u8 sts = 0;
- if ((le32_to_cpu(ioread32(&priv->cca->start)) & CRB_START_INVOKE) !=
+ if ((ioread32(&priv->cca->start) & CRB_START_INVOKE) !=
CRB_START_INVOKE)
sts |= CRB_STS_COMPLETE;
@@ -105,7 +105,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
if (count < 6)
return -EIO;
- if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
+ if (ioread32(&priv->cca->sts) & CRB_CA_STS_ERROR)
return -EIO;
memcpy_fromio(buf, priv->rsp, 6);
@@ -141,11 +141,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
struct crb_priv *priv = chip->vendor.priv;
int rc = 0;
- if (len > le32_to_cpu(ioread32(&priv->cca->cmd_size))) {
+ if (len > ioread32(&priv->cca->cmd_size)) {
dev_err(&chip->dev,
"invalid command count value %x %zx\n",
(unsigned int) len,
- (size_t) le32_to_cpu(ioread32(&priv->cca->cmd_size)));
+ (size_t) ioread32(&priv->cca->cmd_size));
return -E2BIG;
}
@@ -181,7 +181,7 @@ static void crb_cancel(struct tpm_chip *chip)
static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
{
struct crb_priv *priv = chip->vendor.priv;
- u32 cancel = le32_to_cpu(ioread32(&priv->cca->cancel));
+ u32 cancel = ioread32(&priv->cca->cancel);
return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
}
@@ -251,10 +251,10 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENOMEM;
}
- pa = ((u64) le32_to_cpu(ioread32(&priv->cca->cmd_pa_high)) << 32) |
- (u64) le32_to_cpu(ioread32(&priv->cca->cmd_pa_low));
- priv->cmd = devm_ioremap_nocache(dev, pa,
- ioread32(&priv->cca->cmd_size));
+ pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
+ (u64)ioread32(&priv->cca->cmd_pa_low);
+ priv->cmd =
+ devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
if (!priv->cmd) {
dev_err(dev, "ioremap of the command buffer failed\n");
return -ENOMEM;
@@ -262,8 +262,8 @@ static int crb_acpi_add(struct acpi_device *device)
memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
pa = le64_to_cpu(pa);
- priv->rsp = devm_ioremap_nocache(dev, pa,
- ioread32(&priv->cca->rsp_size));
+ priv->rsp =
+ devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
if (!priv->rsp) {
dev_err(dev, "ioremap of the response buffer failed\n");
return -ENOMEM;
--
2.1.4
To support the force mode in tpm_tis we need to use resource locking
in tpm_crb as well, via devm_ioremap_resource.
The light restructuring better aligns crb and tis and makes it easier
to see the that new changes make sense.
Signed-off-by: Jason Gunthorpe <[email protected]>
Tested-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm_crb.c | 157 +++++++++++++++++++++++++++++++--------------
1 file changed, 108 insertions(+), 49 deletions(-)
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 0237006dc4d8..1f844cc63016 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -77,6 +77,8 @@ enum crb_flags {
struct crb_priv {
unsigned int flags;
+ struct resource res;
+ void __iomem *iobase;
struct crb_control_area __iomem *cca;
u8 __iomem *cmd;
u8 __iomem *rsp;
@@ -196,22 +198,121 @@ static const struct tpm_class_ops tpm_crb = {
.req_complete_val = CRB_STS_COMPLETE,
};
-static int crb_acpi_add(struct acpi_device *device)
+static int crb_init(struct acpi_device *device, struct crb_priv *priv)
{
struct tpm_chip *chip;
+ int rc;
+
+ chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
+
+ chip->vendor.priv = priv;
+ chip->acpi_dev_handle = device->handle;
+ chip->flags = TPM_CHIP_FLAG_TPM2;
+
+ rc = tpm_get_timeouts(chip);
+ if (rc)
+ return rc;
+
+ rc = tpm2_do_selftest(chip);
+ if (rc)
+ return rc;
+
+ return tpm_chip_register(chip);
+}
+
+static int crb_check_resource(struct acpi_resource *ares, void *data)
+{
+ struct crb_priv *priv = data;
+ struct resource res;
+
+ if (acpi_dev_resource_memory(ares, &res))
+ priv->res = res;
+
+ return 1;
+}
+
+static void __iomem *crb_access(struct device *dev, struct crb_priv *priv,
+ u64 start, u32 size)
+{
+ struct resource tmp = {};
+
+ tmp.start = start;
+ tmp.end = start + size - 1;
+ tmp.flags = IORESOURCE_MEM;
+
+ /* Detect a 64 bit address on a 32 bit system */
+ if (start != tmp.start)
+ return ERR_PTR(-EINVAL);
+
+ if (!resource_contains(&priv->res, &tmp)) {
+ dev_err(dev,
+ FW_BUG "TPM2 ACPI sub resource %pR is not in the device's region of %pR\n",
+ &tmp, &priv->res);
+ return ERR_PTR(-EINVAL);
+ }
+
+ return priv->iobase + (tmp.start - priv->res.start);
+}
+
+static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
+ struct acpi_table_tpm2 *buf)
+{
+ struct list_head resources;
+ struct device *dev = &device->dev;
+ u64 pa;
+ int ret;
+
+ INIT_LIST_HEAD(&resources);
+ ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
+ priv);
+ if (ret < 0)
+ return ret;
+ acpi_dev_free_resource_list(&resources);
+
+ if (resource_type(&priv->res) != IORESOURCE_MEM) {
+ dev_err(dev,
+ FW_BUG "TPM2 ACPI table does not define a memory resource\n");
+ return -EINVAL;
+ }
+
+ priv->iobase = devm_ioremap_resource(dev, &priv->res);
+ if (IS_ERR(priv->iobase))
+ return PTR_ERR(priv->iobase);
+
+ priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);
+ if (IS_ERR(priv->cca))
+ return PTR_ERR(priv->cca);
+
+ pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
+ (u64)ioread32(&priv->cca->cmd_pa_low);
+ priv->cmd = crb_access(dev, priv, pa, ioread32(&priv->cca->cmd_size));
+ if (IS_ERR(priv->cmd))
+ return PTR_ERR(priv->cmd);
+
+ memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
+ pa = le64_to_cpu(pa);
+ priv->rsp = crb_access(dev, priv, pa, ioread32(&priv->cca->rsp_size));
+ if (IS_ERR(priv->rsp))
+ return PTR_ERR(priv->rsp);
+ return 0;
+}
+
+static int crb_acpi_add(struct acpi_device *device)
+{
struct acpi_table_tpm2 *buf;
struct crb_priv *priv;
struct device *dev = &device->dev;
acpi_status status;
u32 sm;
- u64 pa;
int rc;
status = acpi_get_table(ACPI_SIG_TPM2, 1,
(struct acpi_table_header **) &buf);
if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
- return -ENODEV;
+ return -EINVAL;
}
/* Should the FIFO driver handle this? */
@@ -219,18 +320,9 @@ static int crb_acpi_add(struct acpi_device *device)
if (sm == ACPI_TPM2_MEMORY_MAPPED)
return -ENODEV;
- chip = tpmm_chip_alloc(dev, &tpm_crb);
- if (IS_ERR(chip))
- return PTR_ERR(chip);
-
- chip->flags = TPM_CHIP_FLAG_TPM2;
-
- priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
- GFP_KERNEL);
- if (!priv) {
- dev_err(dev, "failed to devm_kzalloc for private data\n");
+ priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
+ if (!priv)
return -ENOMEM;
- }
/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
* report only ACPI start but in practice seems to require both
@@ -244,44 +336,11 @@ static int crb_acpi_add(struct acpi_device *device)
sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
priv->flags |= CRB_FL_ACPI_START;
- priv->cca = (struct crb_control_area __iomem *)
- devm_ioremap_nocache(dev, buf->control_address, 0x1000);
- if (!priv->cca) {
- dev_err(dev, "ioremap of the control area failed\n");
- return -ENOMEM;
- }
-
- pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
- (u64)ioread32(&priv->cca->cmd_pa_low);
- priv->cmd =
- devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
- if (!priv->cmd) {
- dev_err(dev, "ioremap of the command buffer failed\n");
- return -ENOMEM;
- }
-
- memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
- pa = le64_to_cpu(pa);
- priv->rsp =
- devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
- if (!priv->rsp) {
- dev_err(dev, "ioremap of the response buffer failed\n");
- return -ENOMEM;
- }
-
- chip->vendor.priv = priv;
-
- rc = tpm_get_timeouts(chip);
- if (rc)
- return rc;
-
- chip->acpi_dev_handle = device->handle;
-
- rc = tpm2_do_selftest(chip);
+ rc = crb_map_io(device, priv, buf);
if (rc)
return rc;
- return tpm_chip_register(chip);
+ return crb_init(device, priv);
}
static int crb_acpi_remove(struct acpi_device *device)
--
2.1.4
On Thu, Dec 17, 2015 at 11:23:16AM -0700, Jason Gunthorpe wrote:
> If the ACPI tables do not declare a memory resource for the TPM2
> then do not just fall back to the x86 default base address.
>
> Also be stricter when checking the ancillary TPM2 ACPI data and error
> out if any of this data is wrong rather than blindly assuming TPM1.
>
> Fixes: 399235dc6e95 ("tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0")
> Signed-off-by: Jason Gunthorpe <[email protected]>
> Tested-by: Wilck, Martin <[email protected]>
> Tested-by: Jarkko Sakkinen <[email protected]>
> ---
> drivers/char/tpm/tpm_tis.c | 48 +++++++++++++++++-----------------------------
> 1 file changed, 18 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index fecd27b45fd1..b2b31f5418ca 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,39 +122,11 @@ static inline int is_itpm(struct acpi_device *dev)
> {
> return has_hid(dev, "INTC0102");
> }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> - struct acpi_table_tpm2 *tbl;
> - acpi_status st;
> -
> - /* TPM 1.2 FIFO */
> - if (!has_hid(dev, "MSFT0101"))
> - return 1;
> -
> - st = acpi_get_table(ACPI_SIG_TPM2, 1,
> - (struct acpi_table_header **) &tbl);
> - if (ACPI_FAILURE(st)) {
> - dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
> - return 0;
> - }
> -
> - if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> - return 0;
> -
> - /* TPM 2.0 FIFO */
> - return 1;
> -}
> #else
> static inline int is_itpm(struct acpi_device *dev)
> {
> return 0;
> }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> - return 1;
> -}
> #endif
>
> /* Before we attempt to access the TPM we must see that the valid bit is set.
> @@ -980,11 +952,21 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
>
> static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> {
> + struct acpi_table_tpm2 *tbl;
> + acpi_status st;
> struct list_head resources;
> - struct tpm_info tpm_info = tis_default_info;
> + struct tpm_info tpm_info = {};
> int ret;
>
> - if (!is_fifo(acpi_dev))
> + st = acpi_get_table(ACPI_SIG_TPM2, 1,
> + (struct acpi_table_header **) &tbl);
> + if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> + dev_err(&acpi_dev->dev,
> + FW_BUG "failed to get TPM2 ACPI table\n");
> + return -EINVAL;
> + }
> +
> + if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> return -ENODEV;
>
> INIT_LIST_HEAD(&resources);
> @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>
> acpi_dev_free_resource_list(&resources);
>
> + if (tpm_info.start == 0 && tpm_info.len == 0) {
> + dev_err(&acpi_dev->dev,
> + FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> + return -EINVAL;
> + }
> +
I guess this the only relevant change in this patch? You should propose
removal of is_fifo() as a separate patch if that makes sense. This patch
is now doing orthogonal things.
> if (is_itpm(acpi_dev))
> itpm = true;
>
> --
> 2.1.4
/Jarkko
On Fri, Dec 18, 2015 at 11:34:32AM +0200, Jarkko Sakkinen wrote:
> > + st = acpi_get_table(ACPI_SIG_TPM2, 1,
> > + (struct acpi_table_header **) &tbl);
> > + if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> > + dev_err(&acpi_dev->dev,
> > + FW_BUG "failed to get TPM2 ACPI table\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> > return -ENODEV;
> >
> > INIT_LIST_HEAD(&resources);
> > @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> >
> > acpi_dev_free_resource_list(&resources);
> >
> > + if (tpm_info.start == 0 && tpm_info.len == 0) {
> > + dev_err(&acpi_dev->dev,
> > + FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> > + return -EINVAL;
> > + }
> > +
>
> I guess this the only relevant change in this patch? You should propose
> removal of is_fifo() as a separate patch if that makes sense. This patch
> is now doing orthogonal things.
No, the return code changes are relevant too, and are why is_fifo was
best un-inlined.
The patch is fixing all the ACPI data validatation in one go, not just
the resource range, the description notes this.
Jason
On Fri, Dec 18, 2015 at 09:51:27AM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 18, 2015 at 11:34:32AM +0200, Jarkko Sakkinen wrote:
> > > + st = acpi_get_table(ACPI_SIG_TPM2, 1,
> > > + (struct acpi_table_header **) &tbl);
> > > + if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> > > + dev_err(&acpi_dev->dev,
> > > + FW_BUG "failed to get TPM2 ACPI table\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> > > return -ENODEV;
> > >
> > > INIT_LIST_HEAD(&resources);
> > > @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> > >
> > > acpi_dev_free_resource_list(&resources);
> > >
> > > + if (tpm_info.start == 0 && tpm_info.len == 0) {
> > > + dev_err(&acpi_dev->dev,
> > > + FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> > > + return -EINVAL;
> > > + }
> > > +
> >
> > I guess this the only relevant change in this patch? You should propose
> > removal of is_fifo() as a separate patch if that makes sense. This patch
> > is now doing orthogonal things.
>
> No, the return code changes are relevant too, and are why is_fifo was
> best un-inlined.
>
> The patch is fixing all the ACPI data validatation in one go, not just
> the resource range, the description notes this.
Got you. I think I'm good with this patch.
Reviewed-by: Jarkko Sakkinen <[email protected]>
> Jason
/Jarkko