[PATCH v2 0/3] tpm_tis: Clean up force module parameter

[PATCH v2 0/3] 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 probe/release code needs to be re-used for that.
 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.

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

Martin, this should fix the double loading you noticed, please confirm.  There
is a possibility the force path needs a bit more code to be compatible with
devm_ioremap_resource, I'm not sure, hoping not.

Jason Gunthorpe (3):
  tpm_tis: Disable interrupt auto probing on a per-device basis
  tpm_tis: Use devm_ioremap_resource
  tpm_tis: Clean up the force=1 module parameter

 drivers/char/tpm/tpm_tis.c | 203 +++++++++++++++++++++++++++------------------
 1 file changed, 122 insertions(+), 81 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] 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>
---
 drivers/char/tpm/tpm_tis.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..0a2d94f3d679 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -69,7 +69,7 @@ enum tis_defaults {
 struct tpm_info {
 	unsigned long start;
 	unsigned long len;
-	unsigned int irq;
+	int irq;
 };
 
 static struct tpm_info tis_default_info = {
@@ -807,7 +807,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 +895,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 +906,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)) {
@@ -977,13 +977,14 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
 static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 {
 	struct list_head resources;
-	struct tpm_info tpm_info = tis_default_info;
+	struct tpm_info tpm_info = {};
 	int ret;
 
 	if (!is_fifo(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 +992,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

Re: [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a per-device basis

[Uwe Kleine-König]

Hello,

On Tue, Dec 01, 2015 at 11:58:27AM -0700, Jason Gunthorpe wrote:
> 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>
> ---
>  drivers/char/tpm/tpm_tis.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 8a3509cb10da..0a2d94f3d679 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -69,7 +69,7 @@ enum tis_defaults {
>  struct tpm_info {
>  	unsigned long start;
>  	unsigned long len;
> -	unsigned int irq;
> +	int irq;

I'd add a comment here about the possible values of irq and their
interpretation. Something like:

	/*
	 * irq > 0 means: use irq $irq;
	 * irq = 0 means: autoprobe for an irq;
	 * irq = -1 means: no irq support
	 */

>  };
>  
>  static struct tpm_info tis_default_info = {
> @@ -807,7 +807,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 +895,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 +906,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;

It's definitly a nice improvement of this patch that the init functions
don't change the module parameter any more. (I didn't check if all
changes are gone now, but at least it's two modifications less.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:19:18PM +0100, Uwe Kleine-König wrote:
> I'd add a comment here about the possible values of irq and their
> interpretation. Something like:

Done

> It's definitly a nice improvement of this patch that the init functions
> don't change the module parameter any more. (I didn't check if all
> changes are gone now, but at least it's two modifications less.)

I checked, this gets them all.

Jason

[PATCH v2 2/3] 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>
---
 drivers/char/tpm/tpm_tis.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 0a2d94f3d679..1032855c46b2 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -67,14 +67,16 @@ enum tis_defaults {
 };
 
 struct tpm_info {
-	unsigned long start;
-	unsigned long len;
+	struct resource res;
 	int irq;
 };
 
 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,
 };
 
@@ -716,7 +718,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	chip->acpi_dev_handle = acpi_dev_handle;
 #endif
 
-	chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
+	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
 	if (!chip->vendor.iobase)
 		return -EIO;
 
@@ -899,9 +901,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;
+	memcpy(&tpm_info.res, res, sizeof(*res));
 
 	if (pnp_irq_valid(pnp_dev, 0))
 		tpm_info.irq = pnp_irq(pnp_dev, 0);
@@ -964,12 +969,9 @@ 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);
-	}
+	acpi_dev_resource_memory(ares, &tpm_info->res);
 
 	return 1;
 }
@@ -992,6 +994,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
+	if (resource_size(&tpm_info.res) == 0)
+		return -ENODEV;
+
 	if (is_itpm(acpi_dev))
 		itpm = true;
 
-- 
2.1.4

Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

[Uwe Kleine-König]

Hello,

On Tue, Dec 01, 2015 at 11:58:28AM -0700, Jason Gunthorpe wrote:
> 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>
> ---
>  drivers/char/tpm/tpm_tis.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 0a2d94f3d679..1032855c46b2 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -67,14 +67,16 @@ enum tis_defaults {
>  };
>  
>  struct tpm_info {
> -	unsigned long start;
> -	unsigned long len;
> +	struct resource res;
>  	int irq;
>  };
>  
>  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,
>  };
>  
> @@ -716,7 +718,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	chip->acpi_dev_handle = acpi_dev_handle;
>  #endif
>  
> -	chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
> +	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
>  	if (!chip->vendor.iobase)
>  		return -EIO;
>  
> @@ -899,9 +901,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;
> +	memcpy(&tpm_info.res, res, sizeof(*res));

I think you can do

	tpm_info.res = res;

here, which IMHO reads nicer and maybe is even more efficient (I don't
know much about x86).

>  	if (pnp_irq_valid(pnp_dev, 0))
>  		tpm_info.irq = pnp_irq(pnp_dev, 0);
> @@ -964,12 +969,9 @@ 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);
> -	}
> +	acpi_dev_resource_memory(ares, &tpm_info->res);
>  
>  	return 1;
>  }
> @@ -992,6 +994,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> +	if (resource_size(&tpm_info.res) == 0)
> +		return -ENODEV;
> +

Does this result in an error message from the upper layers?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:22:40PM +0100, Uwe Kleine-König wrote:

> here, which IMHO reads nicer and maybe is even more efficient (I don't
> know much about x86).

Sure

> > +	if (resource_size(&tpm_info.res) == 0)
> > +		return -ENODEV;
> > +
> 
> Does this result in an error message from the upper layers?

I think so, yes. The probe will fail which causes the driver core to
report a message.

The scenario this triggers is if the acpi stuff doesn't have a mem
resource, which is a firmware bug, I think. It could get a dedicated
print if that is what you are thinking?

-	if (resource_size(&tpm_info.res) == 0)
+	if (tpm_info.res.flags == 0) {
+		dev_err(&pdev->dev, FW_BUG "no memory resource defined\n");
 		return -ENODEV;
+	}
 
 	if (is_itpm(acpi_dev))
 		itpm = true;

