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@xxxxxxxxx>
> Reported-by: Michael Marley <michael@xxxxxxxxxxxxxxxxx>
> Reported-by: Jethro Beekman <kernel@xxxxxxxxxxx>
> Reported-by: Matthew Garrett <mjg59@xxxxxxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxx>
> @@ -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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/