[PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

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. This patches makes necessary fixes to tpm_tis and
tpm_crb modules in order to correctly detect, which module should be
used.

For MSFT0101 we must use struct acpi_driver because struct pnp_driver
has a 7 character limitation.

It turned out that the root cause in b371616b8 was not correct for
https://bugzilla.kernel.org/show_bug.cgi?id=98181.

v2:

* One fixup was missing from v1: is_tpm2_fifo -> is_fifo

v3:

* Use pnp_driver for existing HIDs

Reported-by: Michael Saunders <mick.saunders@gmail.com>
Reported-by: Michael Marley <michael@michaelmarley.com>
Reported-by: Jethro Beekman <kernel@jbeekman.nl>
Reported-by: Matthew Garrett <mjg59@srcf.ucam.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h     |   7 ++
 drivers/char/tpm/tpm_crb.c |  20 ++---
 drivers/char/tpm/tpm_tis.c | 190 ++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 174 insertions(+), 43 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 f2dffa7..6efdda1 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 <leendert@watson.ibm.com>
@@ -28,6 +28,7 @@
 #include <linux/wait.h>
 #include <linux/acpi.h>
 #include <linux/freezer.h>
+#include <acpi/actbl2.h>
 #include "tpm.h"
 
 enum tis_access {
@@ -65,6 +66,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.
@@ -91,26 +103,54 @@ struct priv_data {
 };
 
 #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
-static int is_itpm(struct pnp_dev *dev)
+static int has_hid(struct acpi_device *dev, const char *hid)
 {
-	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) {
-		if (!strcmp("INTC0102", id->id))
+	list_for_each_entry(id, &dev->pnp.ids, list)
+		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)
+{
+	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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+		return 0;
+
+	/* TPM 2.0 FIFO */
+	return 1;
+}
 #else
-static inline int is_itpm(struct pnp_dev *dev)
+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.
@@ -600,9 +640,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 +661,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 +746,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 +
@@ -890,27 +929,27 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
 static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 				      const struct pnp_device_id *pnp_id)
 {
-	resource_size_t start, len;
-	unsigned int irq = 0;
+	struct tpm_info tpm_info = tis_default_info;
 	acpi_handle acpi_dev_handle = NULL;
 
-	start = pnp_mem_start(pnp_dev, 0);
-	len = pnp_mem_len(pnp_dev, 0);
+	tpm_info.start = pnp_mem_start(pnp_dev, 0);
+	tpm_info.len = pnp_mem_len(pnp_dev, 0);
 
 	if (pnp_irq_valid(pnp_dev, 0))
-		irq = pnp_irq(pnp_dev, 0);
+		tpm_info.irq = pnp_irq(pnp_dev, 0);
 	else
 		interrupts = false;
 
-	if (is_itpm(pnp_dev))
-		itpm = true;
-
 #ifdef CONFIG_ACPI
-	if (pnp_acpi_device(pnp_dev))
+	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;
+	}
 #endif
 
-	return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start, len, irq);
+	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
 }
 
 static struct pnp_device_id tpm_pnp_tbl[] = {
@@ -950,6 +989,79 @@ module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
 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)