[resource_size is wrong as well since it will return 1 for
 0'd struct resource, sigh..]

Previously it would try to call devm_ioremap with start/len=0 as the
range which should also fails in broadly the same way. So this is just
moving the existing failure up.

Something was needed because the change to struct resource means the
new code would call devm_ioremap_resource with a 0'd resource struct,
which is not as safe as start/len=0 as before.

Jason

Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

[Uwe Kleine-König]

Hello Jason,

On Tue, Dec 01, 2015 at 12:44:19PM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 08:22:40PM +0100, Uwe Kleine-König wrote:
> > > +	if (resource_size(&tpm_info.res) == 0)
> > > +		return -ENODEV;
> > > +
> > 
> > Does this result in an error message from the upper layers?
> 
> I think so, yes. The probe will fail which causes the driver core to
> report a message.
> 
> The scenario this triggers is if the acpi stuff doesn't have a mem
> resource, which is a firmware bug, I think. It could get a dedicated
> print if that is what you are thinking?

The issue I saw is: There are three(?) ways the tpm could be bound. If
one of the succeeds, the other two are expected to fail. But in this
case an error message, that the tpm failed to be bound is at least
misleading.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:52:17PM +0100, Uwe Kleine-König wrote:

> The issue I saw is: There are three(?) ways the tpm could be bound. If
> one of the succeeds, the other two are expected to fail. But in this
> case an error message, that the tpm failed to be bound is at least
> misleading.

My expectation is that the platform will never have a device that can
be bound to more than one and/or the driver core will prevent it (ie
if a PNP and ACPI driver claim the same ID the core should bind the
ACPI device only, not bind the ACPI device then downgrade to PNP and
try to bind the PNP device)

This issue pre-exists this patch. All this patch is doing is forcing
the tpm_tis to fail to bind instead of potentially running two drivers
on the same iorange at once.

The only case where this might not be true is if the user specifies
force. In this case, if forcing and there is acpi/pnp tpm at the same
address, then there will be a message failing the acpi/pnp bind. I
feel that is OK because it does indicate the user has done something
very questionable. (there is little reason to use force if acpi
already has the tpm at the same address range)

Jason

[PATCH v2 3/3] 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 commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
callbacks are called unconditionally") breaks that assumption.

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>
---
 drivers/char/tpm/tpm_tis.c | 170 +++++++++++++++++++++++++++------------------
 1 file changed, 104 insertions(+), 66 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 1032855c46b2..19beaf57f2a9 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -60,8 +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 */
 };
@@ -71,15 +69,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.
  */
@@ -849,7 +838,6 @@ out_err:
 	return rc;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 {
 	u32 intmask;
@@ -891,11 +879,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)
 {
@@ -913,14 +899,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);
 }
@@ -961,7 +945,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)
@@ -1034,80 +1017,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;
+	}
+	memcpy(&tpm_info.res, res, sizeof(*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 force_device(void)
+{
+	struct platform_device *pdev;
+	static const struct resource x86_resources[] = {
+		{
+			.start = 0xFED40000,
+			.end = 0xFED44FFF,
+			.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 = 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 v2 3/3] tpm_tis: Clean up the force=1 module parameter

[Uwe Kleine-König]

Hello,

On Tue, Dec 01, 2015 at 11:58:29AM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> callbacks are called unconditionally") breaks that assumption.

you asked for an alternative wording here. What about:

	The TPM core has long assumed that every device has a driver
	attached, which is not valid. This was noticed with commit
	b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
	callbacks are called unconditionally") which made probing of the
	tpm_tis device fail by mistake and resulted in an oops later on.
	
?

> 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>

Best regards,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:33:58PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Dec 01, 2015 at 11:58:29AM -0700, Jason Gunthorpe wrote:
> > The TPM core has long assumed that every device has a driver attached,
> > however commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> > callbacks are called unconditionally") breaks that assumption.
> 
> you asked for an alternative wording here. What about:
> 
> 	The TPM core has long assumed that every device has a driver
> 	attached, which is not valid.

But it is valid, it is an invariant of the tpm core that a driver be
attached, and prior to 'b8b that has been satisfied.

>       This was noticed with commit
> 	b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> 	callbacks are called unconditionally") which made probing of the
> 	tpm_tis device fail by mistake and resulted in an oops later on.

The probe didn't fail, the 'b8b causes a NULL probe function to result
in no driver being attached.

How about:

 The TPM has for a long time required that every device it uses has an
 attached driver. In the force case the tpm_tis driver met this via
 platform_register_simple and a NULL probe function for the driver.
 However, commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
 callbacks are called unconditionally") causes NULL probe functions
 to no longer bind a driver.

Did we ever reach a conclusion if Martin's patch should go ahead?

Jason

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Tue, Dec 01, 2015 at 11:58:26AM -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 probe/release code needs to be re-used for that.
>  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.
> 
> 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
> 
> Martin, this should fix the double loading you noticed, please confirm.  There
> is a possibility the force path needs a bit more code to be compatible with
> devm_ioremap_resource, I'm not sure, hoping not.

Just wanted to quickly say that I'm good with your interrupt rework
patches. I did only one squash as you can see:

https://github.com/jsakkine/linux-tpmdd/commits/master

For the other changes your arguments how patches should be separated in
this rework made perfect sense after a few re-reads.

I can accept them once I've tested them but in order to test them we
have to get these patches reviewed first as soon as possible.

/Jarkko

> Jason Gunthorpe (3):
>   tpm_tis: Disable interrupt auto probing on a per-device basis
>   tpm_tis: Use devm_ioremap_resource
>   tpm_tis: Clean up the force=1 module parameter
> 
>  drivers/char/tpm/tpm_tis.c | 203 +++++++++++++++++++++++++++------------------
>  1 file changed, 122 insertions(+), 81 deletions(-)
> 
> -- 
> 2.1.4
> 

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Tue, Dec 01, 2015 at 11:58:26AM -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 probe/release code needs to be re-used for that.
>  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.
> 
> 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
> 
> Martin, this should fix the double loading you noticed, please confirm.  There
> is a possibility the force path needs a bit more code to be compatible with
> devm_ioremap_resource, I'm not sure, hoping not.
> 
> Jason Gunthorpe (3):
>   tpm_tis: Disable interrupt auto probing on a per-device basis
>   tpm_tis: Use devm_ioremap_resource
>   tpm_tis: Clean up the force=1 module parameter

I went through the patches and didn't see anything that would shock me
enough not to apply the patches in the current if they also work when
tested *but* are these release critical for Linux v4.4?

I got a bit confused about the discussion that was going on about "where
to fix the probe" crash whether or not both it should be fixed in both
places.

Could you possibly make these apply on top of security/next and
re-submit if needed?

/Jarkko

>  drivers/char/tpm/tpm_tis.c | 203 +++++++++++++++++++++++++++------------------
>  1 file changed, 122 insertions(+), 81 deletions(-)
> 
> -- 
> 2.1.4
> 

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:

> I went through the patches and didn't see anything that would shock me
> enough not to apply the patches in the current if they also work when
> tested *but* are these release critical for Linux v4.4?
> 
> I got a bit confused about the discussion that was going on about "where
> to fix the probe" crash whether or not both it should be fixed in both
> places.

I'm also confused by that..

It sounds like force=1 is broken in 4.4 right now - do we care? Should
we fix this by using Martin's patch?

These changes are complex enough they really shouldn't go into 4.4
unless absolutely necessary.

> Could you possibly make these apply on top of security/next and
> re-submit if needed?

It isn't trivial to reorder all 10 patches to do this, I'd like to
know we need to do this for sure first. Uwe?

Jason

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Peter Huewe]

Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
>On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
>
>> I went through the patches and didn't see anything that would shock
>me
>> enough not to apply the patches in the current if they also work when
>> tested *but* are these release critical for Linux v4.4?
>> 
>> I got a bit confused about the discussion that was going on about
>"where
>> to fix the probe" crash whether or not both it should be fixed in
>both
>> places.
>
>I'm also confused by that..
>
>It sounds like force=1 is broken in 4.4 right now - do we care? Should
>we fix this by using Martin's patch?

I'm not 100% sure if force=1 is broken in 4.3 as well, as I oops when I have my tpm_crb loaded and then call modprobe tpm_tis force=1
Peter
>
>These changes are complex enough they really shouldn't go into 4.4
>unless absolutely necessary.
>
>> Could you possibly make these apply on top of security/next and
>> re-submit if needed?
>
>It isn't trivial to reorder all 10 patches to do this, I'd like to
>know we need to do this for sure first. Uwe?
>
>Jason

-- 
Sent from my mobile

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Tue, Dec 01, 2015 at 05:15:14PM -0800, Peter Huewe wrote:
> 
> 
> Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
> >On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> >
> >> I went through the patches and didn't see anything that would shock
> >me
> >> enough not to apply the patches in the current if they also work when
> >> tested *but* are these release critical for Linux v4.4?
> >> 
> >> I got a bit confused about the discussion that was going on about
> >"where
> >> to fix the probe" crash whether or not both it should be fixed in
> >both
> >> places.
> >
> >I'm also confused by that..
> >
> >It sounds like force=1 is broken in 4.4 right now - do we care? Should
> >we fix this by using Martin's patch?
> 
> I'm not 100% sure if force=1 is broken in 4.3 as well, as I oops when
> I have my tpm_crb loaded and then call modprobe tpm_tis force=1
> Peter

It'd have to be a different regression because v4.3 does not contain the
change that breaks this in v4.4. You had a NUC with discrete TPM module,
am I remembering right?

/Jarkko

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Peter Huewe]

Am 2. Dezember 2015 00:14:23 PST, schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Tue, Dec 01, 2015 at 05:15:14PM -0800, Peter Huewe wrote:
>> 
>> 
>> Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe
><jgunthorpe@obsidianresearch.com>:
>> >On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
>> >
>> >> I went through the patches and didn't see anything that would
>shock
>> >me
>> >> enough not to apply the patches in the current if they also work
>when
>> >> tested *but* are these release critical for Linux v4.4?
>> >> 
>> >> I got a bit confused about the discussion that was going on about
>> >"where
>> >> to fix the probe" crash whether or not both it should be fixed in
>> >both
>> >> places.
>> >
>> >I'm also confused by that..
>> >
>> >It sounds like force=1 is broken in 4.4 right now - do we care?
>Should
>> >we fix this by using Martin's patch?
>> 
>> I'm not 100% sure if force=1 is broken in 4.3 as well, as I oops when
>> I have my tpm_crb loaded and then call modprobe tpm_tis force=1
>> Peter
>
>It'd have to be a different regression because v4.3 does not contain
>the
>change that breaks this in v4.4. You had a NUC with discrete TPM
>module,
>am I remembering right?
>
Nope, intel fw tpm2.0 in my acer laptop.
No nucs here
>/Jarkko

-- 
Sent from my mobile

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Tue, Dec 01, 2015 at 03:22:23PM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> 
> > I went through the patches and didn't see anything that would shock me
> > enough not to apply the patches in the current if they also work when
> > tested *but* are these release critical for Linux v4.4?
> > 
> > I got a bit confused about the discussion that was going on about "where
> > to fix the probe" crash whether or not both it should be fixed in both
> > places.
> 
> I'm also confused by that..
> 
> It sounds like force=1 is broken in 4.4 right now - do we care? Should
> we fix this by using Martin's patch?
> 
> These changes are complex enough they really shouldn't go into 4.4
> unless absolutely necessary.

The reasons I'm asking this are:

* I'm planning to do v4.5 pull request soon.
* If this need to be get this into v4.4, we should act fast. Given the
  complexity of the changes I'd not recommend that unless it is a life
  and death question.

> > Could you possibly make these apply on top of security/next and
> > re-submit if needed?
> 
> It isn't trivial to reorder all 10 patches to do this, I'd like to
> know we need to do this for sure first. Uwe?

Agreed. First we have to know whether these changes have go to v4.4.

> Jason

/Jarkko

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Uwe Kleine-König]

Hello,

Cc += gregkh

On Wed, Dec 02, 2015 at 10:11:14AM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 01, 2015 at 03:22:23PM -0700, Jason Gunthorpe wrote:
> > On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> > 
> > > I went through the patches and didn't see anything that would shock me
> > > enough not to apply the patches in the current if they also work when
> > > tested *but* are these release critical for Linux v4.4?
> > > 
> > > I got a bit confused about the discussion that was going on about "where
> > > to fix the probe" crash whether or not both it should be fixed in both
> > > places.
> > 
> > I'm also confused by that..
> > 
> > It sounds like force=1 is broken in 4.4 right now - do we care? Should
> > we fix this by using Martin's patch?
> > 
> > These changes are complex enough they really shouldn't go into 4.4
> > unless absolutely necessary.
> 
> The reasons I'm asking this are:
> 
> * I'm planning to do v4.5 pull request soon.
> * If this need to be get this into v4.4, we should act fast. Given the
>   complexity of the changes I'd not recommend that unless it is a life
>   and death question.

I'd say we should repair b8b2c7d845d5 ("base/platform: assert that
dev_pm_domain callbacks are called unconditionally") for 4.4-rc$next and
live with the problem that the tpm driver had since long another
release.

The fix is already available, just some minor nitpicking regarding the
commit log has still to be resolved.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Greg Kroah-Hartman]

