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.


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

drivers/char/tpm/tpm_tis.c | 177 +++++++++++++++++++++++++++------------------
1 file changed, 108 insertions(+), 69 deletions(-)

--
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Jason Gunthorpe]

The TPM core has long assumed that every device has a driver attached,
however 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...@ts.fujitsu.com>
Signed-off-by: Jason Gunthorpe <jgunt...@obsidianresearch.com>
---
drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++-----------------
1 file changed, 101 insertions(+), 60 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 0a2d94f3d679..0e5c282aa37e 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 */
};
@@ -72,12 +70,6 @@ struct tpm_info {
int irq;
};

-static struct tpm_info tis_default_info = {
- .start = TIS_MEM_BASE,
- .len = TIS_MEM_LEN,
- .irq = 0,
-};
-
/* Some timeout values are needed before it is known whether the chip is
* TPM 1.0 or TPM 2.0.
*/
@@ -847,7 +839,6 @@ out_err:
return rc;
}

-#ifdef CONFIG_PM_SLEEP
static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
{
u32 intmask;
@@ -889,11 +880,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)
{
@@ -908,14 +897,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);
}
@@ -956,7 +943,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)
@@ -1029,80 +1015,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.start = res->start;
+ tpm_info.len = resource_size(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 = force_device();
+ if (rc)
+ goto out1;
+
+ if (IS_ENABLED(CONFIG_PNP)) {
rc = pnp_register_driver(&tis_pnp_driver);
if (rc)
- return rc;
+ goto out2;
}
-#endif
+
#ifdef CONFIG_ACPI
- if (!force) {
- rc = acpi_bus_register_driver(&tis_acpi_driver);
- if (rc) {
-#ifdef CONFIG_PNP
- pnp_unregister_driver(&tis_pnp_driver);
-#endif
- return rc;
- }
- }
+ rc = acpi_bus_register_driver(&tis_acpi_driver);
+ if (rc)
+ goto out3;
#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;
- }
- rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
if (rc)
- goto err_init;
+ goto out4;
+
return 0;
-err_init:
- platform_device_unregister(pdev);
-err_dev:
- platform_driver_unregister(&tis_drv);
+out4:
+#ifdef CONFIG_ACPI
+ acpi_bus_unregister_driver(&tis_acpi_driver);
+out3:
+#endif
+ pnp_unregister_driver(&tis_pnp_driver);
+out2:
+ platform_device_unregister(force_pdev);
+out1:
return rc;
}

static void __exit cleanup_tis(void)
{
- struct tpm_chip *chip;
-#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
- if (!force) {
+ platform_driver_unregister(&tis_drv);
#ifdef CONFIG_ACPI
- acpi_bus_unregister_driver(&tis_acpi_driver);
-#endif
-#ifdef CONFIG_PNP
- pnp_unregister_driver(&tis_pnp_driver);
+ acpi_bus_unregister_driver(&tis_acpi_driver);
#endif
- return;
- }
-#endif
- chip = dev_get_drvdata(&pdev->dev);
- tpm_chip_unregister(chip);
- tpm_tis_remove(chip);
- platform_device_unregister(pdev);
- platform_driver_unregister(&tis_drv);
+ pnp_unregister_driver(&tis_pnp_driver);
+
+ if (force_pdev)
+ platform_device_unregister(force_pdev);
+
}

module_init(init_tis);

[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 <jgunt...@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;

[Uwe Kleine-König]

Hello Jason,

On Mon, Nov 30, 2015 at 12:27:11PM -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.

Is there a reason not to use 0 for the invalid irq?

> The only thing that asks for autoprobing is the force=1 path.
>
> Signed-off-by: Jason Gunthorpe <jgunt...@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) {

There is even a check for irq == 0 that could be reused maybe?

Best regards
Uwe

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

[Uwe Kleine-König]

On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> called unconditionally") breaks that assumption.

Maybe it's worth to point out that b8b2c7d845d5 didn't break it on
purpose and is fixed accordingly. Still the assumption isn't valid, but
works in practise.

indention problems here.

> + dev_err(&pdev->dev, "no memory resource defined\n");
> + return -ENODEV;
> + }
> + tpm_info.start = res->start;
> + tpm_info.len = resource_size(res);
> +
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (res)
> + tpm_info.irq = res->start;
> + else {

If one branch of an if/else has braces, all of them should.

> + if (pdev == force_pdev)
> + tpm_info.irq = -1;
> + else
> + /* When forcing auto probe the IRQ */
> + tpm_info.irq = 0;
> + }

ah, so 0 means autoprobe and -1 means invalid. Hmm.

Is this added ifdef intended to be in this commit?

