[PATCH v4 0/7] tpm_tis: Clean up force module parameter

[PATCH v4 0/7] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

Drive the force=1 flow through the driver core. There are two main reasons to do this:
 1) To enable tpm_tis for OF environments requires a platform_device anyhow, so
    the force_device needs to be re-used for them.
 2) Recent changes in the core code break the assumption that a driver will be
    'attached' to things created through platform_device_register_simple,
    which causes the tpm core to blow up.

To make force probing reliable this also fixes both tpm_tis and tpm_crb to
properly use request_region to lock the TPM iomemory against multiple access.

v4:
- Alter the commit message for using the common ACPI definitions (Jarkko)
- Move the misplaced error check hunk from patch #4 to #3 (Jarkko)

v3:
- Fix some bugs in getting the struct resource for tpm_tis (Martin Wilck)
- Include tpm_crb in the request_resource cleanup as well, tpm_tis and tpm_crb
  tend to use the same address ranges so both should have locking for safety
- ACPI and endianness cleanups in both drivers

v2:
 - Make sure we request the mem resource in tpm_tis to avoid double-loading
   the driver
 - Re-order the init sequence so that a forced platform device gets first crack at
   loading, and excludes the other mechanisms via the above
 - Checkpatch clean
 - Gotos renamed

Jason Gunthorpe (7):
  tpm_crb: Use the common ACPI definition of struct acpi_tpm2
  tpm_tis: Disable interrupt auto probing on a per-device basis
  tpm_tis: Do not fall back to a hardcoded address for TPM2
  tpm_tis: Use devm_ioremap_resource
  tpm_tis: Clean up the force=1 module parameter
  tpm_crb: Drop le32_to_cpu(ioread32(..))
  tpm_crb: Use devm_ioremap_resource

 drivers/char/tpm/tpm.h     |   7 --
 drivers/char/tpm/tpm_crb.c | 196 +++++++++++++++++++++-------------
 drivers/char/tpm/tpm_tis.c | 254 +++++++++++++++++++++++++--------------------
 3 files changed, 264 insertions(+), 193 deletions(-)

-- 
2.1.4

[PATCH v4 1/7] tpm_crb: Use the common ACPI definition of struct acpi_tpm2

[Jason Gunthorpe]

include/acpi/actbl2.h is the proper place for these definitions
and the needed TPM2 ones have been there since
commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")

This also drops a couple of le32_to_cpu's for members of this table,
the existing swapping was not done consistently, and the standard
used by other Linux callers of acpi_get_table is unswapped.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h     |  7 -------
 drivers/char/tpm/tpm_crb.c | 31 +++++++++----------------------
 drivers/char/tpm/tpm_tis.c |  2 +-
 3 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 542a80cbfd9c..28b477e8da6a 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -128,13 +128,6 @@ enum tpm2_startup_types {
 	TPM2_SU_STATE	= 0x0001,
 };
 
-enum tpm2_start_method {
-	TPM2_START_ACPI = 2,
-	TPM2_START_FIFO = 6,
-	TPM2_START_CRB = 7,
-	TPM2_START_CRB_WITH_ACPI = 8,
-};
-
 struct tpm_chip;
 
 struct tpm_vendor_specific {
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8342cf51ffdc..8dd70696ebe8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,14 +34,6 @@ enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
-struct acpi_tpm2 {
-	struct acpi_table_header hdr;
-	u16 platform_class;
-	u16 reserved;
-	u64 control_area_pa;
-	u32 start_method;
-} __packed;
-
 enum crb_ca_request {
 	CRB_CA_REQ_GO_IDLE	= BIT(0),
 	CRB_CA_REQ_CMD_READY	= BIT(1),
@@ -207,7 +199,7 @@ static const struct tpm_class_ops tpm_crb = {
 static int crb_acpi_add(struct acpi_device *device)
 {
 	struct tpm_chip *chip;
-	struct acpi_tpm2 *buf;
+	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
 	struct device *dev = &device->dev;
 	acpi_status status;
@@ -217,13 +209,14 @@ static int crb_acpi_add(struct acpi_device *device)
 
 	status = acpi_get_table(ACPI_SIG_TPM2, 1,
 				(struct acpi_table_header **) &buf);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev, "failed to get TPM2 ACPI table\n");
+	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
+		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
 		return -ENODEV;
 	}
 
 	/* Should the FIFO driver handle this? */
-	if (buf->start_method == TPM2_START_FIFO)
+	sm = buf->start_method;
+	if (sm == ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
 	chip = tpmm_chip_alloc(dev, &tpm_crb);
@@ -232,11 +225,6 @@ static int crb_acpi_add(struct acpi_device *device)
 
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
-		dev_err(dev, "TPM2 ACPI table has wrong size");
-		return -EINVAL;
-	}
-
 	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
 						GFP_KERNEL);
 	if (!priv) {
@@ -244,21 +232,20 @@ static int crb_acpi_add(struct acpi_device *device)
 		return -ENOMEM;
 	}
 
-	sm = le32_to_cpu(buf->start_method);
-
 	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
 	 * report only ACPI start but in practice seems to require both
 	 * ACPI start and CRB start.
 	 */
-	if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
+	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
 	    !strcmp(acpi_device_hid(device), "MSFT0101"))
 		priv->flags |= CRB_FL_CRB_START;
 