On Wed, Dec 02, 2015 at 09:21:47AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> Cc += gregkh
> 
> On Wed, Dec 02, 2015 at 10:11:14AM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 01, 2015 at 03:22:23PM -0700, Jason Gunthorpe wrote:
> > > On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > I went through the patches and didn't see anything that would shock me
> > > > enough not to apply the patches in the current if they also work when
> > > > tested *but* are these release critical for Linux v4.4?
> > > > 
> > > > I got a bit confused about the discussion that was going on about "where
> > > > to fix the probe" crash whether or not both it should be fixed in both
> > > > places.
> > > 
> > > I'm also confused by that..
> > > 
> > > It sounds like force=1 is broken in 4.4 right now - do we care? Should
> > > we fix this by using Martin's patch?
> > > 
> > > These changes are complex enough they really shouldn't go into 4.4
> > > unless absolutely necessary.
> > 
> > The reasons I'm asking this are:
> > 
> > * I'm planning to do v4.5 pull request soon.
> > * If this need to be get this into v4.4, we should act fast. Given the
> >   complexity of the changes I'd not recommend that unless it is a life
> >   and death question.
> 
> I'd say we should repair b8b2c7d845d5 ("base/platform: assert that
> dev_pm_domain callbacks are called unconditionally") for 4.4-rc$next and
> live with the problem that the tpm driver had since long another
> release.

I was going to queue up
	Subject: [PATCH] base/platform: fix panic when probe function is NULL

for 4.4-final, unless you all object to that.

thanks,

greg k-h

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Uwe Kleine-König]

Hello Greg,