Might be a matter of taste, but having nicer names for the error labels
makes review easier. For example I would have called "out3"
"err_register_acpi" instead. Then you can easily verify that it's placed
right in the error path being directly after acpi_bus_unregister_driver.

Also all kind of strange things happen if you later need to add a label
between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5"
in a similar situation. The alternative is to renumber the label makeing
patch review still harder.

Best regards
Uwe

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

[Jarkko Sakkinen]

Hi

Agreed that named labels would be the preferred choice. It's hard to
make sense of these labels (or at least slower than with named labels).

In addition I want this fix as a single patch, not as two-patch set.
The first patch might have made sense when the fix was being developed
but now it's just really akward change.

I had to squash the patches to make any sense of this.

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

/Jarkko

[Wilck, Martin]

On Mo, 2015-11-30 at 12:27 -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however 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...@ts.fujitsu.com>
> Signed-off-by: Jason Gunthorpe <jgunt...@obsidianresearch.com>

I tested this on my system, deliberately reverting my own fix for
platform_driver_probe() beforehand. It works, no panic any more.

The patch introduces one user-visible change, because now the ACPI and
PnP drivers are registered even with "force=1". This causes my TPM to be
show up twice in sysfs:

/sys/bus/acpi/drivers/tpm_tis/MSFT0101:00
-> ../../../../devices/LNXSYSTM:00/LNXSYBUS:00/MSFT0101:00
/sys/bus/platform/drivers/tpm_tis/tpm_tis
-> ../../../../devices/platform/tpm_tis

Only the platform device is actually bound to the physical device,
though. I'm not sure if I like this.

Regards
Martin



N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±‘êçzX§¶ ›¡Ü¨}©ž²Æ zÚ&j:+v‰¨¾ «‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹ ®w¥¢¸?™¨è­Ú&¢)ߢ f”ù^jÇ«y§m…á@A«a¶Ú ÿ0¶ìh® å’i

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:17:43AM +0100, Uwe Kleine-König wrote:
> Hello Jason,
>
> On Mon, Nov 30, 2015 at 12:27:11PM -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.
>
> Is there a reason not to use 0 for the invalid irq?

0 already means 'no IRQ given, do auto-probe'. -1 is now 'no IRQ
given, disable IRQ operation'

Jason

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:28:35AM +0100, Uwe Kleine-König wrote:

> On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote:
> > The TPM core has long assumed that every device has a driver attached,
> > however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> > called unconditionally") breaks that assumption.
>
> Maybe it's worth to point out that b8b2c7d845d5 didn't break it on
> purpose and is fixed accordingly. Still the assumption isn't valid, but
> works in practise.

Purposeful or not, it is the source of the bug this patch is
fixing.. I'm happy with any language, proposal?

> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
>
> indention problems here.

Woops, forgot to run check patch..

