Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
[PATCH 0/2] Fix tpm_tis ACPI issue with TPM 2.0
317 views
Skip to first unread message
Jarkko Sakkinen
Sep 29, 2015, 1:10:06 PM9/29/15
to
Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method
from TPM2 ACPI table. These patches make necessary fixes to tpm_tis
and tpm_crb modules in order to correctly detect, which module should
be used.
Jarkko Sakkinen (2):
tpm, tpm_tis: use acpi_driver instead of pnp_driver
tpm, tpm_tis: detect TPM2 FIFO devices based on HID
drivers/char/tpm/tpm.h | 7 ++
drivers/char/tpm/tpm_crb.c | 20 ++----
drivers/char/tpm/tpm_tis.c | 160 ++++++++++++++++++++++++++++++---------------
3 files changed, 121 insertions(+), 66 deletions(-)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
MSFT0101. They can be differentiated by looking at the start method
from TPM2 ACPI table. These patches make necessary fixes to tpm_tis
and tpm_crb modules in order to correctly detect, which module should
be used.
Jarkko Sakkinen (2):
tpm, tpm_tis: use acpi_driver instead of pnp_driver
tpm, tpm_tis: detect TPM2 FIFO devices based on HID
drivers/char/tpm/tpm.h | 7 ++
drivers/char/tpm/tpm_crb.c | 20 ++----
drivers/char/tpm/tpm_tis.c | 160 ++++++++++++++++++++++++++++++---------------
3 files changed, 121 insertions(+), 66 deletions(-)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Jarkko Sakkinen
Sep 29, 2015, 1:10:07 PM9/29/15
to
It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.
All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
the start method is always 6.
This patch changes FIFO and CRB drivers so that they check start method
value and initialize only if the start method has a proper value for
that particular interface.
Reported-by: Michael Saunders <mick.s...@gmail.com>
Reported-by: Michael Marley <mic...@michaelmarley.com>
Reported-by: Jethro Beekman <ker...@jbeekman.nl>
Reported-by: Matthew Garrett <mj...@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
drivers/char/tpm/tpm.h | 7 +++++++
drivers/char/tpm/tpm_crb.c | 20 +++++---------------
drivers/char/tpm/tpm_tis.c | 40 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
};
-enum crb_start_method {
- CRB_SM_ACPI_START = 2,
- CRB_SM_CRB = 7,
- CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
- /* At least some versions of AMI BIOS have a bug that TPM2 table has
- * zero address for the control area and therefore we must fail.
- */
- if (!buf->control_area_pa) {
- dev_err(dev, "TPM2 ACPI table has a zero address for the control area\n");
- return -EINVAL;
- }
+ /* Should the FIFO driver handle this? */
+ if (buf->start_method == TPM2_START_FIFO)
+ return -ENODEV;
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
- if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+ if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
- if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+ if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 87581b2..545a38e 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -27,6 +27,7 @@
#include <linux/wait.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <acpi/actbl2.h>
#include "tpm.h"
enum tis_access {
@@ -101,21 +102,52 @@ struct priv_data {
};
#if defined(CONFIG_ACPI)
-static int is_itpm(struct acpi_device *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
{
struct acpi_hardware_id *id;
list_for_each_entry(id, &dev->pnp.ids, list)
- if (!strcmp("INTC0102", id->id))
+ if (!strcmp(hid, id->id))
return 1;
return 0;
}
+
+static inline int is_itpm(struct acpi_device *dev)
+{
+ return has_hid(dev, "INTC0102");
+}
+
+static inline int is_tpm2_fifo(struct acpi_device *dev)
+{
+ struct acpi_table_tpm2 *tbl;
+ acpi_status st;
+
+ if (!has_hid(dev, "MSFT0101"))
+ return 0;
+
+ 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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+ return 0;
+
+ return 1;
+}
#else
static inline int is_itpm(struct acpi_device *dev)
{
return 0;
}
+
+static inline int is_tpm2_fifo(struct acpi_device *dev)
+{
+ return 0;
+}
#endif
/* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -912,6 +944,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
struct tpm_info tpm_info = tis_default_info;
int ret;
+ if (!is_tpm2_fifo(acpi_dev))
+ return -ENODEV;
+
INIT_LIST_HEAD(&resources);
ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
&tpm_info);
@@ -937,6 +972,7 @@ static struct acpi_device_id tpm_acpi_tbl[] = {
{"BCM0102", 0}, /* Broadcom */
{"NSC1200", 0}, /* National */
{"ICO0102", 0}, /* Intel */
+ {"MSFT0101", 0}, /* TPM 2.0 */
/* Add new here */
{"", 0}, /* User Specified */
{"", 0} /* Terminator */
https://bugzilla.kernel.org/show_bug.cgi?id=98181.
All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
the start method is always 6.
This patch changes FIFO and CRB drivers so that they check start method
value and initialize only if the start method has a proper value for
that particular interface.
Reported-by: Michael Saunders <mick.s...@gmail.com>
Reported-by: Michael Marley <mic...@michaelmarley.com>
Reported-by: Jethro Beekman <ker...@jbeekman.nl>
Reported-by: Matthew Garrett <mj...@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
drivers/char/tpm/tpm.h | 7 +++++++
drivers/char/tpm/tpm_crb.c | 20 +++++---------------
drivers/char/tpm/tpm_tis.c | 40 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ 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 1267322..b4564b6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
};
-enum crb_start_method {
- CRB_SM_ACPI_START = 2,
- CRB_SM_CRB = 7,
- CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
- /* At least some versions of AMI BIOS have a bug that TPM2 table has
- * zero address for the control area and therefore we must fail.
- */
- if (!buf->control_area_pa) {
- dev_err(dev, "TPM2 ACPI table has a zero address for the control area\n");
- return -EINVAL;
- }
+ /* Should the FIFO driver handle this? */
+ if (buf->start_method == TPM2_START_FIFO)
+ return -ENODEV;
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
- if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+ if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
- if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+ if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 87581b2..545a38e 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -27,6 +27,7 @@
#include <linux/wait.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <acpi/actbl2.h>
#include "tpm.h"
enum tis_access {
@@ -101,21 +102,52 @@ struct priv_data {
};
#if defined(CONFIG_ACPI)
-static int is_itpm(struct acpi_device *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
{
struct acpi_hardware_id *id;
list_for_each_entry(id, &dev->pnp.ids, list)
- if (!strcmp("INTC0102", id->id))
+ if (!strcmp(hid, id->id))
return 1;
return 0;
}
+
+static inline int is_itpm(struct acpi_device *dev)
+{
+ return has_hid(dev, "INTC0102");
+}
+
+static inline int is_tpm2_fifo(struct acpi_device *dev)
+{
+ struct acpi_table_tpm2 *tbl;
+ acpi_status st;
+
+ if (!has_hid(dev, "MSFT0101"))
+ return 0;
+
+ 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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+ return 0;
+
+ return 1;
+}
#else
static inline int is_itpm(struct acpi_device *dev)
{
return 0;
}
+
+static inline int is_tpm2_fifo(struct acpi_device *dev)
+{
+ return 0;
+}
#endif
/* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -912,6 +944,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
struct tpm_info tpm_info = tis_default_info;
int ret;
+ if (!is_tpm2_fifo(acpi_dev))
+ return -ENODEV;
+
INIT_LIST_HEAD(&resources);
ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
&tpm_info);
@@ -937,6 +972,7 @@ static struct acpi_device_id tpm_acpi_tbl[] = {
{"BCM0102", 0}, /* Broadcom */
{"NSC1200", 0}, /* National */
{"ICO0102", 0}, /* Intel */
+ {"MSFT0101", 0}, /* TPM 2.0 */
/* Add new here */
{"", 0}, /* User Specified */
{"", 0} /* Terminator */
Jarkko Sakkinen
Sep 29, 2015, 1:10:07 PM9/29/15
to
Migrate to struct acpi_driver in order to get out of 7 character
limitation for the HID.
Reported-by: Matthew Garrett <mj...@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
drivers/char/tpm/tpm_tis.c | 124 ++++++++++++++++++++++++++-------------------
1 file changed, 73 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..87581b2 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
/*
* Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
*
* Authors:
* Leendert van Doorn <leen...@watson.ibm.com>
@@ -22,7 +22,6 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
-#include <linux/pnp.h>
#include <linux/slab.h>
#include <linux/interrupt.h>
#include <linux/wait.h>
@@ -65,6 +64,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000, /* 2 sec */
};
+struct tpm_info {
+ unsigned long start;
+ unsigned long len;
+ unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+ .start = TIS_MEM_BASE,
+ .len = TIS_MEM_LEN,
+ .irq = 0,
+};
/* Some timeout values are needed before it is known whether the chip is
* TPM 1.0 or TPM 2.0.
@@ -90,24 +100,19 @@ struct priv_data {
bool irq_tested;
};
-#if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
-static int is_itpm(struct pnp_dev *dev)
+#if defined(CONFIG_ACPI)
+static int is_itpm(struct acpi_device *dev)
{
- struct acpi_device *acpi = pnp_acpi_device(dev);
struct acpi_hardware_id *id;
- if (!acpi)
- return 0;
-
- list_for_each_entry(id, &acpi->pnp.ids, list) {
+ list_for_each_entry(id, &dev->pnp.ids, list)
- }
return 0;
}
#else
-static inline int is_itpm(struct pnp_dev *dev)
return 0;
}
@@ -600,9 +605,8 @@ static void tpm_tis_remove(struct tpm_chip *chip)
release_locality(chip, chip->vendor.locality, 1);
}
-static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
- resource_size_t start, resource_size_t len,
- unsigned int irq)
+static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
+ acpi_handle acpi_dev_handle)
{
u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
@@ -622,7 +626,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
- chip->vendor.iobase = devm_ioremap(dev, start, len);
+ chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
if (!chip->vendor.iobase)
return -EIO;
@@ -707,7 +711,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));
if (interrupts)
- chip->vendor.irq = irq;
+ chip->vendor.irq = tpm_info->irq;
if (interrupts && !chip->vendor.irq) {
irq_s =
ioread8(chip->vendor.iobase +
@@ -886,34 +890,46 @@ static int tpm_tis_resume(struct device *dev)
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)
+#ifdef CONFIG_ACPI
+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)) {
+ tpm_info->irq = res.start;
+ } else if (acpi_dev_resource_memory(ares, &res)) {
+ tpm_info->start = res.start;
+ tpm_info->len = resource_size(&res);
+ }
+static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
{
- resource_size_t start, len;
- unsigned int irq = 0;
- acpi_handle acpi_dev_handle = NULL;
+ struct list_head resources;
+ struct tpm_info tpm_info = tis_default_info;
+ int ret;
+
+ INIT_LIST_HEAD(&resources);
+ ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
+ &tpm_info);
+ if (ret < 0)
+ return ret;
- start = pnp_mem_start(pnp_dev, 0);
- len = pnp_mem_len(pnp_dev, 0);
+ acpi_dev_free_resource_list(&resources);
- if (pnp_irq_valid(pnp_dev, 0))
- irq = pnp_irq(pnp_dev, 0);
- else
+ if (!tpm_info.irq)
interrupts = false;
- if (is_itpm(pnp_dev))
+ if (is_itpm(acpi_dev))
itpm = true;
-#ifdef CONFIG_ACPI
- if (pnp_acpi_device(pnp_dev))
- acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
-#endif
-
- return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start, len, irq);
+ return tpm_tis_init(&acpi_dev->dev, &tpm_info, acpi_dev->handle);
}
-static struct pnp_device_id tpm_pnp_tbl[] = {
+static struct acpi_device_id tpm_acpi_tbl[] = {
{"PNP0C31", 0}, /* TPM */
{"ATM1200", 0}, /* Atmel */
{"IFX0102", 0}, /* Infineon */
@@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
-MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
+MODULE_DEVICE_TABLE(acpi, tpm_acpi_tbl);
-static void tpm_tis_pnp_remove(struct pnp_dev *dev)
+static int tpm_tis_acpi_remove(struct acpi_device *dev)
{
- struct tpm_chip *chip = pnp_get_drvdata(dev);
+ struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
+
tpm_chip_unregister(chip);
tpm_tis_remove(chip);
+
+ return 0;
}
-static struct pnp_driver tis_pnp_driver = {
+static struct acpi_driver tis_acpi_driver = {
.name = "tpm_tis",
- .id_table = tpm_pnp_tbl,
- .probe = tpm_tis_pnp_init,
- .remove = tpm_tis_pnp_remove,
- .driver = {
+ .ids = tpm_acpi_tbl,
+ .ops = {
+ .add = tpm_tis_acpi_init,
+ .remove = tpm_tis_acpi_remove,
+ },
+ .drv = {
.pm = &tpm_tis_pm,
},
};
-#define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
-module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
- sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
+#define TIS_HID_USR_IDX \
+ (sizeof(tpm_acpi_tbl) / sizeof(struct acpi_device_id) - 2)
+module_param_string(hid, tpm_acpi_tbl[TIS_HID_USR_IDX].id,
+ sizeof(tpm_acpi_tbl[TIS_HID_USR_IDX].id), 0444);
MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
#endif
@@ -965,9 +987,9 @@ MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
static int __init init_tis(void)
{
int rc;
-#ifdef CONFIG_PNP
+#ifdef CONFIG_ACPI
if (!force)
- return pnp_register_driver(&tis_pnp_driver);
+ return acpi_bus_register_driver(&tis_acpi_driver);
#endif
rc = platform_driver_register(&tis_drv);
@@ -978,7 +1000,7 @@ static int __init init_tis(void)
rc = PTR_ERR(pdev);
goto err_dev;
}
- rc = tpm_tis_init(&pdev->dev, NULL, TIS_MEM_BASE, TIS_MEM_LEN, 0);
+ rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
if (rc)
goto err_init;
return 0;
@@ -992,9 +1014,9 @@ err_dev:
static void __exit cleanup_tis(void)
{
struct tpm_chip *chip;
-#ifdef CONFIG_PNP
+#ifdef CONFIG_ACPI
if (!force) {
- pnp_unregister_driver(&tis_pnp_driver);
+ acpi_bus_unregister_driver(&tis_acpi_driver);
return;
}
#endif
limitation for the HID.
Reported-by: Matthew Garrett <mj...@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
1 file changed, 73 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..87581b2 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -1,6 +1,6 @@
/*
* Copyright (C) 2005, 2006 IBM Corporation
- * Copyright (C) 2014 Intel Corporation
+ * Copyright (C) 2014, 2015 Intel Corporation
*
* Authors:
* Leendert van Doorn <leen...@watson.ibm.com>
@@ -22,7 +22,6 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
-#include <linux/pnp.h>
#include <linux/slab.h>
#include <linux/interrupt.h>
#include <linux/wait.h>
@@ -65,6 +64,17 @@ enum tis_defaults {
TIS_LONG_TIMEOUT = 2000, /* 2 sec */
};
+struct tpm_info {
+ unsigned long start;
+ unsigned long len;
+ unsigned int irq;
+};
+
+static struct tpm_info tis_default_info = {
+ .start = TIS_MEM_BASE,
+ .len = TIS_MEM_LEN,
+ .irq = 0,
+};
/* Some timeout values are needed before it is known whether the chip is
* TPM 1.0 or TPM 2.0.
@@ -90,24 +100,19 @@ struct priv_data {
bool irq_tested;
};
-#if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
-static int is_itpm(struct pnp_dev *dev)
+#if defined(CONFIG_ACPI)
+static int is_itpm(struct acpi_device *dev)
{
- struct acpi_device *acpi = pnp_acpi_device(dev);
struct acpi_hardware_id *id;
- if (!acpi)
- return 0;
-
- list_for_each_entry(id, &acpi->pnp.ids, list) {
+ list_for_each_entry(id, &dev->pnp.ids, list)
if (!strcmp("INTC0102", id->id))
return 1;
- }
return 0;
}
#else
-static inline int is_itpm(struct pnp_dev *dev)
+static inline int is_itpm(struct acpi_device *dev)
{
return 0;
}
@@ -600,9 +605,8 @@ static void tpm_tis_remove(struct tpm_chip *chip)
release_locality(chip, chip->vendor.locality, 1);
}
-static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
- resource_size_t start, resource_size_t len,
- unsigned int irq)
+static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
+ acpi_handle acpi_dev_handle)
{
u32 vendor, intfcaps, intmask;
int rc, i, irq_s, irq_e, probe;
@@ -622,7 +626,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
chip->acpi_dev_handle = acpi_dev_handle;
#endif
- chip->vendor.iobase = devm_ioremap(dev, start, len);
+ chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
if (!chip->vendor.iobase)
return -EIO;
@@ -707,7 +711,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.locality));
if (interrupts)
- chip->vendor.irq = irq;
+ chip->vendor.irq = tpm_info->irq;
if (interrupts && !chip->vendor.irq) {
irq_s =
ioread8(chip->vendor.iobase +
@@ -886,34 +890,46 @@ static int tpm_tis_resume(struct device *dev)
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)
+#ifdef CONFIG_ACPI
+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)) {
+ tpm_info->irq = res.start;
+ } else if (acpi_dev_resource_memory(ares, &res)) {
+ tpm_info->start = res.start;
+ tpm_info->len = resource_size(&res);
+ }
+
+ return 1;
+}
+
+ return 1;
+}
+static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
{
- resource_size_t start, len;
- unsigned int irq = 0;
- acpi_handle acpi_dev_handle = NULL;
+ struct list_head resources;
+ struct tpm_info tpm_info = tis_default_info;
+ int ret;
+
+ INIT_LIST_HEAD(&resources);
+ ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
+ &tpm_info);
+ if (ret < 0)
+ return ret;
- start = pnp_mem_start(pnp_dev, 0);
- len = pnp_mem_len(pnp_dev, 0);
+ acpi_dev_free_resource_list(&resources);
- if (pnp_irq_valid(pnp_dev, 0))
- irq = pnp_irq(pnp_dev, 0);
- else
+ if (!tpm_info.irq)
interrupts = false;
- if (is_itpm(pnp_dev))
+ if (is_itpm(acpi_dev))
itpm = true;
-#ifdef CONFIG_ACPI
- if (pnp_acpi_device(pnp_dev))
- acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
-#endif
-
- return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start, len, irq);
+ return tpm_tis_init(&acpi_dev->dev, &tpm_info, acpi_dev->handle);
}
-static struct pnp_device_id tpm_pnp_tbl[] = {
+static struct acpi_device_id tpm_acpi_tbl[] = {
{"PNP0C31", 0}, /* TPM */
{"ATM1200", 0}, /* Atmel */
{"IFX0102", 0}, /* Infineon */
@@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
{"", 0}, /* User Specified */
{"", 0} /* Terminator */
};
{"", 0} /* Terminator */
-MODULE_DEVICE_TABLE(pnp, tpm_pnp_tbl);
+MODULE_DEVICE_TABLE(acpi, tpm_acpi_tbl);
-static void tpm_tis_pnp_remove(struct pnp_dev *dev)
+static int tpm_tis_acpi_remove(struct acpi_device *dev)
{
- struct tpm_chip *chip = pnp_get_drvdata(dev);
+ struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
+
tpm_chip_unregister(chip);
tpm_tis_remove(chip);
+
+ return 0;
}
-static struct pnp_driver tis_pnp_driver = {
+static struct acpi_driver tis_acpi_driver = {
.name = "tpm_tis",
- .id_table = tpm_pnp_tbl,
- .probe = tpm_tis_pnp_init,
- .remove = tpm_tis_pnp_remove,
- .driver = {
+ .ids = tpm_acpi_tbl,
+ .ops = {
+ .add = tpm_tis_acpi_init,
+ .remove = tpm_tis_acpi_remove,
+ },
+ .drv = {
.pm = &tpm_tis_pm,
},
};
-#define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2
-module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
- sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
+#define TIS_HID_USR_IDX \
+ (sizeof(tpm_acpi_tbl) / sizeof(struct acpi_device_id) - 2)
+module_param_string(hid, tpm_acpi_tbl[TIS_HID_USR_IDX].id,
+ sizeof(tpm_acpi_tbl[TIS_HID_USR_IDX].id), 0444);
MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
#endif
@@ -965,9 +987,9 @@ MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
static int __init init_tis(void)
{
int rc;
-#ifdef CONFIG_PNP
+#ifdef CONFIG_ACPI
if (!force)
- return pnp_register_driver(&tis_pnp_driver);
+ return acpi_bus_register_driver(&tis_acpi_driver);
#endif
rc = platform_driver_register(&tis_drv);
@@ -978,7 +1000,7 @@ static int __init init_tis(void)
rc = PTR_ERR(pdev);
goto err_dev;
}
- rc = tpm_tis_init(&pdev->dev, NULL, TIS_MEM_BASE, TIS_MEM_LEN, 0);
+ rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
if (rc)
goto err_init;
return 0;
@@ -992,9 +1014,9 @@ err_dev:
static void __exit cleanup_tis(void)
{
struct tpm_chip *chip;
-#ifdef CONFIG_PNP
+#ifdef CONFIG_ACPI
if (!force) {
- pnp_unregister_driver(&tis_pnp_driver);
+ acpi_bus_unregister_driver(&tis_acpi_driver);
return;
}
#endif
Jason Gunthorpe
Sep 29, 2015, 1:30:07 PM9/29/15
to
On Tue, Sep 29, 2015 at 08:07:10PM +0300, Jarkko Sakkinen wrote:
> -static struct pnp_device_id tpm_pnp_tbl[] = {
> +static struct acpi_device_id tpm_acpi_tbl[] = {
> {"PNP0C31", 0}, /* TPM */
> {"ATM1200", 0}, /* Atmel */
> {"IFX0102", 0}, /* Infineon */
> @@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
> {"", 0}, /* User Specified */
> {"", 0} /* Terminator */
> };
Is this OK? I don't know alot about x86 PNP, but I thought the
> -static struct pnp_device_id tpm_pnp_tbl[] = {
> +static struct acpi_device_id tpm_acpi_tbl[] = {
> {"PNP0C31", 0}, /* TPM */
> {"ATM1200", 0}, /* Atmel */
> {"IFX0102", 0}, /* Infineon */
> @@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
> {"", 0}, /* User Specified */
> {"", 0} /* Terminator */
> };
pnp_device_id scheme would work with ACPI and legacy PNPBIOS stuff,
and changing to ACPI means ACPI only?
If so, should we care? Is there a spec for non-ACPI TPM discovery we
need to be following here?
> struct tpm_chip *chip;
> -#ifdef CONFIG_PNP
> +#ifdef CONFIG_ACPI
like:
> if (!force) {
> - pnp_unregister_driver(&tis_pnp_driver);
> + acpi_bus_unregister_driver(&tis_acpi_driver);
acpi_bus_unregister_driver(&tis_acpi_driver);
I think alot of the core driver stuff supports that now?
Jason
kbuild test robot
Sep 29, 2015, 1:50:09 PM9/29/15
to
Hi Jarkko,
[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/char/tpm/tpm_tis.c:136:13: sparse: cast to restricted __le32
vim +136 drivers/char/tpm/tpm_tis.c
120
121 static inline int is_tpm2_fifo(struct acpi_device *dev)
122 {
123 struct acpi_table_tpm2 *tbl;
124 acpi_status st;
125
126 if (!has_hid(dev, "MSFT0101"))
127 return 0;
128
129 st = acpi_get_table(ACPI_SIG_TPM2, 1,
130 (struct acpi_table_header **) &tbl);
131 if (ACPI_FAILURE(st)) {
132 dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
133 return 0;
134 }
135
> 136 if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
137 return 0;
138
139 return 1;
140 }
141 #else
142 static inline int is_itpm(struct acpi_device *dev)
143 {
144 return 0;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/char/tpm/tpm_tis.c:136:13: sparse: cast to restricted __le32
vim +136 drivers/char/tpm/tpm_tis.c
120
121 static inline int is_tpm2_fifo(struct acpi_device *dev)
122 {
123 struct acpi_table_tpm2 *tbl;
124 acpi_status st;
125
126 if (!has_hid(dev, "MSFT0101"))
127 return 0;
128
129 st = acpi_get_table(ACPI_SIG_TPM2, 1,
130 (struct acpi_table_header **) &tbl);
131 if (ACPI_FAILURE(st)) {
132 dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
133 return 0;
134 }
135
> 136 if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
137 return 0;
138
139 return 1;
140 }
141 #else
142 static inline int is_itpm(struct acpi_device *dev)
143 {
144 return 0;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Matthew Garrett
Sep 29, 2015, 2:10:09 PM9/29/15
to
On Tue, Sep 29, 2015 at 08:07:10PM +0300, Jarkko Sakkinen wrote:
> Migrate to struct acpi_driver in order to get out of 7 character
> limitation for the HID.
Are we guaranteed that there are no old systems reporting these devices
> limitation for the HID.
via pnpbios rather than acpi?
--
Matthew Garrett | mj...@srcf.ucam.org
Jarkko Sakkinen
Sep 29, 2015, 2:20:08 PM9/29/15
to
It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.
All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
the start method is always 6.
This patch changes FIFO and CRB drivers so that they check start method
value and initialize only if the start method has a proper value for
that particular interface.
Reported-by: Michael Saunders <mick.s...@gmail.com>
Reported-by: Michael Marley <mic...@michaelmarley.com>
Reported-by: Jethro Beekman <ker...@jbeekman.nl>
https://bugzilla.kernel.org/show_bug.cgi?id=98181.
All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
the start method is always 6.
This patch changes FIFO and CRB drivers so that they check start method
value and initialize only if the start method has a proper value for
that particular interface.
Reported-by: Michael Saunders <mick.s...@gmail.com>
Reported-by: Michael Marley <mic...@michaelmarley.com>
Reported-by: Jethro Beekman <ker...@jbeekman.nl>
Reported-by: Matthew Garrett <mj...@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
drivers/char/tpm/tpm.h | 7 +++++++
drivers/char/tpm/tpm_crb.c | 20 +++++---------------
drivers/char/tpm/tpm_tis.c | 42 ++++++++++++++++++++++++++++++++++++++++--
drivers/char/tpm/tpm_crb.c | 20 +++++---------------
3 files changed, 52 insertions(+), 17 deletions(-)
index 87581b2..de61b9f 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -27,6 +27,7 @@
#include <linux/wait.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <acpi/actbl2.h>
#include "tpm.h"
enum tis_access {
@@ -101,21 +102,54 @@ struct priv_data {
#include <linux/wait.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <acpi/actbl2.h>
#include "tpm.h"
enum tis_access {
};
#if defined(CONFIG_ACPI)
-static int is_itpm(struct acpi_device *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
{
struct acpi_hardware_id *id;
list_for_each_entry(id, &dev->pnp.ids, list)
- if (!strcmp("INTC0102", id->id))
+ if (!strcmp(hid, id->id))
return 1;
return 0;
}
+
+static inline int is_itpm(struct acpi_device *dev)
+{
+ return has_hid(dev, "INTC0102");
+}
+
+static inline int is_fifo(struct acpi_device *dev)
#if defined(CONFIG_ACPI)
-static int is_itpm(struct acpi_device *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
{
struct acpi_hardware_id *id;
list_for_each_entry(id, &dev->pnp.ids, list)
- if (!strcmp("INTC0102", id->id))
+ if (!strcmp(hid, id->id))
return 1;
return 0;
}
+
+static inline int is_itpm(struct acpi_device *dev)
+{
+ return has_hid(dev, "INTC0102");
+}
+
+{
+ struct acpi_table_tpm2 *tbl;
+ acpi_status st;
+
+ /* TPM 1.2 FIFO */
+ struct acpi_table_tpm2 *tbl;
+ acpi_status st;
+
+ 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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+ (struct acpi_table_header **) &tbl);
+ if (ACPI_FAILURE(st)) {
+ dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
+ return 0;
+ }
+
+ return 0;
+
+ /* TPM 2.0 FIFO */
+ return 1;
+}
#else
+}
#else
static inline int is_itpm(struct acpi_device *dev)
{
return 0;
}
+
+static inline int is_fifo(struct acpi_device *dev)
+{
+ return 1;
+}
+ return 1;
+}
#endif
/* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -912,6 +946,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
/* Before we attempt to access the TPM we must see that the valid bit is set.
struct tpm_info tpm_info = tis_default_info;
int ret;
+ if (!is_fifo(acpi_dev))
+ return -ENODEV;
+
INIT_LIST_HEAD(&resources);
+
INIT_LIST_HEAD(&resources);
ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
&tpm_info);
@@ -937,6 +974,7 @@ static struct acpi_device_id tpm_acpi_tbl[] = {
{"BCM0102", 0}, /* Broadcom */
{"NSC1200", 0}, /* National */
{"ICO0102", 0}, /* Intel */
+ {"MSFT0101", 0}, /* TPM 2.0 */
/* Add new here */
{"NSC1200", 0}, /* National */
{"ICO0102", 0}, /* Intel */
+ {"MSFT0101", 0}, /* TPM 2.0 */
/* Add new here */
{"", 0}, /* User Specified */
{"", 0} /* Terminator */
--
{"", 0} /* Terminator */
2.5.0
Jarkko Sakkinen
Sep 29, 2015, 2:20:08 PM9/29/15
to
Both for FIFO and CRB interface TCG has decided to use the same HID
MSFT0101. They can be differentiated by looking at the start method
from TPM2 ACPI table. These patches make necessary fixes to tpm_tis
and tpm_crb modules in order to correctly detect, which module should
be used.
v2:
MSFT0101. They can be differentiated by looking at the start method
from TPM2 ACPI table. These patches make necessary fixes to tpm_tis
and tpm_crb modules in order to correctly detect, which module should
be used.
* One fixup was missing from v1: is_tpm2_fifo -> is_fifo
Jarkko Sakkinen (2):
tpm, tpm_tis: use acpi_driver instead of pnp_driver
tpm, tpm_tis: detect TPM2 FIFO devices based on HID
drivers/char/tpm/tpm.h | 7 ++
drivers/char/tpm/tpm_crb.c | 20 ++----
3 files changed, 124 insertions(+), 67 deletions(-)
Jarkko Sakkinen
Sep 29, 2015, 2:20:09 PM9/29/15
to
Migrate to struct acpi_driver in order to get out of 7 character
limitation for the HID.
limitation for the HID.
Reported-by: Matthew Garrett <mj...@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
Signed-off-by: Jarkko Sakkinen <jarkko....@linux.intel.com>
---
drivers/char/tpm/tpm_tis.c | 124 ++++++++++++++++++++++++++-------------------
1 file changed, 73 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
1 file changed, 73 insertions(+), 51 deletions(-)
index f2dffa7..87581b2 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
if (!strcmp("INTC0102", id->id))
return 1;
- }
return 0;
}
#else
-static inline int is_itpm(struct pnp_dev *dev)
- }
return 0;
}
#else
-static inline int is_itpm(struct pnp_dev *dev)
+static inline int is_itpm(struct acpi_device *dev)
+
+ return 1;
+}
+ return 1;
+}
-static struct pnp_device_id tpm_pnp_tbl[] = {
+static struct acpi_device_id tpm_acpi_tbl[] = {
{"PNP0C31", 0}, /* TPM */
{"ATM1200", 0}, /* Atmel */
{"IFX0102", 0}, /* Infineon */
@@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
+static struct acpi_device_id tpm_acpi_tbl[] = {
{"PNP0C31", 0}, /* TPM */
{"ATM1200", 0}, /* Atmel */
{"IFX0102", 0}, /* Infineon */
@@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
{"", 0}, /* User Specified */
{"", 0} /* Terminator */
{"", 0} /* Terminator */
struct tpm_chip *chip;
-#ifdef CONFIG_PNP
+#ifdef CONFIG_ACPI
-#ifdef CONFIG_PNP
+#ifdef CONFIG_ACPI
if (!force) {
- pnp_unregister_driver(&tis_pnp_driver);
+ acpi_bus_unregister_driver(&tis_acpi_driver);
return;
- pnp_unregister_driver(&tis_pnp_driver);
+ acpi_bus_unregister_driver(&tis_acpi_driver);
}
#endif
Jarkko Sakkinen
Sep 30, 2015, 2:00:17 AM9/30/15
to
On Tue, Sep 29, 2015 at 06:30:25PM +0100, Matthew Garrett wrote:
> On Tue, Sep 29, 2015 at 08:07:10PM +0300, Jarkko Sakkinen wrote:
> > Migrate to struct acpi_driver in order to get out of 7 character
> > limitation for the HID.
>
> Are we guaranteed that there are no old systems reporting these devices
> via pnpbios rather than acpi?
Right. I think it would make sense to be conservative and use
> On Tue, Sep 29, 2015 at 08:07:10PM +0300, Jarkko Sakkinen wrote:
> > Migrate to struct acpi_driver in order to get out of 7 character
> > limitation for the HID.
>
> Are we guaranteed that there are no old systems reporting these devices
> via pnpbios rather than acpi?
acpi_driver only for MSFT0101 because at least I don't know answer
to this question. It would be a risk to take these fixes to the stable
kernels othwerwise.
> --
> Matthew Garrett | mj...@srcf.ucam.org
/Jarkko
Jarkko Sakkinen
Sep 30, 2015, 2:00:18 AM9/30/15
to
On Tue, Sep 29, 2015 at 11:26:53AM -0600, Jason Gunthorpe wrote:
> On Tue, Sep 29, 2015 at 08:07:10PM +0300, Jarkko Sakkinen wrote:
>
> > -static struct pnp_device_id tpm_pnp_tbl[] = {
> > +static struct acpi_device_id tpm_acpi_tbl[] = {
> > {"PNP0C31", 0}, /* TPM */
> > {"ATM1200", 0}, /* Atmel */
> > {"IFX0102", 0}, /* Infineon */
> > @@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
> > {"", 0}, /* User Specified */
> > {"", 0} /* Terminator */
> > };
>
> Is this OK? I don't know alot about x86 PNP, but I thought the
> pnp_device_id scheme would work with ACPI and legacy PNPBIOS stuff,
> and changing to ACPI means ACPI only?
>
> If so, should we care? Is there a spec for non-ACPI TPM discovery we
> need to be following here?
I found at least all the IDs listed from drivers/acpi/acpi_pnp.c but you
> On Tue, Sep 29, 2015 at 08:07:10PM +0300, Jarkko Sakkinen wrote:
>
> > -static struct pnp_device_id tpm_pnp_tbl[] = {
> > +static struct acpi_device_id tpm_acpi_tbl[] = {
> > {"PNP0C31", 0}, /* TPM */
> > {"ATM1200", 0}, /* Atmel */
> > {"IFX0102", 0}, /* Infineon */
> > @@ -925,28 +941,34 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
> > {"", 0}, /* User Specified */
> > {"", 0} /* Terminator */
> > };
>
> Is this OK? I don't know alot about x86 PNP, but I thought the
> pnp_device_id scheme would work with ACPI and legacy PNPBIOS stuff,
> and changing to ACPI means ACPI only?
>
> If so, should we care? Is there a spec for non-ACPI TPM discovery we
> need to be following here?
might be right that they might be (don't know) with pnpbios.
Maybe a better solution would to have two tables and have only MSFT0101
in tpm_acpi_tbl in order to make sure that old functionality is not
broken up because we want this also to the stable kernels.
/Jarkko
Matthew Garrett
Sep 30, 2015, 2:10:14 AM9/30/15
to
On Wed, Sep 30, 2015 at 08:58:35AM +0300, Jarkko Sakkinen wrote:
> Right. I think it would make sense to be conservative and use
> acpi_driver only for MSFT0101 because at least I don't know answer
> to this question. It would be a risk to take these fixes to the stable
> kernels othwerwise.
In that case I'd suggest just adding it to the platform driver struct
> Right. I think it would make sense to be conservative and use
> acpi_driver only for MSFT0101 because at least I don't know answer
> to this question. It would be a risk to take these fixes to the stable
> kernels othwerwise.
and tidying up the init path a little (and having the platform driver
init code request resources rather than hardcoding them). It'll make it
easier to deal with any systems that declare TPMs in other tables such
as Device Tree later on.
--
Matthew Garrett | mj...@srcf.ucam.org
Matthew Garrett
Sep 30, 2015, 2:10:16 AM9/30/15
to
On Wed, Sep 30, 2015 at 08:56:12AM +0300, Jarkko Sakkinen wrote:
> I found at least all the IDs listed from drivers/acpi/acpi_pnp.c but you
> might be right that they might be (don't know) with pnpbios.
The entries in acpi_pnp.c mean "If this device is declared in an ACPI
> I found at least all the IDs listed from drivers/acpi/acpi_pnp.c but you
> might be right that they might be (don't know) with pnpbios.
table, create a PNP device". TPMs are kind of at the cutoff where some
vendors may have shipped non-ACPI systems with TPMs that were declared
via pnpbios, so there's a (small) risk that there are some people using
TPMs tied to PNP devices without a corresponding ACPI device.
> Maybe a better solution would to have two tables and have only MSFT0101
> in tpm_acpi_tbl in order to make sure that old functionality is not
> broken up because we want this also to the stable kernels.
--
Matthew Garrett | mj...@srcf.ucam.org
0 new messages