On Wed, Dec 02, 2015 at 08:53:38AM -0800, Greg Kroah-Hartman wrote:
> On Wed, Dec 02, 2015 at 09:21:47AM +0100, Uwe Kleine-König wrote:
> > > > These changes are complex enough they really shouldn't go into 4.4
> > > > unless absolutely necessary.
> > > 
> > > The reasons I'm asking this are:
> > > 
> > > * I'm planning to do v4.5 pull request soon.
> > > * If this need to be get this into v4.4, we should act fast. Given the
> > >   complexity of the changes I'd not recommend that unless it is a life
> > >   and death question.
> > 
> > I'd say we should repair b8b2c7d845d5 ("base/platform: assert that
> > dev_pm_domain callbacks are called unconditionally") for 4.4-rc$next and
> > live with the problem that the tpm driver had since long another
> > release.
> 
> I was going to queue up
> 	Subject: [PATCH] base/platform: fix panic when probe function is NULL
> 
> for 4.4-final, unless you all object to that.

Martin is about to send a v3 of this patch, please pick up this v3
instead.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v3] base/platform: fix binding for drivers without probe callback

[martin.wilck]

From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

Since b8b2c7d845d5, platform_drv_probe() is called for all platform
devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
platform_drv_probe() will return the error code from dev_pm_domain_attach().

This causes real_probe() to enter the "probe_failed" path and set
dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
success if both dev->bus->probe and drv->probe were missing. As a result,
a device and driver could be "bound" together just by matching their names;
this doesn't work any more after b8b2c7d845d5.

This change broke the assumptions of certain drivers; for example, the TPM
code has long assumed that platform driver and device with matching name
could be bound in this way. That assumption may cause such drivers to
fail with Oops during initialization after applying this change. Failure
in suspend/resume tests under qemu has also been reported.

This patch restores the previous (4.3.0 and earlier) behavior of
platform_drv_probe() in the case when the associated platform driver has
no "probe" function.

Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally")
Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
---
 v2: fixed style issues, rephrased commit message.
 v3: rephrased commit message and subject again.

 drivers/base/platform.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1dd6d3b..176b59f 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -513,10 +513,15 @@ static int platform_drv_probe(struct device *_dev)
 		return ret;
 
 	ret = dev_pm_domain_attach(_dev, true);
-	if (ret != -EPROBE_DEFER && drv->probe) {
-		ret = drv->probe(dev);
-		if (ret)
-			dev_pm_domain_detach(_dev, true);
+	if (ret != -EPROBE_DEFER) {
+		if (drv->probe) {
+			ret = drv->probe(dev);
+			if (ret)
+				dev_pm_domain_detach(_dev, true);
+		} else {
+			/* don't fail if just dev_pm_domain_attach failed */
+			ret = 0;
+		}
 	}
 
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
-- 
1.8.3.1

Re: [PATCH v3] base/platform: fix binding for drivers without probe callback

[Uwe Kleine-König]

Hello Martin,

On Thu, Dec 03, 2015 at 09:51:44AM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> platform_drv_probe() will return the error code from dev_pm_domain_attach().
> 
> This causes real_probe() to enter the "probe_failed" path and set
> dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> success if both dev->bus->probe and drv->probe were missing. As a result,
> a device and driver could be "bound" together just by matching their names;
> this doesn't work any more after b8b2c7d845d5.
> 
> This change broke the assumptions of certain drivers; for example, the TPM
> code has long assumed that platform driver and device with matching name
> could be bound in this way. That assumption may cause such drivers to
> fail with Oops during initialization after applying this change. Failure
> in suspend/resume tests under qemu has also been reported.
> 
> This patch restores the previous (4.3.0 and earlier) behavior of
> platform_drv_probe() in the case when the associated platform driver has
> no "probe" function.
> 
> Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally")
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [tpmdd-devel] [PATCH v3] base/platform: fix binding for drivers without probe callback

[Jarkko Sakkinen]

On Thu, Dec 03, 2015 at 09:51:44AM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> platform_drv_probe() will return the error code from dev_pm_domain_attach().
> 
> This causes real_probe() to enter the "probe_failed" path and set
> dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> success if both dev->bus->probe and drv->probe were missing. As a result,
> a device and driver could be "bound" together just by matching their names;
> this doesn't work any more after b8b2c7d845d5.
> 
> This change broke the assumptions of certain drivers; for example, the TPM
> code has long assumed that platform driver and device with matching name
> could be bound in this way. That assumption may cause such drivers to
> fail with Oops during initialization after applying this change. Failure
> in suspend/resume tests under qemu has also been reported.
> 
> This patch restores the previous (4.3.0 and earlier) behavior of
> platform_drv_probe() in the case when the associated platform driver has
> no "probe" function.
> 
> Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally")
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> ---
>  v2: fixed style issues, rephrased commit message.
>  v3: rephrased commit message and subject again.
> 
>  drivers/base/platform.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1dd6d3b..176b59f 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -513,10 +513,15 @@ static int platform_drv_probe(struct device *_dev)
>  		return ret;
>  
>  	ret = dev_pm_domain_attach(_dev, true);
> -	if (ret != -EPROBE_DEFER && drv->probe) {
> -		ret = drv->probe(dev);
> -		if (ret)
> -			dev_pm_domain_detach(_dev, true);
> +	if (ret != -EPROBE_DEFER) {
> +		if (drv->probe) {
> +			ret = drv->probe(dev);
> +			if (ret)
> +				dev_pm_domain_detach(_dev, true);
> +		} else {
> +			/* don't fail if just dev_pm_domain_attach failed */
> +			ret = 0;
> +		}
>  	}
>  
>  	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> -- 
> 1.8.3.1

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

/Jarkko

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 01, 2015 at 11:58:26AM -0700, Jason Gunthorpe wrote:

> I went through the patches and didn't see anything that would shock me
> enough not to apply the patches in the current if they also work when
> tested *but* are these release critical for Linux v4.4?

Jarkko,

Can you explain how

commit 399235dc6e95400a1322a9999e92073bc572f0c8
Author: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date:   Tue Sep 29 00:32:19 2015 +0300

    tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0

Is supposed to work? I get the jist of the idea, but I'm not seeing
how it can work reliably..

The idea is to pass off TPM2_START_FIFO to tpm_tis?

I'm guessing that if the driver probe order is tpm_crb,tpm_tis then
things work because tpm_crb will claim the device first? Otherwise
tpm_tis claims these things unconditionally? If the probe order is
reversed things become broken?

What is the address tpm_tis should be using? I see two things, it
either uses the x86 default address or it expects the ACPI to have a
MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
I removed that code in this series. Perhaps tpm_tis should be using
control_area_pa ? Will ACPI ever present a struct resource? (if yes,
why isn't tpm_crb using one?)

There is also something wrong with the endianness in the acpi
stuff. I don't see endianness conversions in other acpi places, so I
wonder if the ones in tpm_crb are correct. If they are correct then
the struct needs le/be notations and there are some missing
conversions.

Jason

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Wed, Dec 02, 2015 at 11:27:27AM -0700, Jason Gunthorpe wrote:

> I'm guessing that if the driver probe order is tpm_crb,tpm_tis then
> things work because tpm_crb will claim the device first? Otherwise
> tpm_tis claims these things unconditionally? If the probe order is
> reversed things become broken?

Okay, I didn't find the is_fifo before, so that make sense

But this:

> What is the address tpm_tis should be using? I see two things, it
> either uses the x86 default address or it expects the ACPI to have a
> MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> I removed that code in this series. Perhaps tpm_tis should be using
> control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> why isn't tpm_crb using one?)

Is then still a problem. On Martin's system the MSFT0101 device does
not have a struct resource attached to it. Does any system, or is this
just dead code?

Should the control_area_pa be used?

Martin: could you try this (along with the other hunk to prevent the
oops):

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..6824a00ba513 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -138,6 +138,8 @@ static inline int is_fifo(struct acpi_device *dev)
 	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
 		return 0;
 
+	dev_err(&dev->dev, "control area pa is %x\n", tbl->control_area_pa);
+
 	/* TPM 2.0 FIFO */
 	return 1;
 }

Hoping to see it print 0xFED40000

> There is also something wrong with the endianness in the acpi
> stuff. I don't see endianness conversions in other acpi places, so I
> wonder if the ones in tpm_crb are correct. If they are correct then
> the struct needs le/be notations and there are some missing
> conversions.

I've made a patch to take care of this and move every thing to the
include/acpi/actbl2.h definitions, which is why I didn't notice
is_fifo in the first place...

Jason

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Wed, Dec 02, 2015 at 12:11:55PM -0700, Jason Gunthorpe wrote:
> On Wed, Dec 02, 2015 at 11:27:27AM -0700, Jason Gunthorpe wrote:
> 
> > I'm guessing that if the driver probe order is tpm_crb,tpm_tis then
> > things work because tpm_crb will claim the device first? Otherwise
> > tpm_tis claims these things unconditionally? If the probe order is
> > reversed things become broken?
> 
> Okay, I didn't find the is_fifo before, so that make sense
> 
> But this:
> 
> > What is the address tpm_tis should be using? I see two things, it
> > either uses the x86 default address or it expects the ACPI to have a
> > MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> > I removed that code in this series. Perhaps tpm_tis should be using
> > control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> > why isn't tpm_crb using one?)
> 
> Is then still a problem. On Martin's system the MSFT0101 device does
> not have a struct resource attached to it. Does any system, or is this
> just dead code?
> 
> Should the control_area_pa be used?

I guess it'd be more realiable. In my NUC the current fix works and the
people who tested it. If you supply me a fix that changes it to use that
I can test it and this will give also coverage to the people who tested
my original fix.

/Jarkko

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:

> I guess it'd be more realiable. In my NUC the current fix works and the
> people who tested it. If you supply me a fix that changes it to use that
> I can test it and this will give also coverage to the people who tested
> my original fix.

Here is the updated series:

https://github.com/jgunthorpe/linux/commits/for-jarkko

What does your dmesg say?

It really isn't OK to hardwire an address for acpi devices, so I've
added something like this. Just completely guessing that control_pa is
where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?

>From c9f7c0465008657f7fc7880496f68f4a1b3b4a26 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Thu, 3 Dec 2015 10:58:56 -0700
Subject: [PATCH 3/5] tpm_tis: Do not fall back to a hardcoded address for TPM2

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.

WIP: Guess that the control_address is the base address for the
TIS 1.2 memory mapped interface.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm_tis.c | 50 +++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index fecd27b45fd1..6b28f8003425 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)) {
+		dev_err(&acpi_dev->dev,
+			FW_BUG "failed to get TPM2 ACPI table\n");
+		return -ENODEV;
+	}
+
+	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&resources);
@@ -996,6 +978,14 @@ 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) {
+		tpm_info.start = tbl->control_address;
+		tpm_info.len = TIS_MEM_LEN;
+		dev_err(&acpi_dev->dev,
+			FW_BUG "TPM2 ACPI table does not define a memory resource, using 0x%lx instead\n",
+			tpm_info.start);
+	}
+
 	if (is_itpm(acpi_dev))
 		itpm = true;
 