> > + if (res)
> > + tpm_info.irq = res->start;
> > + else {
>
> If one branch of an if/else has braces, all of them should.

Is that a thing now? Surprised checkpatch doesn't complain.

> > static bool force;
> > +#ifdef CONFIG_X86
> > module_param(force, bool, 0444);
> > MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
> > +#endif
>
> Is this added ifdef intended to be in this commit?

Yes, upon auditing all this it is clear the values we have are
hard-wired to x86, so this will never work on another platform.

I'm happy to put that in another patch.

> > -err_init:
> > - platform_device_unregister(pdev);
> > -err_dev:
> > - platform_driver_unregister(&tis_drv);
> > +out4:
> > +#ifdef CONFIG_ACPI
> > + acpi_bus_unregister_driver(&tis_acpi_driver);
> > +out3:
> > +#endif
> > + pnp_unregister_driver(&tis_pnp_driver);
> > +out2:
> > + platform_device_unregister(force_pdev);
> > +out1:
>
> Might be a matter of taste, but having nicer names for the error labels
> makes review easier. For example I would have called "out3"
> "err_register_acpi" instead. Then you can easily verify that it's placed
> right in the error path being directly after
> acpi_bus_unregister_driver.

The downside is it is harder to review the goto sites because there is
no longer much logic to their ordering, but sure, names seem a bit
more common in tpm.

> Also all kind of strange things happen if you later need to add a label
> between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5"
> in a similar situation.

Yes, it isn't so bad to do that.

Jason

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 12:50:28PM +0100, Wilck, Martin wrote:

> The patch introduces one user-visible change, because now the ACPI and
> PnP drivers are registered even with "force=1". This causes my TPM to be
> show up twice in sysfs:

Registering all the drivers is deliberate, IMHO, force should not be
used if the driver binds normally.

However, getting two is a bug, and it is because tpm_tis is not doing
resource management.

I'll add another patch to fix that..

Also, I'll change the order of the driver registers so that the
platform driver goes first, that way if force is used the platform
driver will bind and still auto probe irqs and then the other drivers
will bounce off the claimed resource.

Jason

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 10:35:08AM +0200, Jarkko Sakkinen wrote:

> In addition I want this fix as a single patch, not as two-patch set.
> The first patch might have made sense when the fix was being developed
> but now it's just really akward change.

No, you are not in tune with the kernel standard when you are
suggesting merging these patches. Each patch is self contained, encompasses a
single idea/change, and is justifiable on its own.

Ie SubmittingPatches explains:

The point to remember is that each patch should make an easily understood
change that can be verified by reviewers. Each patch should be
justifiable on its own merits.

If anything the larger patch should be split, because there is alot
going on there..

Jason

[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

[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 <jgunt...@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;

[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 <jgunt...@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;

[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...@ts.fujitsu.com>

Signed-off-by: Jason Gunthorpe <jgunt...@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);

[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 <jgunt...@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
*/

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

[Uwe Kleine-König]

Hello,

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

[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...@ts.fujitsu.com>
> Signed-off-by: Jason Gunthorpe <jgunt...@obsidianresearch.com>

Best regards,

Uwe

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

[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

[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

[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

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

[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

[Jarkko Sakkinen]

On Tue, Dec 01, 2015 at 10:43:42AM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 10:35:08AM +0200, Jarkko Sakkinen wrote:
>
> > In addition I want this fix as a single patch, not as two-patch set.
> > The first patch might have made sense when the fix was being developed
> > but now it's just really akward change.
>
> No, you are not in tune with the kernel standard when you are
> suggesting merging these patches. Each patch is self contained, encompasses a
> single idea/change, and is justifiable on its own.
>
> Ie SubmittingPatches explains:
>
> The point to remember is that each patch should make an easily understood
> change that can be verified by reviewers. Each patch should be
> justifiable on its own merits.
>
> If anything the larger patch should be split, because there is alot
> going on there..

Just saying that at least for me it was easier to understand what was
going on once I squashed the patch. Labels were the only really
confusing part, not the patch size...

> Jason

/Jarkko

[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

[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

[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

[Peter Huewe]

Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <jgunt...@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

[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

[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 <jgunt...@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

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

[Peter Huewe]

Nope, intel fw tpm2.0 in my acer laptop.
No nucs here
>/Jarkko

--
Sent from my mobile

[Wilck, Martin]

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‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±‘êçzX§¶ ›¡Ü¨}©ž²Æ zÚ&j:+v‰¨¾ «‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹ ®w¥¢¸?™¨è­Ú&¢)ߢ f”ù^jÇ«y§m…á@A«a¶Ú ÿ0¶ìh® å’i

[Greg Kroah-Hartman]

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

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

[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

[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....@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

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

[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....@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

[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

[Wilck, Martin]

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Ç+‰·Ÿ®‰­†+%ŠËlzwm…éb 맲æìr¸›zX§» ®w¥Š{ayº ʇڙë,j ­¢f£¢·hš‹àz¹ ®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾ «‘êçzZ+ƒùšŽŠÝ¢j" ú!¶iO•æ¬z·švØ^ ¶ m§ÿðà nÆŠàþY&—

[martin...@ts.fujitsu.com]

From: Martin Wilck <Martin...@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...@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

[Uwe Kleine-König]

Hello Martin,

On Thu, Dec 03, 2015 at 09:51:44AM +0100, martin...@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin...@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...@ts.fujitsu.com>

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

Thanks

Uwe

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

[Jarkko Sakkinen]

On Thu, Dec 03, 2015 at 09:51:44AM +0100, martin...@ts.fujitsu.com wrote:

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

/Jarkko

[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 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 <jgunt...@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 <jgunt...@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

[Wilck, Martin]

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

｢鴈ｹｻ ｮ&ﾞ~ｺ&ｶ ｬ�-ｱ鰡ｶ ･学ｮ寨岾ﾊ穃饕樌dzｹﾞ�奛跖w* jgｬｱｨ ｶ凹至ﾝ｢j/�艘ｹﾞ槙�巌勣隴ﾚ&｢)ﾟ｡ｫaｶﾚ �ｮGｫ晞hｮ 詼:+v鴎学閹ﾙ･>W坡�iﾛaxP jﾘmｶ�ﾃ-ｻ +�d喟

[Wilck, Martin]

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

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

[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

[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

[Jarkko Sakkinen]

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.

[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

[Wilck, Martin]

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

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

[Wilck, Martin]

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

[Wilck, Martin]

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

[Jarkko Sakkinen]

Aah. Right.

> Martin

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