-	if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
+	if (sm == ACPI_TPM2_START_METHOD ||
+	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
 
 	priv->cca = (struct crb_control_area __iomem *)
-		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
+		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
 	if (!priv->cca) {
 		dev_err(dev, "ioremap of the control area failed\n");
 		return -ENOMEM;
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..304323bdcaaa 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -135,7 +135,7 @@ static inline int is_fifo(struct acpi_device *dev)
 		return 0;
 	}
 
-	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
 		return 0;
 
 	/* TPM 2.0 FIFO */
-- 
2.1.4

Re: [PATCH v4 1/7] tpm_crb: Use the common ACPI definition of struct acpi_tpm2

[Jarkko Sakkinen]

On Thu, Jan 07, 2016 at 05:36:20PM -0700, Jason Gunthorpe wrote:
> include/acpi/actbl2.h is the proper place for these definitions
> and the needed TPM2 ones have been there since
> commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> 
> This also drops a couple of le32_to_cpu's for members of this table,
> the existing swapping was not done consistently, and the standard
> used by other Linux callers of acpi_get_table is unswapped.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

PS. I'll update the commit message a bit. Thanks.

/Jarkko


> ---
>  drivers/char/tpm/tpm.h     |  7 -------
>  drivers/char/tpm/tpm_crb.c | 31 +++++++++----------------------
>  drivers/char/tpm/tpm_tis.c |  2 +-
>  3 files changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 542a80cbfd9c..28b477e8da6a 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -128,13 +128,6 @@ enum tpm2_startup_types {
>  	TPM2_SU_STATE	= 0x0001,
>  };
>  
> -enum tpm2_start_method {
> -	TPM2_START_ACPI = 2,
> -	TPM2_START_FIFO = 6,
> -	TPM2_START_CRB = 7,
> -	TPM2_START_CRB_WITH_ACPI = 8,
> -};
> -
>  struct tpm_chip;
>  
>  struct tpm_vendor_specific {
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 8342cf51ffdc..8dd70696ebe8 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,14 +34,6 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> -struct acpi_tpm2 {
> -	struct acpi_table_header hdr;
> -	u16 platform_class;
> -	u16 reserved;
> -	u64 control_area_pa;
> -	u32 start_method;
> -} __packed;
> -
>  enum crb_ca_request {
>  	CRB_CA_REQ_GO_IDLE	= BIT(0),
>  	CRB_CA_REQ_CMD_READY	= BIT(1),
> @@ -207,7 +199,7 @@ static const struct tpm_class_ops tpm_crb = {
>  static int crb_acpi_add(struct acpi_device *device)
>  {
>  	struct tpm_chip *chip;
> -	struct acpi_tpm2 *buf;
> +	struct acpi_table_tpm2 *buf;
>  	struct crb_priv *priv;
>  	struct device *dev = &device->dev;
>  	acpi_status status;
> @@ -217,13 +209,14 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(dev, "failed to get TPM2 ACPI table\n");
> +	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> +		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -ENODEV;
>  	}
>  
>  	/* Should the FIFO driver handle this? */
> -	if (buf->start_method == TPM2_START_FIFO)
> +	sm = buf->start_method;
> +	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	chip = tpmm_chip_alloc(dev, &tpm_crb);
> @@ -232,11 +225,6 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	chip->flags = TPM_CHIP_FLAG_TPM2;
>  
> -	if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
> -		dev_err(dev, "TPM2 ACPI table has wrong size");
> -		return -EINVAL;
> -	}
> -
>  	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
>  						GFP_KERNEL);
>  	if (!priv) {
> @@ -244,21 +232,20 @@ static int crb_acpi_add(struct acpi_device *device)
>  		return -ENOMEM;
>  	}
>  
> -	sm = le32_to_cpu(buf->start_method);
> -
>  	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
>  	 * report only ACPI start but in practice seems to require both
>  	 * ACPI start and CRB start.
>  	 */
> -	if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
> +	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
>  	    !strcmp(acpi_device_hid(device), "MSFT0101"))
>  		priv->flags |= CRB_FL_CRB_START;
>  
> -	if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
> +	if (sm == ACPI_TPM2_START_METHOD ||
> +	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
>  	priv->cca = (struct crb_control_area __iomem *)
> -		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
> +		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
>  	if (!priv->cca) {
>  		dev_err(dev, "ioremap of the control area failed\n");
>  		return -ENOMEM;
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 8a3509cb10da..304323bdcaaa 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -135,7 +135,7 @@ static inline int is_fifo(struct acpi_device *dev)
>  		return 0;
>  	}
>  
> -	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return 0;
>  
>  	/* TPM 2.0 FIFO */
> -- 
> 2.1.4
> 

[PATCH v4 2/7] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jason Gunthorpe]

Instead of clearing the global interrupts flag when any device
does not have an interrupt just pass -1 through tpm_info.irq.

The only thing that asks for autoprobing is the force=1 path.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 304323bdcaaa..fecd27b45fd1 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -69,7 +69,11 @@ enum tis_defaults {
 struct tpm_info {
 	unsigned long start;
 	unsigned long len;
-	unsigned int irq;
+	/* irq > 0 means: use irq $irq;
+	 * irq = 0 means: autoprobe for an irq;
+	 * irq = -1 means: no irq support
+	 */
+	int irq;
 };
 
 static struct tpm_info tis_default_info = {
@@ -807,7 +811,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&chip->vendor.read_queue);
 	init_waitqueue_head(&chip->vendor.int_queue);
-	if (interrupts) {
+	if (interrupts && tpm_info->irq != -1) {
 		if (tpm_info->irq) {
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
 						 tpm_info->irq);
@@ -895,9 +899,9 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
 
 #ifdef CONFIG_PNP
 static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
-				      const struct pnp_device_id *pnp_id)
+			    const struct pnp_device_id *pnp_id)
 {
-	struct tpm_info tpm_info = tis_default_info;
+	struct tpm_info tpm_info = {};
 	acpi_handle acpi_dev_handle = NULL;
 
 	tpm_info.start = pnp_mem_start(pnp_dev, 0);
@@ -906,7 +910,7 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 	if (pnp_irq_valid(pnp_dev, 0))
 		tpm_info.irq = pnp_irq(pnp_dev, 0);
 	else
-		interrupts = false;
+		tpm_info.irq = -1;
 
 #ifdef CONFIG_ACPI
 	if (pnp_acpi_device(pnp_dev)) {
@@ -984,6 +988,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&resources);
+	tpm_info.irq = -1;
 	ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
 				     &tpm_info);
 	if (ret < 0)
@@ -991,9 +996,6 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
-	if (!tpm_info.irq)
-		interrupts = false;
-
 	if (is_itpm(acpi_dev))
 		itpm = true;
 
-- 
2.1.4

[PATCH v4 3/7] tpm_tis: Do not fall back to a hardcoded address for TPM2

[Jason Gunthorpe]

If the ACPI tables do not declare a memory resource for the TPM2
then do not just fall back to the x86 default base address.

Also be stricter when checking the ancillary TPM2 ACPI data and error
out if any of this data is wrong rather than blindly assuming TPM1.

Fixes: 399235dc6e95 ("tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0")
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis.c | 48 +++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index fecd27b45fd1..b2b31f5418ca 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -122,39 +122,11 @@ static inline int is_itpm(struct acpi_device *dev)
 {
 	return has_hid(dev, "INTC0102");
 }
-
-static inline int is_fifo(struct acpi_device *dev)
-{
-	struct acpi_table_tpm2 *tbl;
-	acpi_status st;
-
-	/* TPM 1.2 FIFO */
-	if (!has_hid(dev, "MSFT0101"))
-		return 1;
-
-	st = acpi_get_table(ACPI_SIG_TPM2, 1,
-			    (struct acpi_table_header **) &tbl);
-	if (ACPI_FAILURE(st)) {
-		dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
-		return 0;
-	}
-
-	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
-		return 0;
-
-	/* TPM 2.0 FIFO */
-	return 1;
-}
 #else
 static inline int is_itpm(struct acpi_device *dev)
 {
 	return 0;
 }
-
-static inline int is_fifo(struct acpi_device *dev)
-{
-	return 1;
-}
 #endif
 
 /* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -980,11 +952,21 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
 
 static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 {
+	struct acpi_table_tpm2 *tbl;
+	acpi_status st;
 	struct list_head resources;
-	struct tpm_info tpm_info = tis_default_info;
+	struct tpm_info tpm_info = {};
 	int ret;
 
-	if (!is_fifo(acpi_dev))
+	st = acpi_get_table(ACPI_SIG_TPM2, 1,
+			    (struct acpi_table_header **) &tbl);
+	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
+		dev_err(&acpi_dev->dev,
+			FW_BUG "failed to get TPM2 ACPI table\n");
+		return -EINVAL;
+	}
+
+	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&resources);
@@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
+	if (tpm_info.start == 0 && tpm_info.len == 0) {
+		dev_err(&acpi_dev->dev,
+			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
+		return -EINVAL;
+	}
+
 	if (is_itpm(acpi_dev))
 		itpm = true;
 
-- 
2.1.4

Re: [PATCH v4 3/7] tpm_tis: Do not fall back to a hardcoded address for TPM2

[Jarkko Sakkinen]

On Thu, Jan 07, 2016 at 05:36:22PM -0700, Jason Gunthorpe wrote:
> If the ACPI tables do not declare a memory resource for the TPM2
> then do not just fall back to the x86 default base address.
> 
> Also be stricter when checking the ancillary TPM2 ACPI data and error
> out if any of this data is wrong rather than blindly assuming TPM1.
> 
> Fixes: 399235dc6e95 ("tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0")
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
> ---
>  drivers/char/tpm/tpm_tis.c | 48 +++++++++++++++++-----------------------------
>  1 file changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index fecd27b45fd1..b2b31f5418ca 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,39 +122,11 @@ static inline int is_itpm(struct acpi_device *dev)
>  {
>  	return has_hid(dev, "INTC0102");
>  }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> -	struct acpi_table_tpm2 *tbl;
> -	acpi_status st;
> -
> -	/* TPM 1.2 FIFO */
> -	if (!has_hid(dev, "MSFT0101"))
> -		return 1;
> -
> -	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> -			    (struct acpi_table_header **) &tbl);
> -	if (ACPI_FAILURE(st)) {
> -		dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
> -		return 0;
> -	}
> -
> -	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> -		return 0;
> -
> -	/* TPM 2.0 FIFO */
> -	return 1;
> -}
>  #else
>  static inline int is_itpm(struct acpi_device *dev)
>  {
>  	return 0;
>  }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> -	return 1;
> -}
>  #endif
>  
>  /* Before we attempt to access the TPM we must see that the valid bit is set.
> @@ -980,11 +952,21 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
>  
>  static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  {
> +	struct acpi_table_tpm2 *tbl;
> +	acpi_status st;
>  	struct list_head resources;
> -	struct tpm_info tpm_info = tis_default_info;
> +	struct tpm_info tpm_info = {};
>  	int ret;
>  
> -	if (!is_fifo(acpi_dev))
> +	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> +			    (struct acpi_table_header **) &tbl);
> +	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "failed to get TPM2 ACPI table\n");
> +		return -EINVAL;
> +	}
> +
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	INIT_LIST_HEAD(&resources);
> @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> +	if (tpm_info.start == 0 && tpm_info.len == 0) {
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> +		return -EINVAL;
> +	}
> +
>  	if (is_itpm(acpi_dev))
>  		itpm = true;
>  
> -- 
> 2.1.4
> 

[PATCH v4 4/7] tpm_tis: Use devm_ioremap_resource

[Jason Gunthorpe]

This does a request_resource under the covers which means tis holds a
lock on the memory range it is using so other drivers cannot grab it.
When doing probing it is important to ensure that other drivers are
not using the same range before tis starts touching it.

To do this flow the actual struct resource from the device right
through to devm_ioremap_resource. This ensures all the proper resource
meta-data is carried down.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index b2b31f5418ca..399c39e16a5c 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -67,8 +67,7 @@ enum tis_defaults {
 };
 
 struct tpm_info {
-	unsigned long start;
-	unsigned long len;
+	struct resource res;
 	/* irq > 0 means: use irq $irq;
 	 * irq = 0 means: autoprobe for an irq;
 	 * irq = -1 means: no irq support
@@ -77,8 +76,11 @@ struct tpm_info {
 };
 
 static struct tpm_info tis_default_info = {
-	.start = TIS_MEM_BASE,
-	.len = TIS_MEM_LEN,
+	.res = {
+		.start = TIS_MEM_BASE,
+		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
+		.flags = IORESOURCE_MEM,
+	},
 	.irq = 0,
 };
 
@@ -692,9 +694,9 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	chip->acpi_dev_handle = acpi_dev_handle;
 #endif
 
-	chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
-	if (!chip->vendor.iobase)
-		return -EIO;
+	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
+	if (IS_ERR(chip->vendor.iobase))
+		return PTR_ERR(chip->vendor.iobase);
 
 	/* Maximum timeouts */
 	chip->vendor.timeout_a = TIS_TIMEOUT_A_MAX;