-- 
2.1.4

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Thu, Dec 03, 2015 at 11:19:32AM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:
> 
> > I guess it'd be more realiable. In my NUC the current fix works and the
> > people who tested it. If you supply me a fix that changes it to use that
> > I can test it and this will give also coverage to the people who tested
> > my original fix.
> 
> Here is the updated series:
> 
> https://github.com/jgunthorpe/linux/commits/for-jarkko
> 
> What does your dmesg say?
> 
> It really isn't OK to hardwire an address for acpi devices, so I've
> added something like this. Just completely guessing that control_pa is
> where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?

I'm a bit confused about the discussion because Martin replied that
tpm_tis used to get the address range before applying this series.

And pnp_driver in the backend for TPM 1.x devices grabs the address
range from DSDT.

/Jarkko

> From c9f7c0465008657f7fc7880496f68f4a1b3b4a26 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Date: Thu, 3 Dec 2015 10:58:56 -0700
> Subject: [PATCH 3/5] tpm_tis: Do not fall back to a hardcoded address for TPM2
> 
> 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.
> 
> WIP: Guess that the control_address is the base address for the
> TIS 1.2 memory mapped interface.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 50 +++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index fecd27b45fd1..6b28f8003425 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)) {
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "failed to get TPM2 ACPI table\n");
> +		return -ENODEV;
> +	}
> +
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	INIT_LIST_HEAD(&resources);
> @@ -996,6 +978,14 @@ 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) {
> +		tpm_info.start = tbl->control_address;
> +		tpm_info.len = TIS_MEM_LEN;
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource, using 0x%lx instead\n",
> +			tpm_info.start);
> +	}
> +
>  	if (is_itpm(acpi_dev))
>  		itpm = true;
>  
> -- 
> 2.1.4
> 

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Sun, Dec 06, 2015 at 06:02:26AM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 03, 2015 at 11:19:32AM -0700, Jason Gunthorpe wrote:
> > On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:
> > 
> > > I guess it'd be more realiable. In my NUC the current fix works and the
> > > people who tested it. If you supply me a fix that changes it to use that
> > > I can test it and this will give also coverage to the people who tested
> > > my original fix.
> > 
> > Here is the updated series:
> > 
> > https://github.com/jgunthorpe/linux/commits/for-jarkko
> > 
> > What does your dmesg say?
> > 
> > It really isn't OK to hardwire an address for acpi devices, so I've
> > added something like this. Just completely guessing that control_pa is
> > where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?
> 
> I'm a bit confused about the discussion because Martin replied that
> tpm_tis used to get the address range before applying this series.
> 
> And pnp_driver in the backend for TPM 1.x devices grabs the address
> range from DSDT.

You can completely ignore this question. I saw Martins reply with a fix for
"tpm_tis: Use devm_ioremap_resource" that you should squash into that
change. So it's proved that TPM ACPI device objects do not always have a
memory resource. Good.

I think these changes are important but there's no really reason to rush
them. Maybe, since there's been a lot of commentary, it'd be better to
resubmit a new revision of the series to the mailing list so that it can
be peer-reviewed once again.

> /Jarkko