+{
+	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;
+}
+
+static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
+{
+	struct list_head resources;
+	struct tpm_info tpm_info = tis_default_info;
+	int ret;
+
+	if (!is_fifo(acpi_dev))
+		return -ENODEV;
+
+	INIT_LIST_HEAD(&resources);
+	ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
+				     &tpm_info);
+	if (ret < 0)
+		return ret;
+
+	acpi_dev_free_resource_list(&resources);
+
+	if (!tpm_info.irq)
+		interrupts = false;
+
+	if (is_itpm(acpi_dev))
+		itpm = true;
+
+	return tpm_tis_init(&acpi_dev->dev, &tpm_info, acpi_dev->handle);
+}
+
+static int tpm_tis_acpi_remove(struct acpi_device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
+
+	tpm_chip_unregister(chip);
+	tpm_tis_remove(chip);
+
+	return 0;
+}
+
+static struct acpi_device_id tpm_acpi_tbl[] = {
+	{"MSFT0101", 0},	/* TPM 2.0 */
+	/* Add new here */
+	{"", 0},		/* User Specified */
+	{"", 0}			/* Terminator */
+};
+MODULE_DEVICE_TABLE(acpi, tpm_acpi_tbl);
+
+static struct acpi_driver tis_acpi_driver = {
+	.name = "tpm_tis",
+	.ids = tpm_acpi_tbl,
+	.ops = {
+		.add = tpm_tis_acpi_init,
+		.remove = tpm_tis_acpi_remove,
+	},
+	.drv = {
+		.pm = &tpm_tis_pm,
+	},
+};
+#endif
+
 static struct platform_driver tis_drv = {
 	.driver = {
 		.name		= "tpm_tis",
@@ -966,9 +1078,25 @@ static int __init init_tis(void)
 {
 	int rc;
 #ifdef CONFIG_PNP
-	if (!force)
-		return pnp_register_driver(&tis_pnp_driver);
+	if (!force) {
+		rc = pnp_register_driver(&tis_pnp_driver);
+		if (rc)
+			return rc;
+	}
+#endif
+#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;
+		}
+	}
+#endif
+	if (!force)
+		return 0;
 
 	rc = platform_driver_register(&tis_drv);
 	if (rc < 0)
@@ -978,7 +1106,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,6 +1120,12 @@ err_dev:
 static void __exit cleanup_tis(void)
 {
 	struct tpm_chip *chip;
+#ifdef CONFIG_ACPI
+	if (!force) {
+		acpi_bus_unregister_driver(&tis_acpi_driver);
+		return;
+	}
+#endif
 #ifdef CONFIG_PNP
 	if (!force) {
 		pnp_unregister_driver(&tis_pnp_driver);
-- 
2.5.0

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Wed, 2015-09-30 at 18:19 +0300, Jarkko Sakkinen wrote:
> 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. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.

This would require testing with TPM 1.2. I don't have a machine/env
right now for 1.2 (I'm going to setup one with old Dell laptop as 
soon as I have the time). Is there anyone who could verify this with
1.2?

/Jarkko

> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders <mick.saunders@gmail.com>
> Reported-by: Michael Marley <michael@michaelmarley.com>
> Reported-by: Jethro Beekman <kernel@jbeekman.nl>
> Reported-by: Matthew Garrett <mjg59@srcf.ucam.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm.h     |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190
> ++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 174 insertions(+), 43 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 f2dffa7..6efdda1 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 <leendert@watson.ibm.com>
> @@ -28,6 +28,7 @@
>  #include <linux/wait.h>
>  #include <linux/acpi.h>
>  #include <linux/freezer.h>
> +#include <acpi/actbl2.h>
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,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.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> -	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) {
> -		if (!strcmp("INTC0102", id->id))
> +	list_for_each_entry(id, &dev->pnp.ids, list)
> +		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)
> +{
> +	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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> +		return 0;
> +
> +	/* TPM 2.0 FIFO */
> +	return 1;
> +}
>  #else
> -static inline int is_itpm(struct pnp_dev *dev)
> +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.
> @@ -600,9 +640,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 +661,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 +746,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 +
> @@ -890,27 +929,27 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm,
> tpm_pm_suspend, tpm_tis_resume);
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  				      const struct pnp_device_id
> *pnp_id)
>  {
> -	resource_size_t start, len;
> -	unsigned int irq = 0;
> +	struct tpm_info tpm_info = tis_default_info;
>  	acpi_handle acpi_dev_handle = NULL;
>  
> -	start = pnp_mem_start(pnp_dev, 0);
> -	len = pnp_mem_len(pnp_dev, 0);
> +	tpm_info.start = pnp_mem_start(pnp_dev, 0);
> +	tpm_info.len = pnp_mem_len(pnp_dev, 0);
>  
>  	if (pnp_irq_valid(pnp_dev, 0))
> -		irq = pnp_irq(pnp_dev, 0);
> +		tpm_info.irq = pnp_irq(pnp_dev, 0);
>  	else
>  		interrupts = false;
>  
> -	if (is_itpm(pnp_dev))
> -		itpm = true;
> -
>  #ifdef CONFIG_ACPI
> -	if (pnp_acpi_device(pnp_dev))
> +	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;
> +	}
>  #endif
>  
> -	return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start,
> len, irq);
> +	return tpm_tis_init(&pnp_dev->dev, &tpm_info,
> acpi_dev_handle);
>  }
>  
>  static struct pnp_device_id tpm_pnp_tbl[] = {
> @@ -950,6 +989,79 @@ module_param_string(hid,
> tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  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)
> +{
> +	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;
> +}
> +
> +static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> +{
> +	struct list_head resources;
> +	struct tpm_info tpm_info = tis_default_info;
> +	int ret;
> +
> +	if (!is_fifo(acpi_dev))
> +		return -ENODEV;
> +
> +	INIT_LIST_HEAD(&resources);
> +	ret = acpi_dev_get_resources(acpi_dev, &resources,
> tpm_check_resource,
> +				     &tpm_info);
> +	if (ret < 0)
> +		return ret;
> +
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (!tpm_info.irq)
> +		interrupts = false;
> +
> +	if (is_itpm(acpi_dev))
> +		itpm = true;
> +
> +	return tpm_tis_init(&acpi_dev->dev, &tpm_info, acpi_dev
> ->handle);
> +}
> +
> +static int tpm_tis_acpi_remove(struct acpi_device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +
> +	return 0;
> +}
> +
> +static struct acpi_device_id tpm_acpi_tbl[] = {
> +	{"MSFT0101", 0},	/* TPM 2.0 */
> +	/* Add new here */
> +	{"", 0},		/* User Specified */
> +	{"", 0}			/* Terminator */
> +};
> +MODULE_DEVICE_TABLE(acpi, tpm_acpi_tbl);
> +
> +static struct acpi_driver tis_acpi_driver = {
> +	.name = "tpm_tis",
> +	.ids = tpm_acpi_tbl,
> +	.ops = {
> +		.add = tpm_tis_acpi_init,
> +		.remove = tpm_tis_acpi_remove,
> +	},
> +	.drv = {
> +		.pm = &tpm_tis_pm,
> +	},
> +};
> +#endif
> +
>  static struct platform_driver tis_drv = {
>  	.driver = {
>  		.name		= "tpm_tis",
> @@ -966,9 +1078,25 @@ static int __init init_tis(void)
>  {
>  	int rc;
>  #ifdef CONFIG_PNP
> -	if (!force)
> -		return pnp_register_driver(&tis_pnp_driver);
> +	if (!force) {
> +		rc = pnp_register_driver(&tis_pnp_driver);
> +		if (rc)
> +			return rc;
> +	}
> +#endif
> +#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;
> +		}
> +	}
> +#endif
> +	if (!force)
> +		return 0;
>  
>  	rc = platform_driver_register(&tis_drv);
>  	if (rc < 0)
> @@ -978,7 +1106,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,6 +1120,12 @@ err_dev:
>  static void __exit cleanup_tis(void)
>  {
>  	struct tpm_chip *chip;
> +#ifdef CONFIG_ACPI
> +	if (!force) {
> +		acpi_bus_unregister_driver(&tis_acpi_driver);
> +		return;
> +	}
> +#endif
>  #ifdef CONFIG_PNP
>  	if (!force) {
>  		pnp_unregister_driver(&tis_pnp_driver);

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

On Wed, Sep 30, 2015 at 06:19:28PM +0300, Jarkko Sakkinen wrote:
> 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. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.
> 
> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders <mick.saunders@gmail.com>
> Reported-by: Michael Marley <michael@michaelmarley.com>
> Reported-by: Jethro Beekman <kernel@jbeekman.nl>
> Reported-by: Matthew Garrett <mjg59@srcf.ucam.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Tested-by: Michael Marley <michael@michaelmarley.com> ?

/Jarkko

> ---
>  drivers/char/tpm/tpm.h     |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190 ++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 174 insertions(+), 43 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 f2dffa7..6efdda1 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 <leendert@watson.ibm.com>
> @@ -28,6 +28,7 @@
>  #include <linux/wait.h>
>  #include <linux/acpi.h>
>  #include <linux/freezer.h>
> +#include <acpi/actbl2.h>
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,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.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> -	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) {
> -		if (!strcmp("INTC0102", id->id))
> +	list_for_each_entry(id, &dev->pnp.ids, list)
> +		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)
> +{
> +	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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> +		return 0;
> +
> +	/* TPM 2.0 FIFO */
> +	return 1;
> +}
>  #else
> -static inline int is_itpm(struct pnp_dev *dev)
> +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.
> @@ -600,9 +640,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 +661,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 +746,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 +
> @@ -890,27 +929,27 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  				      const struct pnp_device_id *pnp_id)
>  {
> -	resource_size_t start, len;
> -	unsigned int irq = 0;
> +	struct tpm_info tpm_info = tis_default_info;
>  	acpi_handle acpi_dev_handle = NULL;
>  
> -	start = pnp_mem_start(pnp_dev, 0);
> -	len = pnp_mem_len(pnp_dev, 0);
> +	tpm_info.start = pnp_mem_start(pnp_dev, 0);
> +	tpm_info.len = pnp_mem_len(pnp_dev, 0);
>  
>  	if (pnp_irq_valid(pnp_dev, 0))
> -		irq = pnp_irq(pnp_dev, 0);
> +		tpm_info.irq = pnp_irq(pnp_dev, 0);
>  	else
>  		interrupts = false;
>  
> -	if (is_itpm(pnp_dev))
> -		itpm = true;
> -
>  #ifdef CONFIG_ACPI
> -	if (pnp_acpi_device(pnp_dev))
> +	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;
> +	}
>  #endif
>  
> -	return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start, len, irq);
> +	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
>  }
>  
>  static struct pnp_device_id tpm_pnp_tbl[] = {
> @@ -950,6 +989,79 @@ module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  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)
> +{
> +	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;
> +}
> +
> +static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> +{
> +	struct list_head resources;
> +	struct tpm_info tpm_info = tis_default_info;
> +	int ret;
> +
> +	if (!is_fifo(acpi_dev))
> +		return -ENODEV;
> +
> +	INIT_LIST_HEAD(&resources);
> +	ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
> +				     &tpm_info);
> +	if (ret < 0)
> +		return ret;
> +
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (!tpm_info.irq)
> +		interrupts = false;
> +
> +	if (is_itpm(acpi_dev))
> +		itpm = true;
> +
> +	return tpm_tis_init(&acpi_dev->dev, &tpm_info, acpi_dev->handle);
> +}
> +
> +static int tpm_tis_acpi_remove(struct acpi_device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +
> +	return 0;
> +}
> +
> +static struct acpi_device_id tpm_acpi_tbl[] = {
> +	{"MSFT0101", 0},	/* TPM 2.0 */
> +	/* Add new here */
> +	{"", 0},		/* User Specified */
> +	{"", 0}			/* Terminator */
> +};
> +MODULE_DEVICE_TABLE(acpi, tpm_acpi_tbl);
> +
> +static struct acpi_driver tis_acpi_driver = {
> +	.name = "tpm_tis",
> +	.ids = tpm_acpi_tbl,
> +	.ops = {
> +		.add = tpm_tis_acpi_init,
> +		.remove = tpm_tis_acpi_remove,
> +	},
> +	.drv = {
> +		.pm = &tpm_tis_pm,
> +	},
> +};
> +#endif
> +
>  static struct platform_driver tis_drv = {
>  	.driver = {
>  		.name		= "tpm_tis",
> @@ -966,9 +1078,25 @@ static int __init init_tis(void)
>  {
>  	int rc;
>  #ifdef CONFIG_PNP
> -	if (!force)
> -		return pnp_register_driver(&tis_pnp_driver);
> +	if (!force) {
> +		rc = pnp_register_driver(&tis_pnp_driver);
> +		if (rc)
> +			return rc;
> +	}
> +#endif
> +#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;
> +		}
> +	}
> +#endif
> +	if (!force)
> +		return 0;
>  
>  	rc = platform_driver_register(&tis_drv);
>  	if (rc < 0)
> @@ -978,7 +1106,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,6 +1120,12 @@ err_dev:
>  static void __exit cleanup_tis(void)
>  {
>  	struct tpm_chip *chip;
> +#ifdef CONFIG_ACPI
> +	if (!force) {
> +		acpi_bus_unregister_driver(&tis_acpi_driver);
> +		return;
> +	}
> +#endif
>  #ifdef CONFIG_PNP
>  	if (!force) {
>  		pnp_unregister_driver(&tis_pnp_driver);
> -- 
> 2.5.0
> 

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Jarkko Sakkinen]

Hi

This would need Tested-by's. I've tested it both PTT/fTPM based system
and dTPM (2.0) based system. Thank you.

/Jarkko

On Wed, Sep 30, 2015 at 06:19:28PM +0300, Jarkko Sakkinen wrote:
> 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. This patches makes necessary fixes to tpm_tis and
> tpm_crb modules in order to correctly detect, which module should be
> used.
> 
> For MSFT0101 we must use struct acpi_driver because struct pnp_driver
> has a 7 character limitation.
> 
> It turned out that the root cause in b371616b8 was not correct for
> https://bugzilla.kernel.org/show_bug.cgi?id=98181.
> 
> v2:
> 
> * One fixup was missing from v1: is_tpm2_fifo -> is_fifo
> 
> v3:
> 
> * Use pnp_driver for existing HIDs
> 
> Reported-by: Michael Saunders <mick.saunders@gmail.com>
> Reported-by: Michael Marley <michael@michaelmarley.com>
> Reported-by: Jethro Beekman <kernel@jbeekman.nl>
> Reported-by: Matthew Garrett <mjg59@srcf.ucam.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm.h     |   7 ++
>  drivers/char/tpm/tpm_crb.c |  20 ++---
>  drivers/char/tpm/tpm_tis.c | 190 ++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 174 insertions(+), 43 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 f2dffa7..6efdda1 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 <leendert@watson.ibm.com>
> @@ -28,6 +28,7 @@
>  #include <linux/wait.h>
>  #include <linux/acpi.h>
>  #include <linux/freezer.h>
> +#include <acpi/actbl2.h>
>  #include "tpm.h"
>  
>  enum tis_access {
> @@ -65,6 +66,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.
> @@ -91,26 +103,54 @@ struct priv_data {
>  };
>  
>  #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct acpi_device *dev, const char *hid)
>  {
> -	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) {
> -		if (!strcmp("INTC0102", id->id))
> +	list_for_each_entry(id, &dev->pnp.ids, list)
> +		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)
> +{
> +	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 (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> +		return 0;
> +
> +	/* TPM 2.0 FIFO */
> +	return 1;
> +}
>  #else
> -static inline int is_itpm(struct pnp_dev *dev)
> +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.
> @@ -600,9 +640,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 +661,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 +746,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 +
> @@ -890,27 +929,27 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  				      const struct pnp_device_id *pnp_id)
>  {
> -	resource_size_t start, len;
> -	unsigned int irq = 0;
> +	struct tpm_info tpm_info = tis_default_info;
>  	acpi_handle acpi_dev_handle = NULL;
>  
> -	start = pnp_mem_start(pnp_dev, 0);
> -	len = pnp_mem_len(pnp_dev, 0);
> +	tpm_info.start = pnp_mem_start(pnp_dev, 0);
> +	tpm_info.len = pnp_mem_len(pnp_dev, 0);
>  
>  	if (pnp_irq_valid(pnp_dev, 0))
> -		irq = pnp_irq(pnp_dev, 0);
> +		tpm_info.irq = pnp_irq(pnp_dev, 0);
>  	else
>  		interrupts = false;
>  
> -	if (is_itpm(pnp_dev))
> -		itpm = true;
> -
>  #ifdef CONFIG_ACPI
> -	if (pnp_acpi_device(pnp_dev))
> +	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;
> +	}
>  #endif
>  
> -	return tpm_tis_init(&pnp_dev->dev, acpi_dev_handle, start, len, irq);
> +	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
>  }
>  
>  static struct pnp_device_id tpm_pnp_tbl[] = {
> @@ -950,6 +989,79 @@ module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  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)
> +{
> +	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;
> +}
> +
> +static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> +{
> +	struct list_head resources;
> +	struct tpm_info tpm_info = tis_default_info;
> +	int ret;
> +
> +	if (!is_fifo(acpi_dev))
> +		return -ENODEV;
> +
> +	INIT_LIST_HEAD(&resources);
> +	ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
> +				     &tpm_info);
> +	if (ret < 0)
> +		return ret;
> +
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (!tpm_info.irq)
> +		interrupts = false;
> +
> +	if (is_itpm(acpi_dev))
> +		itpm = true;
> +
> +	return tpm_tis_init(&acpi_dev->dev, &tpm_info, acpi_dev->handle);
> +}
> +
> +static int tpm_tis_acpi_remove(struct acpi_device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(&dev->dev);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +
> +	return 0;
> +}
> +
> +static struct acpi_device_id tpm_acpi_tbl[] = {
> +	{"MSFT0101", 0},	/* TPM 2.0 */
> +	/* Add new here */
> +	{"", 0},		/* User Specified */
> +	{"", 0}			/* Terminator */
> +};
> +MODULE_DEVICE_TABLE(acpi, tpm_acpi_tbl);
> +
> +static struct acpi_driver tis_acpi_driver = {
> +	.name = "tpm_tis",
> +	.ids = tpm_acpi_tbl,
> +	.ops = {
> +		.add = tpm_tis_acpi_init,
> +		.remove = tpm_tis_acpi_remove,
> +	},
> +	.drv = {
> +		.pm = &tpm_tis_pm,
> +	},
> +};
> +#endif
> +
>  static struct platform_driver tis_drv = {
>  	.driver = {
>  		.name		= "tpm_tis",
> @@ -966,9 +1078,25 @@ static int __init init_tis(void)
>  {
>  	int rc;
>  #ifdef CONFIG_PNP
> -	if (!force)
> -		return pnp_register_driver(&tis_pnp_driver);
> +	if (!force) {
> +		rc = pnp_register_driver(&tis_pnp_driver);
> +		if (rc)
> +			return rc;
> +	}
> +#endif
> +#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;
> +		}
> +	}
> +#endif
> +	if (!force)
> +		return 0;
>  
>  	rc = platform_driver_register(&tis_drv);
>  	if (rc < 0)
> @@ -978,7 +1106,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,6 +1120,12 @@ err_dev:
>  static void __exit cleanup_tis(void)
>  {
>  	struct tpm_chip *chip;
> +#ifdef CONFIG_ACPI
> +	if (!force) {
> +		acpi_bus_unregister_driver(&tis_acpi_driver);
> +		return;
> +	}
> +#endif
>  #ifdef CONFIG_PNP
>  	if (!force) {
>  		pnp_unregister_driver(&tis_pnp_driver);
> -- 
> 2.5.0
> 

Re: [PATCH v3] tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

[Matthew Garrett]

On Thu, Oct 08, 2015 at 06:04:27PM +0300, Jarkko Sakkinen wrote:
> Hi
> 
> This would need Tested-by's. I've tested it both PTT/fTPM based system
> and dTPM (2.0) based system. Thank you.

I'm on the road at the moment, but I'll be able to test this next week.

-- 
Matthew Garrett | mjg59@srcf.ucam.org