@@ -875,9 +877,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 {
 	struct tpm_info tpm_info = {};
 	acpi_handle acpi_dev_handle = NULL;
+	struct resource *res;
 
-	tpm_info.start = pnp_mem_start(pnp_dev, 0);
-	tpm_info.len = pnp_mem_len(pnp_dev, 0);
+	res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+	tpm_info.res = *res;
 
 	if (pnp_irq_valid(pnp_dev, 0))
 		tpm_info.irq = pnp_irq(pnp_dev, 0);
@@ -940,12 +945,10 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
 	struct tpm_info *tpm_info = (struct tpm_info *) data;
 	struct resource res;
 
-	if (acpi_dev_resource_interrupt(ares, 0, &res)) {
+	if (acpi_dev_resource_interrupt(ares, 0, &res))
 		tpm_info->irq = res.start;
-	} else if (acpi_dev_resource_memory(ares, &res)) {
-		tpm_info->start = res.start;
-		tpm_info->len = resource_size(&res);
-	}
+	else if (acpi_dev_resource_memory(ares, &res))
+		tpm_info->res = res;
 
 	return 1;
 }
@@ -978,7 +981,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
-	if (tpm_info.start == 0 && tpm_info.len == 0) {
+	if (resource_type(&tpm_info.res) != IORESOURCE_MEM) {
 		dev_err(&acpi_dev->dev,
 			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
 		return -EINVAL;
-- 
2.1.4

[PATCH v4 5/7] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

The TPM core has long assumed that every device has a driver attached,
however the force path was attaching the TPM core outside of a driver
context. This isn't generally reliable as the user could detatch the
driver using sysfs or something, but commit b8b2c7d845d5 ("base/platform:
assert that dev_pm_domain callbacks are called unconditionally")
forced the issue by leaving the driver pointer NULL if there is
no probe.

Rework the TPM setup to create a platform device with resources and
then allow the driver core to naturally bind and probe it through the
normal mechanisms. All this structure is needed anyhow to enable TPM
for OF environments.

Finally, since the entire flow is changing convert the init/exit to use
the modern ifdef-less coding style when possible

Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_tis.c | 169 ++++++++++++++++++++++++++++-----------------
 1 file changed, 104 insertions(+), 65 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 399c39e16a5c..12aa96a69b6c 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -60,7 +60,6 @@ enum tis_int_flags {
 };
 
 enum tis_defaults {
-	TIS_MEM_BASE = 0xFED40000,
 	TIS_MEM_LEN = 0x5000,
 	TIS_SHORT_TIMEOUT = 750,	/* ms */
 	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
@@ -75,15 +74,6 @@ struct tpm_info {
 	int irq;
 };
 
-static struct tpm_info tis_default_info = {
-	.res = {
-		.start = TIS_MEM_BASE,
-		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
-		.flags = IORESOURCE_MEM,
-	},
-	.irq = 0,
-};
-
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 1.0 or TPM 2.0.
  */
@@ -825,7 +815,6 @@ out_err:
 	return rc;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 {
 	u32 intmask;
@@ -867,11 +856,9 @@ static int tpm_tis_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
 
-#ifdef CONFIG_PNP
 static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 			    const struct pnp_device_id *pnp_id)
 {
@@ -889,14 +876,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 	else
 		tpm_info.irq = -1;
 
-#ifdef CONFIG_ACPI
 	if (pnp_acpi_device(pnp_dev)) {
 		if (is_itpm(pnp_acpi_device(pnp_dev)))
 			itpm = true;
 
-		acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
+		acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
 	}
-#endif
 
 	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
 }
@@ -937,7 +922,6 @@ static struct pnp_driver tis_pnp_driver = {
 module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
 		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
 MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
-#endif
 
 #ifdef CONFIG_ACPI
 static int tpm_check_resource(struct acpi_resource *ares, void *data)
@@ -1024,80 +1008,135 @@ static struct acpi_driver tis_acpi_driver = {
 };
 #endif
 
+static struct platform_device *force_pdev;
+
+static int tpm_tis_plat_probe(struct platform_device *pdev)
+{
+	struct tpm_info tpm_info = {};
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		dev_err(&pdev->dev, "no memory resource defined\n");
+		return -ENODEV;
+	}
+	tpm_info.res = *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res) {
+		tpm_info.irq = res->start;
+	} else {
+		if (pdev == force_pdev)
+			tpm_info.irq = -1;
+		else
+			/* When forcing auto probe the IRQ */
+			tpm_info.irq = 0;
+	}
+
+	return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
+}
+
+static int tpm_tis_plat_remove(struct platform_device *pdev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
+
+	tpm_chip_unregister(chip);
+	tpm_tis_remove(chip);
+
+	return 0;
+}
+
 static struct platform_driver tis_drv = {
+	.probe = tpm_tis_plat_probe,
+	.remove = tpm_tis_plat_remove,
 	.driver = {
 		.name		= "tpm_tis",
 		.pm		= &tpm_tis_pm,
 	},
 };
 
-static struct platform_device *pdev;
-
 static bool force;
+#ifdef CONFIG_X86
 module_param(force, bool, 0444);
 MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
+#endif
+
+static int tpm_tis_force_device(void)
+{
+	struct platform_device *pdev;
+	static const struct resource x86_resources[] = {
+		{
+			.start = 0xFED40000,
+			.end = 0xFED40000 + TIS_MEM_LEN - 1,
+			.flags = IORESOURCE_MEM,
+		},
+	};
+
+	if (!force)
+		return 0;
+
+	/* The driver core will match the name tpm_tis of the device to
+	 * the tpm_tis platform driver and complete the setup via
+	 * tpm_tis_plat_probe
+	 */
+	pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
+					       ARRAY_SIZE(x86_resources));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+	force_pdev = pdev;
+
+	return 0;
+}
+
 static int __init init_tis(void)
 {
 	int rc;
-#ifdef CONFIG_PNP
-	if (!force) {
-		rc = pnp_register_driver(&tis_pnp_driver);
-		if (rc)
-			return rc;
-	}
-#endif
+
+	rc = tpm_tis_force_device();
+	if (rc)
+		goto err_force;
+
+	rc = platform_driver_register(&tis_drv);
+	if (rc)
+		goto err_platform;
+
 #ifdef CONFIG_ACPI
-	if (!force) {
-		rc = acpi_bus_register_driver(&tis_acpi_driver);
-		if (rc) {
-#ifdef CONFIG_PNP
-			pnp_unregister_driver(&tis_pnp_driver);
-#endif
-			return rc;
-		}
-	}
+	rc = acpi_bus_register_driver(&tis_acpi_driver);
+	if (rc)
+		goto err_acpi;
 #endif
-	if (!force)
-		return 0;
 
-	rc = platform_driver_register(&tis_drv);
-	if (rc < 0)
-		return rc;
-	pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
-	if (IS_ERR(pdev)) {
-		rc = PTR_ERR(pdev);
-		goto err_dev;
+	if (IS_ENABLED(CONFIG_PNP)) {
+		rc = pnp_register_driver(&tis_pnp_driver);
+		if (rc)
+			goto err_pnp;
 	}
-	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
-	if (rc)
-		goto err_init;
+
 	return 0;
-err_init:
-	platform_device_unregister(pdev);
-err_dev:
-	platform_driver_unregister(&tis_drv);
+
+err_pnp:
+#ifdef CONFIG_ACPI
+	acpi_bus_unregister_driver(&tis_acpi_driver);
+err_acpi:
+#endif
+	platform_device_unregister(force_pdev);
+err_platform:
+	if (force_pdev)
+		platform_device_unregister(force_pdev);
+err_force:
 	return rc;
 }
 
 static void __exit cleanup_tis(void)
 {
-	struct tpm_chip *chip;
-#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
-	if (!force) {
+	pnp_unregister_driver(&tis_pnp_driver);
 #ifdef CONFIG_ACPI
-		acpi_bus_unregister_driver(&tis_acpi_driver);
-#endif
-#ifdef CONFIG_PNP
-		pnp_unregister_driver(&tis_pnp_driver);
-#endif
-		return;
-	}
+	acpi_bus_unregister_driver(&tis_acpi_driver);
 #endif
-	chip = dev_get_drvdata(&pdev->dev);
-	tpm_chip_unregister(chip);
-	tpm_tis_remove(chip);
-	platform_device_unregister(pdev);
 	platform_driver_unregister(&tis_drv);
+
+	if (force_pdev)
+		platform_device_unregister(force_pdev);
 }
 
 module_init(init_tis);
-- 
2.1.4

Re: [PATCH v4 5/7] tpm_tis: Clean up the force=1 module parameter

[Jarkko Sakkinen]

On Thu, Jan 07, 2016 at 05:36:24PM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however the force path was attaching the TPM core outside of a driver
> context. This isn't generally reliable as the user could detatch the
> driver using sysfs or something, but commit b8b2c7d845d5 ("base/platform:
> assert that dev_pm_domain callbacks are called unconditionally")
> forced the issue by leaving the driver pointer NULL if there is
> no probe.
> 
> Rework the TPM setup to create a platform device with resources and
> then allow the driver core to naturally bind and probe it through the
> normal mechanisms. All this structure is needed anyhow to enable TPM
> for OF environments.
> 
> Finally, since the entire flow is changing convert the init/exit to use
> the modern ifdef-less coding style when possible
> 
> Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis.c | 169 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 104 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 399c39e16a5c..12aa96a69b6c 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -60,7 +60,6 @@ enum tis_int_flags {
>  };
>  
>  enum tis_defaults {
> -	TIS_MEM_BASE = 0xFED40000,
>  	TIS_MEM_LEN = 0x5000,
>  	TIS_SHORT_TIMEOUT = 750,	/* ms */
>  	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
> @@ -75,15 +74,6 @@ struct tpm_info {
>  	int irq;
>  };
>  
> -static struct tpm_info tis_default_info = {
> -	.res = {
> -		.start = TIS_MEM_BASE,
> -		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
> -		.flags = IORESOURCE_MEM,
> -	},
> -	.irq = 0,
> -};
> -
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
>   */
> @@ -825,7 +815,6 @@ out_err:
>  	return rc;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  {
>  	u32 intmask;
> @@ -867,11 +856,9 @@ static int tpm_tis_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>  
> -#ifdef CONFIG_PNP
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  			    const struct pnp_device_id *pnp_id)
>  {
> @@ -889,14 +876,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  	else
>  		tpm_info.irq = -1;
>  
> -#ifdef CONFIG_ACPI
>  	if (pnp_acpi_device(pnp_dev)) {
>  		if (is_itpm(pnp_acpi_device(pnp_dev)))
>  			itpm = true;
>  
> -		acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
> +		acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
>  	}
> -#endif
>  
>  	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
>  }
> @@ -937,7 +922,6 @@ static struct pnp_driver tis_pnp_driver = {
>  module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
>  MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
> -#endif
>  
>  #ifdef CONFIG_ACPI
>  static int tpm_check_resource(struct acpi_resource *ares, void *data)
> @@ -1024,80 +1008,135 @@ static struct acpi_driver tis_acpi_driver = {
>  };
>  #endif
>  
> +static struct platform_device *force_pdev;
> +
> +static int tpm_tis_plat_probe(struct platform_device *pdev)
> +{
> +	struct tpm_info tpm_info = {};
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL) {
> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		return -ENODEV;
> +	}
> +	tpm_info.res = *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (res) {
> +		tpm_info.irq = res->start;
> +	} else {
> +		if (pdev == force_pdev)
> +			tpm_info.irq = -1;
> +		else
> +			/* When forcing auto probe the IRQ */
> +			tpm_info.irq = 0;
> +	}
> +
> +	return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
> +}
> +
> +static int tpm_tis_plat_remove(struct platform_device *pdev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +
> +	return 0;
> +}
> +
>  static struct platform_driver tis_drv = {
> +	.probe = tpm_tis_plat_probe,
> +	.remove = tpm_tis_plat_remove,
>  	.driver = {
>  		.name		= "tpm_tis",
>  		.pm		= &tpm_tis_pm,
>  	},
>  };
>  
> -static struct platform_device *pdev;
> -
>  static bool force;
> +#ifdef CONFIG_X86
>  module_param(force, bool, 0444);
>  MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
> +#endif
> +
> +static int tpm_tis_force_device(void)
> +{
> +	struct platform_device *pdev;
> +	static const struct resource x86_resources[] = {
> +		{
> +			.start = 0xFED40000,
> +			.end = 0xFED40000 + TIS_MEM_LEN - 1,
> +			.flags = IORESOURCE_MEM,
> +		},
> +	};
> +
> +	if (!force)
> +		return 0;
> +
> +	/* The driver core will match the name tpm_tis of the device to
> +	 * the tpm_tis platform driver and complete the setup via
> +	 * tpm_tis_plat_probe
> +	 */
> +	pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
> +					       ARRAY_SIZE(x86_resources));
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +	force_pdev = pdev;
> +
> +	return 0;
> +}
> +
>  static int __init init_tis(void)
>  {
>  	int rc;
> -#ifdef CONFIG_PNP
> -	if (!force) {
> -		rc = pnp_register_driver(&tis_pnp_driver);
> -		if (rc)
> -			return rc;
> -	}
> -#endif
> +
> +	rc = tpm_tis_force_device();
> +	if (rc)
> +		goto err_force;
> +
> +	rc = platform_driver_register(&tis_drv);
> +	if (rc)
> +		goto err_platform;
> +
>  #ifdef CONFIG_ACPI
> -	if (!force) {
> -		rc = acpi_bus_register_driver(&tis_acpi_driver);
> -		if (rc) {
> -#ifdef CONFIG_PNP
> -			pnp_unregister_driver(&tis_pnp_driver);
> -#endif
> -			return rc;
> -		}
> -	}
> +	rc = acpi_bus_register_driver(&tis_acpi_driver);
> +	if (rc)
> +		goto err_acpi;
>  #endif
> -	if (!force)
> -		return 0;
>  
> -	rc = platform_driver_register(&tis_drv);
> -	if (rc < 0)
> -		return rc;
> -	pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
> -	if (IS_ERR(pdev)) {
> -		rc = PTR_ERR(pdev);
> -		goto err_dev;
> +	if (IS_ENABLED(CONFIG_PNP)) {
> +		rc = pnp_register_driver(&tis_pnp_driver);
> +		if (rc)
> +			goto err_pnp;
>  	}
> -	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
> -	if (rc)
> -		goto err_init;
> +
>  	return 0;
> -err_init:
> -	platform_device_unregister(pdev);
> -err_dev:
> -	platform_driver_unregister(&tis_drv);
> +
> +err_pnp:
> +#ifdef CONFIG_ACPI
> +	acpi_bus_unregister_driver(&tis_acpi_driver);
> +err_acpi:
> +#endif
> +	platform_device_unregister(force_pdev);
> +err_platform:
> +	if (force_pdev)
> +		platform_device_unregister(force_pdev);
> +err_force:
>  	return rc;
>  }
>  
>  static void __exit cleanup_tis(void)
>  {
> -	struct tpm_chip *chip;
> -#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
> -	if (!force) {
> +	pnp_unregister_driver(&tis_pnp_driver);
>  #ifdef CONFIG_ACPI
> -		acpi_bus_unregister_driver(&tis_acpi_driver);
> -#endif
> -#ifdef CONFIG_PNP
> -		pnp_unregister_driver(&tis_pnp_driver);
> -#endif
> -		return;
> -	}
> +	acpi_bus_unregister_driver(&tis_acpi_driver);
>  #endif
> -	chip = dev_get_drvdata(&pdev->dev);
> -	tpm_chip_unregister(chip);
> -	tpm_tis_remove(chip);
> -	platform_device_unregister(pdev);
>  	platform_driver_unregister(&tis_drv);
> +
> +	if (force_pdev)
> +		platform_device_unregister(force_pdev);
>  }
>  
>  module_init(init_tis);
> -- 
> 2.1.4
> 

[PATCH v4 6/7] tpm_crb: Drop le32_to_cpu(ioread32(..))

[Jason Gunthorpe]

ioread32 and readl are defined to read from PCI style memory, ie little
endian and return the result in host order. On platforms where a
swap is required ioread32/readl do the swap internally (eg see ppc).

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8dd70696ebe8..0237006dc4d8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -89,7 +89,7 @@ static u8 crb_status(struct tpm_chip *chip)
 	struct crb_priv *priv = chip->vendor.priv;
 	u8 sts = 0;
 
-	if ((le32_to_cpu(ioread32(&priv->cca->start)) & CRB_START_INVOKE) !=
+	if ((ioread32(&priv->cca->start) & CRB_START_INVOKE) !=
 	    CRB_START_INVOKE)
 		sts |= CRB_STS_COMPLETE;
 
@@ -105,7 +105,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	if (count < 6)
 		return -EIO;
 
-	if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
+	if (ioread32(&priv->cca->sts) & CRB_CA_STS_ERROR)
 		return -EIO;
 
 	memcpy_fromio(buf, priv->rsp, 6);
@@ -141,11 +141,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	struct crb_priv *priv = chip->vendor.priv;
 	int rc = 0;
 
-	if (len > le32_to_cpu(ioread32(&priv->cca->cmd_size))) {
+	if (len > ioread32(&priv->cca->cmd_size)) {
 		dev_err(&chip->dev,
 			"invalid command count value %x %zx\n",
 			(unsigned int) len,
-			(size_t) le32_to_cpu(ioread32(&priv->cca->cmd_size)));
+			(size_t) ioread32(&priv->cca->cmd_size));
 		return -E2BIG;
 	}
 
@@ -181,7 +181,7 @@ static void crb_cancel(struct tpm_chip *chip)
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	struct crb_priv *priv = chip->vendor.priv;
-	u32 cancel = le32_to_cpu(ioread32(&priv->cca->cancel));
+	u32 cancel = ioread32(&priv->cca->cancel);
 
 	return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
 }
@@ -251,10 +251,10 @@ static int crb_acpi_add(struct acpi_device *device)
 		return -ENOMEM;
 	}
 
-	pa = ((u64) le32_to_cpu(ioread32(&priv->cca->cmd_pa_high)) << 32) |
-		(u64) le32_to_cpu(ioread32(&priv->cca->cmd_pa_low));
-	priv->cmd = devm_ioremap_nocache(dev, pa,
-					 ioread32(&priv->cca->cmd_size));
+	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
+	     (u64)ioread32(&priv->cca->cmd_pa_low);
+	priv->cmd =
+	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
 	if (!priv->cmd) {
 		dev_err(dev, "ioremap of the command buffer failed\n");
 		return -ENOMEM;
@@ -262,8 +262,8 @@ static int crb_acpi_add(struct acpi_device *device)
 
 	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
 	pa = le64_to_cpu(pa);
-	priv->rsp = devm_ioremap_nocache(dev, pa,
-					 ioread32(&priv->cca->rsp_size));
+	priv->rsp =
+	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
 	if (!priv->rsp) {
 		dev_err(dev, "ioremap of the response buffer failed\n");
 		return -ENOMEM;
-- 
2.1.4

[PATCH v4 7/7] tpm_crb: Use devm_ioremap_resource

[Jason Gunthorpe]

To support the force mode in tpm_tis we need to use resource locking
in tpm_crb as well, via devm_ioremap_resource.

The light restructuring better aligns crb and tis and makes it easier
to see the that new changes make sense.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 157 +++++++++++++++++++++++++++++++--------------
 1 file changed, 108 insertions(+), 49 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 0237006dc4d8..1f844cc63016 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -77,6 +77,8 @@ enum crb_flags {
 
 struct crb_priv {
 	unsigned int flags;
+	struct resource res;
+	void __iomem *iobase;
 	struct crb_control_area __iomem *cca;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
@@ -196,22 +198,121 @@ static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_STS_COMPLETE,
 };
 
-static int crb_acpi_add(struct acpi_device *device)
+static int crb_init(struct acpi_device *device, struct crb_priv *priv)
 {
 	struct tpm_chip *chip;
+	int rc;
+
+	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	chip->vendor.priv = priv;
+	chip->acpi_dev_handle = device->handle;
+	chip->flags = TPM_CHIP_FLAG_TPM2;
+
+	rc = tpm_get_timeouts(chip);
+	if (rc)
+		return rc;
+
+	rc = tpm2_do_selftest(chip);
+	if (rc)
+		return rc;
+
+	return tpm_chip_register(chip);
+}
+
+static int crb_check_resource(struct acpi_resource *ares, void *data)
+{
+	struct crb_priv *priv = data;
+	struct resource res;
+
+	if (acpi_dev_resource_memory(ares, &res))
+		priv->res = res;
+
+	return 1;
+}
+
+static void __iomem *crb_access(struct device *dev, struct crb_priv *priv,
+				u64 start, u32 size)
+{
+	struct resource tmp = {};
+
+	tmp.start = start;
+	tmp.end = start + size - 1;
+	tmp.flags = IORESOURCE_MEM;
+
+	/* Detect a 64 bit address on a 32 bit system */
+	if (start != tmp.start)
+		return ERR_PTR(-EINVAL);
+
+	if (!resource_contains(&priv->res, &tmp)) {
+		dev_err(dev,
+			FW_BUG "TPM2 ACPI sub resource %pR is not in the device's region of %pR\n",
+			&tmp, &priv->res);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return priv->iobase + (tmp.start - priv->res.start);
+}
+
+static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
+		      struct acpi_table_tpm2 *buf)
+{
+	struct list_head resources;
+	struct device *dev = &device->dev;
+	u64 pa;
+	int ret;
+
+	INIT_LIST_HEAD(&resources);
+	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
+				     priv);
+	if (ret < 0)
+		return ret;
+	acpi_dev_free_resource_list(&resources);
+
+	if (resource_type(&priv->res) != IORESOURCE_MEM) {
+		dev_err(dev,
+			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
+		return -EINVAL;
+	}
+
+	priv->iobase = devm_ioremap_resource(dev, &priv->res);
+	if (IS_ERR(priv->iobase))
+		return PTR_ERR(priv->iobase);
+
+	priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);
+	if (IS_ERR(priv->cca))
+		return PTR_ERR(priv->cca);
+
+	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
+	     (u64)ioread32(&priv->cca->cmd_pa_low);
+	priv->cmd = crb_access(dev, priv, pa, ioread32(&priv->cca->cmd_size));
+	if (IS_ERR(priv->cmd))
+		return PTR_ERR(priv->cmd);
+
+	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
+	pa = le64_to_cpu(pa);
+	priv->rsp = crb_access(dev, priv, pa, ioread32(&priv->cca->rsp_size));
+	if (IS_ERR(priv->rsp))
+		return PTR_ERR(priv->rsp);
+	return 0;
+}
+
+static int crb_acpi_add(struct acpi_device *device)
+{
 	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
 	struct device *dev = &device->dev;
 	acpi_status status;
 	u32 sm;
-	u64 pa;
 	int rc;
 
 	status = acpi_get_table(ACPI_SIG_TPM2, 1,
 				(struct acpi_table_header **) &buf);
 	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
 		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	/* Should the FIFO driver handle this? */
@@ -219,18 +320,9 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (sm == ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
-	chip = tpmm_chip_alloc(dev, &tpm_crb);
-	if (IS_ERR(chip))
-		return PTR_ERR(chip);
-
-	chip->flags = TPM_CHIP_FLAG_TPM2;
-
-	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
-						GFP_KERNEL);
-	if (!priv) {
-		dev_err(dev, "failed to devm_kzalloc for private data\n");
+	priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
-	}
 
 	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
 	 * report only ACPI start but in practice seems to require both
@@ -244,44 +336,11 @@ static int crb_acpi_add(struct acpi_device *device)
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
 
-	priv->cca = (struct crb_control_area __iomem *)
-		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
-	if (!priv->cca) {
-		dev_err(dev, "ioremap of the control area failed\n");
-		return -ENOMEM;
-	}
-
-	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
-	     (u64)ioread32(&priv->cca->cmd_pa_low);
-	priv->cmd =
-	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
-	if (!priv->cmd) {
-		dev_err(dev, "ioremap of the command buffer failed\n");
-		return -ENOMEM;
-	}
-
-	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
-	pa = le64_to_cpu(pa);
-	priv->rsp =
-	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
-	if (!priv->rsp) {
-		dev_err(dev, "ioremap of the response buffer failed\n");
-		return -ENOMEM;
-	}
-
-	chip->vendor.priv = priv;
-
-	rc = tpm_get_timeouts(chip);
-	if (rc)
-		return rc;
-
-	chip->acpi_dev_handle = device->handle;
-
-	rc = tpm2_do_selftest(chip);
+	rc = crb_map_io(device, priv, buf);
 	if (rc)
 		return rc;
 
-	return tpm_chip_register(chip);
+	return crb_init(device, priv);
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
-- 
2.1.4

Re: [PATCH v4 7/7] tpm_crb: Use devm_ioremap_resource

[kbuild test robot]

Hi Jason,

[auto build test WARNING on next-20160107]
[cannot apply to char-misc/char-misc-testing v4.4-rc8 v4.4-rc7 v4.4-rc6 v4.4-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jason-Gunthorpe/tpm_tis-Clean-up-force-module-parameter/20160108-084227


coccinelle warnings: (new ones prefixed by >>)

>> drivers/char/tpm/tpm_crb.c:297:1-3: WARNING: PTR_ERR_OR_ZERO can be used

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] tpm_crb: fix ptr_ret.cocci warnings

[kbuild test robot]

drivers/char/tpm/tpm_crb.c:297:1-3: WARNING: PTR_ERR_OR_ZERO can be used


 Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Generated by: scripts/coccinelle/api/ptr_ret.cocci

CC: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 tpm_crb.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -294,9 +294,7 @@ static int crb_map_io(struct acpi_device
 	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
 	pa = le64_to_cpu(pa);
 	priv->rsp = crb_access(dev, priv, pa, ioread32(&priv->cca->rsp_size));
-	if (IS_ERR(priv->rsp))
-		return PTR_ERR(priv->rsp);
-	return 0;
+	return PTR_ERR_OR_ZERO(priv->rsp);
 }
 
 static int crb_acpi_add(struct acpi_device *device)

Re: [PATCH v4 7/7] tpm_crb: Use devm_ioremap_resource

[Jarkko Sakkinen]

On Thu, Jan 07, 2016 at 05:36:26PM -0700, Jason Gunthorpe wrote:
> To support the force mode in tpm_tis we need to use resource locking
> in tpm_crb as well, via devm_ioremap_resource.
> 
> The light restructuring better aligns crb and tis and makes it easier
> to see the that new changes make sense.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

PS. There's one cocci warning. I'll amend the fixup for that. Thanks for
the good work!

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 157 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 108 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 0237006dc4d8..1f844cc63016 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -77,6 +77,8 @@ enum crb_flags {
>  
>  struct crb_priv {
>  	unsigned int flags;
> +	struct resource res;
> +	void __iomem *iobase;
>  	struct crb_control_area __iomem *cca;
>  	u8 __iomem *cmd;
>  	u8 __iomem *rsp;
> @@ -196,22 +198,121 @@ static const struct tpm_class_ops tpm_crb = {
>  	.req_complete_val = CRB_STS_COMPLETE,
>  };
>  
> -static int crb_acpi_add(struct acpi_device *device)
> +static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>  {
>  	struct tpm_chip *chip;
> +	int rc;
> +
> +	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->vendor.priv = priv;
> +	chip->acpi_dev_handle = device->handle;
> +	chip->flags = TPM_CHIP_FLAG_TPM2;
> +
> +	rc = tpm_get_timeouts(chip);
> +	if (rc)
> +		return rc;
> +
> +	rc = tpm2_do_selftest(chip);
> +	if (rc)
> +		return rc;
> +
> +	return tpm_chip_register(chip);
> +}
> +
> +static int crb_check_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct crb_priv *priv = data;
> +	struct resource res;
> +
> +	if (acpi_dev_resource_memory(ares, &res))
> +		priv->res = res;
> +
> +	return 1;
> +}
> +
> +static void __iomem *crb_access(struct device *dev, struct crb_priv *priv,
> +				u64 start, u32 size)
> +{
> +	struct resource tmp = {};
> +
> +	tmp.start = start;
> +	tmp.end = start + size - 1;
> +	tmp.flags = IORESOURCE_MEM;
> +
> +	/* Detect a 64 bit address on a 32 bit system */
> +	if (start != tmp.start)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!resource_contains(&priv->res, &tmp)) {
> +		dev_err(dev,
> +			FW_BUG "TPM2 ACPI sub resource %pR is not in the device's region of %pR\n",
> +			&tmp, &priv->res);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return priv->iobase + (tmp.start - priv->res.start);
> +}
> +
> +static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> +		      struct acpi_table_tpm2 *buf)
> +{
> +	struct list_head resources;
> +	struct device *dev = &device->dev;
> +	u64 pa;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&resources);
> +	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> +				     priv);
> +	if (ret < 0)
> +		return ret;
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (resource_type(&priv->res) != IORESOURCE_MEM) {
> +		dev_err(dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->iobase = devm_ioremap_resource(dev, &priv->res);
> +	if (IS_ERR(priv->iobase))
> +		return PTR_ERR(priv->iobase);
> +
> +	priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);
> +	if (IS_ERR(priv->cca))
> +		return PTR_ERR(priv->cca);
> +
> +	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> +	     (u64)ioread32(&priv->cca->cmd_pa_low);
> +	priv->cmd = crb_access(dev, priv, pa, ioread32(&priv->cca->cmd_size));
> +	if (IS_ERR(priv->cmd))
> +		return PTR_ERR(priv->cmd);
> +
> +	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> +	pa = le64_to_cpu(pa);
> +	priv->rsp = crb_access(dev, priv, pa, ioread32(&priv->cca->rsp_size));
> +	if (IS_ERR(priv->rsp))
> +		return PTR_ERR(priv->rsp);
> +	return 0;
> +}
> +
> +static int crb_acpi_add(struct acpi_device *device)
> +{
>  	struct acpi_table_tpm2 *buf;
>  	struct crb_priv *priv;
>  	struct device *dev = &device->dev;
>  	acpi_status status;
>  	u32 sm;
> -	u64 pa;
>  	int rc;
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
>  	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> -		return -ENODEV;
> +		return -EINVAL;
>  	}
>  
>  	/* Should the FIFO driver handle this? */
> @@ -219,18 +320,9 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
> -	chip = tpmm_chip_alloc(dev, &tpm_crb);
> -	if (IS_ERR(chip))
> -		return PTR_ERR(chip);
> -
> -	chip->flags = TPM_CHIP_FLAG_TPM2;
> -
> -	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
> -						GFP_KERNEL);
> -	if (!priv) {
> -		dev_err(dev, "failed to devm_kzalloc for private data\n");
> +	priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
> -	}
>  
>  	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
>  	 * report only ACPI start but in practice seems to require both
> @@ -244,44 +336,11 @@ static int crb_acpi_add(struct acpi_device *device)
>  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
> -	priv->cca = (struct crb_control_area __iomem *)
> -		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
> -	if (!priv->cca) {
> -		dev_err(dev, "ioremap of the control area failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> -	     (u64)ioread32(&priv->cca->cmd_pa_low);
> -	priv->cmd =
> -	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
> -	if (!priv->cmd) {
> -		dev_err(dev, "ioremap of the command buffer failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> -	pa = le64_to_cpu(pa);
> -	priv->rsp =
> -	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
> -	if (!priv->rsp) {
> -		dev_err(dev, "ioremap of the response buffer failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	chip->vendor.priv = priv;
> -
> -	rc = tpm_get_timeouts(chip);
> -	if (rc)
> -		return rc;
> -
> -	chip->acpi_dev_handle = device->handle;
> -
> -	rc = tpm2_do_selftest(chip);
> +	rc = crb_map_io(device, priv, buf);
>  	if (rc)
>  		return rc;
>  
> -	return tpm_chip_register(chip);
> +	return crb_init(device, priv);
>  }
>  
>  static int crb_acpi_remove(struct acpi_device *device)
> -- 
> 2.1.4
> 

Re: [PATCH v4 7/7] tpm_crb: Use devm_ioremap_resource

[Jarkko Sakkinen]

On Thu, Jan 07, 2016 at 05:36:26PM -0700, Jason Gunthorpe wrote:
> To support the force mode in tpm_tis we need to use resource locking
> in tpm_crb as well, via devm_ioremap_resource.
> 
> The light restructuring better aligns crb and tis and makes it easier
> to see the that new changes make sense.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko
> ---
>  drivers/char/tpm/tpm_crb.c | 157 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 108 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 0237006dc4d8..1f844cc63016 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -77,6 +77,8 @@ enum crb_flags {
>  
>  struct crb_priv {
>  	unsigned int flags;
> +	struct resource res;
> +	void __iomem *iobase;
>  	struct crb_control_area __iomem *cca;
>  	u8 __iomem *cmd;
>  	u8 __iomem *rsp;
> @@ -196,22 +198,121 @@ static const struct tpm_class_ops tpm_crb = {
>  	.req_complete_val = CRB_STS_COMPLETE,
>  };
>  
> -static int crb_acpi_add(struct acpi_device *device)
> +static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>  {
>  	struct tpm_chip *chip;
> +	int rc;
> +
> +	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->vendor.priv = priv;
> +	chip->acpi_dev_handle = device->handle;
> +	chip->flags = TPM_CHIP_FLAG_TPM2;
> +
> +	rc = tpm_get_timeouts(chip);
> +	if (rc)
> +		return rc;
> +
> +	rc = tpm2_do_selftest(chip);
> +	if (rc)
> +		return rc;
> +
> +	return tpm_chip_register(chip);
> +}
> +
> +static int crb_check_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct crb_priv *priv = data;
> +	struct resource res;
> +
> +	if (acpi_dev_resource_memory(ares, &res))
> +		priv->res = res;
> +
> +	return 1;
> +}
> +
> +static void __iomem *crb_access(struct device *dev, struct crb_priv *priv,
> +				u64 start, u32 size)
> +{
> +	struct resource tmp = {};
> +
> +	tmp.start = start;
> +	tmp.end = start + size - 1;
> +	tmp.flags = IORESOURCE_MEM;
> +
> +	/* Detect a 64 bit address on a 32 bit system */
> +	if (start != tmp.start)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!resource_contains(&priv->res, &tmp)) {
> +		dev_err(dev,
> +			FW_BUG "TPM2 ACPI sub resource %pR is not in the device's region of %pR\n",
> +			&tmp, &priv->res);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return priv->iobase + (tmp.start - priv->res.start);
> +}
> +
> +static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> +		      struct acpi_table_tpm2 *buf)
> +{
> +	struct list_head resources;
> +	struct device *dev = &device->dev;
> +	u64 pa;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&resources);
> +	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> +				     priv);
> +	if (ret < 0)
> +		return ret;
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (resource_type(&priv->res) != IORESOURCE_MEM) {
> +		dev_err(dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->iobase = devm_ioremap_resource(dev, &priv->res);
> +	if (IS_ERR(priv->iobase))
> +		return PTR_ERR(priv->iobase);
> +
> +	priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);
> +	if (IS_ERR(priv->cca))
> +		return PTR_ERR(priv->cca);
> +
> +	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> +	     (u64)ioread32(&priv->cca->cmd_pa_low);
> +	priv->cmd = crb_access(dev, priv, pa, ioread32(&priv->cca->cmd_size));
> +	if (IS_ERR(priv->cmd))
> +		return PTR_ERR(priv->cmd);
> +
> +	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> +	pa = le64_to_cpu(pa);
> +	priv->rsp = crb_access(dev, priv, pa, ioread32(&priv->cca->rsp_size));
> +	if (IS_ERR(priv->rsp))
> +		return PTR_ERR(priv->rsp);
> +	return 0;
> +}
> +
> +static int crb_acpi_add(struct acpi_device *device)
> +{
>  	struct acpi_table_tpm2 *buf;
>  	struct crb_priv *priv;
>  	struct device *dev = &device->dev;
>  	acpi_status status;
>  	u32 sm;
> -	u64 pa;
>  	int rc;
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
>  	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> -		return -ENODEV;
> +		return -EINVAL;
>  	}
>  
>  	/* Should the FIFO driver handle this? */
> @@ -219,18 +320,9 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
> -	chip = tpmm_chip_alloc(dev, &tpm_crb);
> -	if (IS_ERR(chip))
> -		return PTR_ERR(chip);
> -
> -	chip->flags = TPM_CHIP_FLAG_TPM2;
> -
> -	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
> -						GFP_KERNEL);
> -	if (!priv) {
> -		dev_err(dev, "failed to devm_kzalloc for private data\n");
> +	priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
> -	}
>  
>  	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
>  	 * report only ACPI start but in practice seems to require both
> @@ -244,44 +336,11 @@ static int crb_acpi_add(struct acpi_device *device)
>  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
> -	priv->cca = (struct crb_control_area __iomem *)
> -		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
> -	if (!priv->cca) {
> -		dev_err(dev, "ioremap of the control area failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> -	     (u64)ioread32(&priv->cca->cmd_pa_low);
> -	priv->cmd =
> -	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
> -	if (!priv->cmd) {
> -		dev_err(dev, "ioremap of the command buffer failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> -	pa = le64_to_cpu(pa);
> -	priv->rsp =
> -	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
> -	if (!priv->rsp) {
> -		dev_err(dev, "ioremap of the response buffer failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	chip->vendor.priv = priv;
> -
> -	rc = tpm_get_timeouts(chip);
> -	if (rc)
> -		return rc;
> -
> -	chip->acpi_dev_handle = device->handle;
> -
> -	rc = tpm2_do_selftest(chip);
> +	rc = crb_map_io(device, priv, buf);
>  	if (rc)
>  		return rc;
>  
> -	return tpm_chip_register(chip);
> +	return crb_init(device, priv);
>  }
>  
>  static int crb_acpi_remove(struct acpi_device *device)
> -- 
> 2.1.4
> 

Re: [PATCH v4 0/7] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Thu, Jan 07, 2016 at 05:36:19PM -0700, Jason Gunthorpe wrote:
> Drive the force=1 flow through the driver core. There are two main reasons to do this:
>  1) To enable tpm_tis for OF environments requires a platform_device anyhow, so
>     the force_device needs to be re-used for them.
>  2) Recent changes in the core code break the assumption that a driver will be
>     'attached' to things created through platform_device_register_simple,
>     which causes the tpm core to blow up.
> 
> To make force probing reliable this also fixes both tpm_tis and tpm_crb to
> properly use request_region to lock the TPM iomemory against multiple access.

Applied. Thank you.

/Jarkko

> 
> v4:
> - Alter the commit message for using the common ACPI definitions (Jarkko)
> - Move the misplaced error check hunk from patch #4 to #3 (Jarkko)
> 
> v3:
> - Fix some bugs in getting the struct resource for tpm_tis (Martin Wilck)
> - Include tpm_crb in the request_resource cleanup as well, tpm_tis and tpm_crb
>   tend to use the same address ranges so both should have locking for safety
> - ACPI and endianness cleanups in both drivers
> 
> v2:
>  - Make sure we request the mem resource in tpm_tis to avoid double-loading
>    the driver
>  - Re-order the init sequence so that a forced platform device gets first crack at
>    loading, and excludes the other mechanisms via the above
>  - Checkpatch clean
>  - Gotos renamed
> 
> Jason Gunthorpe (7):
>   tpm_crb: Use the common ACPI definition of struct acpi_tpm2
>   tpm_tis: Disable interrupt auto probing on a per-device basis
>   tpm_tis: Do not fall back to a hardcoded address for TPM2
>   tpm_tis: Use devm_ioremap_resource
>   tpm_tis: Clean up the force=1 module parameter
>   tpm_crb: Drop le32_to_cpu(ioread32(..))
>   tpm_crb: Use devm_ioremap_resource
> 
>  drivers/char/tpm/tpm.h     |   7 --
>  drivers/char/tpm/tpm_crb.c | 196 +++++++++++++++++++++-------------
>  drivers/char/tpm/tpm_tis.c | 254 +++++++++++++++++++++++++--------------------
>  3 files changed, 264 insertions(+), 193 deletions(-)
> 
> -- 
> 2.1.4
>