/Jarkko

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Sun, Dec 06, 2015 at 06:15:44AM +0200, Jarkko Sakkinen wrote:
> On Sun, Dec 06, 2015 at 06:02:26AM +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 03, 2015 at 11:19:32AM -0700, Jason Gunthorpe wrote:
> > > On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > I guess it'd be more realiable. In my NUC the current fix works and the
> > > > people who tested it. If you supply me a fix that changes it to use that
> > > > I can test it and this will give also coverage to the people who tested
> > > > my original fix.
> > > 
> > > Here is the updated series:
> > > 
> > > https://github.com/jgunthorpe/linux/commits/for-jarkko
> > > 
> > > What does your dmesg say?
> > > 
> > > It really isn't OK to hardwire an address for acpi devices, so I've
> > > added something like this. Just completely guessing that control_pa is
> > > where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?
> > 
> > I'm a bit confused about the discussion because Martin replied that
> > tpm_tis used to get the address range before applying this series.
> > 
> > And pnp_driver in the backend for TPM 1.x devices grabs the address
> > range from DSDT.
> 
> You can completely ignore this question. I saw Martins reply with a fix for
> "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> change. So it's proved that TPM ACPI device objects do not always have a
> memory resource. Good.
> 
> I think these changes are important but there's no really reason to rush
> them. Maybe, since there's been a lot of commentary, it'd be better to
> resubmit a new revision of the series to the mailing list so that it can
> be peer-reviewed once again.

Maybe even there could be a common tpm_tcg driver once the common code
has been factored out (at some point, lets take this step by step and
fix the issues first). Transmit functions are not heavy and ACPI stuff
is mostly the same.

/Jarkko

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Sun, Dec 06, 2015 at 06:15:44AM +0200, Jarkko Sakkinen wrote:
> You can completely ignore this question. I saw Martins reply with a fix for
> "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> change.

It isn't quite the right fix - but I've added something that should be OK
now that Martin has debugged it.

> So it's proved that TPM ACPI device objects do not always have a
> memory resource. Good.

Hopefully.. It is so confusing because tpm_tis had that wrong
fallback, and tpm_crb incorrectly doesn't use the resource
structure. So we've never actually had anything that requires the
resource struct for 2.0 until now.

> resubmit a new revision of the series to the mailing list so that it can
> be peer-reviewed once again.

If Martin is happy I will send them to the list, the latest patches
are on my github:

https://github.com/jgunthorpe/linux/commits/for-jarkko

Jason

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 752 bytes --]

> > I'm a bit confused about the discussion because Martin replied that
> > tpm_tis used to get the address range before applying this series.
> > 
> > And pnp_driver in the backend for TPM 1.x devices grabs the address
> > range from DSDT.
> 
> You can completely ignore this question. I saw Martins reply with a fix for
> "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> change. So it's proved that TPM ACPI device objects do not always have a
> memory resource. Good.

Repeat, the memory resource DOES exist on my system. Not sure what proof
you saw there.

Martin

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Mon, Dec 07, 2015 at 09:06:50AM +0100, Wilck, Martin wrote:
> > > I'm a bit confused about the discussion because Martin replied that
> > > tpm_tis used to get the address range before applying this series.
> > > 
> > > And pnp_driver in the backend for TPM 1.x devices grabs the address
> > > range from DSDT.
> > 
> > You can completely ignore this question. I saw Martins reply with a fix for
> > "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> > change. So it's proved that TPM ACPI device objects do not always have a
> > memory resource. Good.
> 
> Repeat, the memory resource DOES exist on my system. Not sure what proof
> you saw there.

Ok, lets go this through.

I deduced this from two facts:

* It used to have memory resource as conditional and as a fallback use
  fixed value.
* Your workaround reverted the situation to this.

Did I understand something incorrectly?

/Jarkko

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1528 bytes --]

> > > You can completely ignore this question. I saw Martins reply with a fix for
> > > "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> > > change. So it's proved that TPM ACPI device objects do not always have a
> > > memory resource. Good.
> > 
> > Repeat, the memory resource DOES exist on my system. Not sure what proof
> > you saw there.
> 
> Ok, lets go this through.
> 
> I deduced this from two facts:
> 
> * It used to have memory resource as conditional and as a fallback use
>   fixed value.
> * Your workaround reverted the situation to this.
> 
> Did I understand something incorrectly?

The problem in my case didn't occur because ACPI was lacking a resource.
It has one "extra" resource that Jason's original code didn't
recognize. 

Jason's code was wrongly assuming that a resource that isn't of type
"IRQ" has to be of type "MEMORY". If I print out the resource types
encountered in tpm_check_resource(), I get
ACPI_RESOURCE_TYPE_FIXED_MEMORY32  (0x0a) first, followed by
ACPI_RESOURCE_TYPE_END_TAG (0x07). The latter was mistakenly used by
Jason't code as a memory resource. This is how ACPI ResourceTemplates
work (a list with an end marker). The correct solution is to always
check the return value of acpi_dev_resource_memory(), as it's currently
implemented in Jason't current "for-jarkko" branch.

Martin


> 
> /Jarkko
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Mon, Dec 07, 2015 at 10:52:51AM +0100, Wilck, Martin wrote:
> > > > You can completely ignore this question. I saw Martins reply with a fix for
> > > > "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> > > > change. So it's proved that TPM ACPI device objects do not always have a
> > > > memory resource. Good.
> > > 
> > > Repeat, the memory resource DOES exist on my system. Not sure what proof
> > > you saw there.
> > 
> > Ok, lets go this through.
> > 
> > I deduced this from two facts:
> > 
> > * It used to have memory resource as conditional and as a fallback use
> >   fixed value.
> > * Your workaround reverted the situation to this.
> > 
> > Did I understand something incorrectly?
> 
> The problem in my case didn't occur because ACPI was lacking a resource.
> It has one "extra" resource that Jason's original code didn't
> recognize. 
> 
> Jason's code was wrongly assuming that a resource that isn't of type
> "IRQ" has to be of type "MEMORY". If I print out the resource types
> encountered in tpm_check_resource(), I get
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32  (0x0a) first, followed by
> ACPI_RESOURCE_TYPE_END_TAG (0x07). The latter was mistakenly used by
> Jason't code as a memory resource. This is how ACPI ResourceTemplates
> work (a list with an end marker). The correct solution is to always
> check the return value of acpi_dev_resource_memory(), as it's currently
> implemented in Jason't current "for-jarkko" branch.

Aah. Right.

> Martin

/Jarkko

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 983 bytes --]

On Mi, 2015-12-02 at 12:11 -0700, Jason Gunthorpe wrote:

> > What is the address tpm_tis should be using? I see two things, it
> > either uses the x86 default address or it expects the ACPI to have a
> > MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> > I removed that code in this series. Perhaps tpm_tis should be using
> > control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> > why isn't tpm_crb using one?)
> 
> Is then still a problem. On Martin's system the MSFT0101 device does
> not have a struct resource attached to it. Does any system, or is this
> just dead code?

ACPI defines a mem resource corresponding to the standard TIS memory
area on my system, and it used to be detected fine with Jarkko's patch.
Somehow your latest changes broke it, not sure why.

Martin

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Thu, Dec 03, 2015 at 09:30:30AM +0100, Wilck, Martin wrote:
> On Mi, 2015-12-02 at 12:11 -0700, Jason Gunthorpe wrote:
> 
> > > What is the address tpm_tis should be using? I see two things, it
> > > either uses the x86 default address or it expects the ACPI to have a
> > > MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> > > I removed that code in this series. Perhaps tpm_tis should be using
> > > control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> > > why isn't tpm_crb using one?)
> > 
> > Is then still a problem. On Martin's system the MSFT0101 device does
> > not have a struct resource attached to it. Does any system, or is this
> > just dead code?
> 
> ACPI defines a mem resource corresponding to the standard TIS memory
> area on my system, and it used to be detected fine with Jarkko's patch.
> Somehow your latest changes broke it, not sure why.

Are you certain? Based on what you sent me, that output is only
possible if there is no mem resource.

With the prior arrangement no mem resource means the x86 default
address is used, which is the only way I can see how your system
works.

Jason

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1554 bytes --]




On Do, 2015-12-03 at 10:00 -0700, Jason Gunthorpe wrote:
> On Thu, Dec 03, 2015 at 09:30:30AM +0100, Wilck, Martin wrote:
> > On Mi, 2015-12-02 at 12:11 -0700, Jason Gunthorpe wrote:
> > 
> > > > What is the address tpm_tis should be using? I see two things, it
> > > > either uses the x86 default address or it expects the ACPI to have a
> > > > MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> > > > I removed that code in this series. Perhaps tpm_tis should be using
> > > > control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> > > > why isn't tpm_crb using one?)
> > > 
> > > Is then still a problem. On Martin's system the MSFT0101 device does
> > > not have a struct resource attached to it. Does any system, or is this
> > > just dead code?
> > 
> > ACPI defines a mem resource corresponding to the standard TIS memory
> > area on my system, and it used to be detected fine with Jarkko's patch.
> > Somehow your latest changes broke it, not sure why.
> 
> Are you certain? Based on what you sent me, that output is only
> possible if there is no mem resource.

Yes, I am certain. I checked the DSDT, and I put a debug statement right
after the resource detection in tpm_tis.

Martin


> With the prior arrangement no mem resource means the x86 default
> address is used, which is the only way I can see how your system
> works.



> 
> Jason
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1393 bytes --]

> > ACPI defines a mem resource corresponding to the standard TIS memory
> > area on my system, and it used to be detected fine with Jarkko's patch.
> > Somehow your latest changes broke it, not sure why.
> 
> Are you certain? Based on what you sent me, that output is only
> possible if there is no mem resource.
> 
> With the prior arrangement no mem resource means the x86 default
> address is used, which is the only way I can see how your system
> works.

The following simple change fixes the ACPI probing after applying your
latest series. The must have been another ACPI resource that you were
erroneously using as mem resource. 

The IS_ERR change() didn't fix it. I think it's not needed, although it
probably can't hurt.

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index a1898c8..4c65a7d 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -954,7 +954,8 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
 
 	if (acpi_dev_resource_interrupt(ares, 0, &res))
 		tpm_info->irq = res.start;
-	acpi_dev_resource_memory(ares, &tpm_info->res);
+	else if (acpi_dev_resource_memory(ares, &res))
+		memcpy(&tpm_info->res, &res, sizeof(res));
 
 	return 1;
 }

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Fri, Dec 04, 2015 at 10:10:15AM +0100, Wilck, Martin wrote:

> The following simple change fixes the ACPI probing after applying your
> latest series. The must have been another ACPI resource that you were
> erroneously using as mem resource. 

Close, acpi_dev_resource_memory destroys it's output parameter when it
fails :(

Should be:

	if (acpi_dev_resource_interrupt(ares, 0, &res))
		tpm_info->irq = res.start;
	else if (acpi_dev_resource_memory(ares, &res))
		tpm_info->res = res;

> The IS_ERR change() didn't fix it. I think it's not needed, although it
> probably can't hurt.

IS_ERR should address the oops though??

I've put all the revised patches here:

https://github.com/jgunthorpe/linux/commits/for-jarkko

If you are OK with them now I'll post the series.

Jason

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 965 bytes --]

> IS_ERR should address the oops though??

No, see my answer to Jarkko in the other part of the thread.

> I've put all the revised patches here:
> 
> https://github.com/jgunthorpe/linux/commits/for-jarkko
> 
> If you are OK with them now I'll post the series.

I haven't re-reviewed it, but the test went alright. 

As reported before, with "force=1", I get the error message:

[ 1351.677808] tpm_tis MSFT0101:00: can't request region for resource
[mem 0xfed40000-0xfed44fff]
[ 1351.687431] tpm_tis: probe of MSFT0101:00 failed with error -16

This is kind of misleading because the TPM is actually working as a
platform device. But I can follow your previous argument that this is
acceptable because people who use "force=1" should know what they are
doing, so I don't regard this as critical.

Martin

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Mon, Dec 07, 2015 at 10:59:15AM +0100, Wilck, Martin wrote:
> > IS_ERR should address the oops though??
> 
> No, see my answer to Jarkko in the other part of the thread.

I'm confused, is there an oops that still need to be fixed?

> As reported before, with "force=1", I get the error message:
> 
> [ 1351.677808] tpm_tis MSFT0101:00: can't request region for resource
> [mem 0xfed40000-0xfed44fff]
> [ 1351.687431] tpm_tis: probe of MSFT0101:00 failed with error -16

Great, that you so much, that is what I expected to see!

> This is kind of misleading because the TPM is actually working as a
> platform device. But I can follow your previous argument that this is
> acceptable because people who use "force=1" should know what they are
> doing, so I don't regard this as critical.

Right.

Jason

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jarkko Sakkinen]

On Wed, Dec 02, 2015 at 11:27:27AM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 01, 2015 at 11:58:26AM -0700, Jason Gunthorpe wrote:
> 
> > I went through the patches and didn't see anything that would shock me
> > enough not to apply the patches in the current if they also work when
> > tested *but* are these release critical for Linux v4.4?
> 
> Jarkko,
> 
> Can you explain how
> 
> commit 399235dc6e95400a1322a9999e92073bc572f0c8
> Author: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Date:   Tue Sep 29 00:32:19 2015 +0300
> 
>     tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0
> 
> Is supposed to work? I get the jist of the idea, but I'm not seeing
> how it can work reliably..

The idea is that circulate the problem that pnp driver infra can pass at
most 7 character device IDs and MSFT0101 (used for TPM2 devices) has 8
characters. They have disjoint sets of device IDs so both cannot ever
attach. I don't know who was idiot enough to invent 8 character device
ID for TPM2 devices but that's the reality.

It's not a perfect fix but I couldn't figure out anything more clever
at that time. And nobody else was paying attention to the issue so
I had to do something and people who reported bug tested the patch and
were happy so I'm confident I did the right thing in the situation.

> The idea is to pass off TPM2_START_FIFO to tpm_tis?
> 
> I'm guessing that if the driver probe order is tpm_crb,tpm_tis then
> things work because tpm_crb will claim the device first? Otherwise
> tpm_tis claims these things unconditionally? If the probe order is
> reversed things become broken?
> 
> What is the address tpm_tis should be using? I see two things, it
> either uses the x86 default address or it expects the ACPI to have a
> MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> I removed that code in this series. Perhaps tpm_tis should be using
> control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> why isn't tpm_crb using one?)

Doesn't also PNP driver do this assumption when the backend is ACPI?

> There is also something wrong with the endianness in the acpi
> stuff. I don't see endianness conversions in other acpi places, so I
> wonder if the ones in tpm_crb are correct. If they are correct then
> the struct needs le/be notations and there are some missing
> conversions.

/Jarkko

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4681 bytes --]

On Di, 2015-12-01 at 11:58 -0700, Jason Gunthorpe wrote:

> Martin, this should fix the double loading you noticed, please confirm.  There
> is a possibility the force path needs a bit more code to be compatible with
> devm_ioremap_resource, I'm not sure, hoping not.

Nope, this one oopses in the ACPI probing path.

[   12.287350] tpm_tis MSFT0101:00: invalid resource
[   12.292625] BUG: unable to handle kernel paging request at ffffffffffffffea
[   12.300427] IP: [<ffffffff81337481>] ioread8+0x31/0x40
[   12.306188] PGD 1a19067 PUD 1a1b067 PMD 0 
[   12.310793] Oops: 0000 [#1] SMP 
[   12.314416] Modules linked in: tpm_tis(+) nfsd auth_rpcgss nfs_acl lockd grace sunrpc sch_fq_codel ip_tables xfs libcrc32c sr_mod cdrom sd_mod mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ahci ttm libahci drm be2net vxlan libata ip6_udp_tunnel udp_tunnel
[   12.343483] CPU: 14 PID: 826 Comm: systemd-udevd Not tainted 4.4.0-rc2+ #15
[   12.351263] Hardware name: FUJITSU PRIMERGY RX2530 M1/D3279-B1, BIOS V5.0.0.11 R0.74.0 for D3279-B1x                    09/21/2015
[   12.364367] task: ffff88046bd52a80 ti: ffff88046bc94000 task.ti: ffff88046bc94000
[   12.372720] RIP: 0010:[<ffffffff81337481>]  [<ffffffff81337481>] ioread8+0x31/0x40
[   12.381205] RSP: 0018:ffff88046bc97a60  EFLAGS: 00010296
[   12.387142] RAX: ffffffffffffffea RBX: ffff88086ce27800 RCX: 0000000000000000
[   12.395113] RDX: 0000000000000001 RSI: ffff88086f10dff8 RDI: ffffffffffffffea
[   12.403077] RBP: ffff88046bc97ab0 R08: 000000000000000a R09: 0000000000000000
[   12.411042] R10: 0000000000000000 R11: 00000000000003ea R12: 00000000fffb9d01
[   12.419006] R13: ffff88086c6e0a68 R14: ffff88046bc97ad0 R15: ffff88046f4f8118
[   12.426972] FS:  00007fa4af348880(0000) GS:ffff88086f100000(0000) knlGS:0000000000000000
[   12.436002] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.442414] CR2: ffffffffffffffea CR3: 000000046bd7b000 CR4: 00000000001406e0
[   12.450378] Stack:
[   12.452622]  ffffffffa0121f21 ffff88046bc97ac0 ffffffffa0121600 ffff88046bc97ad0
[   12.460922]  0000000071883a14 ffff88086c6e0800 ffff88046bc97ad0 ffff88046bc97ac0
[   12.469221]  0000000000000009 ffff88046a44f540 ffff88046bc97b30 ffffffffa0122772
[   12.477518] Call Trace:
[   12.480252]  [<ffffffffa0121f21>] ? tpm_tis_init+0xf1/0x750 [tpm_tis]
[   12.487451]  [<ffffffffa0121600>] ? tpm_tis_probe_irq_single+0x160/0x160 [tpm_tis]
[   12.495894]  [<ffffffffa0122772>] tpm_tis_acpi_init+0xb2/0x120 [tpm_tis]
[   12.503387]  [<ffffffff81396e4a>] acpi_device_probe+0x4a/0xf7
[   12.509809]  [<ffffffff814564b9>] driver_probe_device+0x169/0x450
[   12.516620]  [<ffffffff81456825>] __driver_attach+0x85/0x90
[   12.522839]  [<ffffffff814567a0>] ? driver_probe_device+0x450/0x450
[   12.529837]  [<ffffffff8145427c>] bus_for_each_dev+0x6c/0xc0
[   12.536161]  [<ffffffff81455ece>] driver_attach+0x1e/0x20
[   12.542188]  [<ffffffff814559e0>] bus_add_driver+0x1d0/0x290
[   12.548510]  [<ffffffffa013d000>] ? 0xffffffffa013d000
[   12.554244]  [<ffffffff814571d0>] driver_register+0x60/0xe0
[   12.560463]  [<ffffffff81396d1e>] acpi_bus_register_driver+0x3b/0x43
[   12.567564]  [<ffffffffa013d08f>] init_tis+0x8f/0x1000 [tpm_tis]
[   12.574279]  [<ffffffff8132d8be>] ? kasprintf+0x4e/0x70
[   12.580116]  [<ffffffffa013d000>] ? 0xffffffffa013d000
[   12.585853]  [<ffffffff8100213d>] do_one_initcall+0xcd/0x1f0
[   12.592171]  [<ffffffff811d619b>] ? kmem_cache_alloc_trace+0x17b/0x1e0
[   12.599468]  [<ffffffff81179808>] ? do_init_module+0x27/0x1e8
[   12.605890]  [<ffffffff81179841>] do_init_module+0x60/0x1e8
[   12.612111]  [<ffffffff811002ae>] load_module+0x1c2e/0x24c0
[   12.618330]  [<ffffffff810fcab0>] ? __symbol_put+0x60/0x60
[   12.624453]  [<ffffffff810fce30>] ? copy_module_from_fd.isra.54+0x110/0x160
[   12.632229]  [<ffffffff81100d4f>] SyS_finit_module+0x9f/0xd0
[   12.638549]  [<ffffffff816bdb6e>] entry_SYSCALL_64_fastpath+0x12/0x71
[   12.645745] Code: 00 77 28 48 81 ff 00 00 01 00 76 08 0f b7 d7 ec 0f b6 c0 c3 55 48 c7 c6 10 fc 94 81 48 89 e5 e8 96 ff ff ff b8 ff 00 00 00 5d c3 <8a> 07 0f b6 c0 c3 66 0f 1f 84 00 00 00 00 00 48 81 ff ff ff 03 
[   12.667510] RIP  [<ffffffff81337481>] ioread8+0x31/0x40
[   12.673353]  RSP <ffff88046bc97a60>
[   12.677244] CR2: ffffffffffffffea
[   12.680943] ---[ end trace 5854533536fd5101 ]---
[   12.687465] Kernel panic - not syncing: Fatal exception
[   12.693338] Kernel Offset: disabled
[   12.701145] ---[ end Kernel panic - not syncing: Fatal exception


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

On Wed, Dec 02, 2015 at 01:34:38PM +0100, Wilck, Martin wrote:
> On Di, 2015-12-01 at 11:58 -0700, Jason Gunthorpe wrote:
> 
> > Martin, this should fix the double loading you noticed, please confirm.  There
> > is a possibility the force path needs a bit more code to be compatible with
> > devm_ioremap_resource, I'm not sure, hoping not.
> 
> Nope, this one oopses in the ACPI probing path.

This fixes this oops:

        chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
-       if (!chip->vendor.iobase)
-               return -EIO;
+       if (IS_ERR(chip->vendor.iobase))
+               return PTR_ERR(chip->vendor.iobase);


And I see that the ACPI stuff needs other work :(

Jason