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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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" <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
---
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
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 <[email protected]>
> ---
> 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/ |
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 <[email protected]>
> ---
> 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/ |
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" <[email protected]>
> Signed-off-by: Jason Gunthorpe <[email protected]>
Best regards,
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
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
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
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
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/ |
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
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
>
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
>
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
Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <[email protected]>:
>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
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
On Tue, Dec 01, 2015 at 05:15:14PM -0800, Peter Huewe wrote:
>
>
> Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <[email protected]>:
> >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
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/ |
Am 2. Dezember 2015 00:14:23 PST, schrieb Jarkko Sakkinen <[email protected]>:
>On Tue, Dec 01, 2015 at 05:15:14PM -0800, Peter Huewe wrote:
>>
>>
>> Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe
><[email protected]>:
>> >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
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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/ |
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
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 <[email protected]>
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
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
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 <[email protected]>
> 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
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
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
From: Martin Wilck <[email protected]>
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 <[email protected]>
---
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
Hello Martin,
On Thu, Dec 03, 2015 at 09:51:44AM +0100, [email protected] wrote:
> From: Martin Wilck <[email protected]>
>
> 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 <[email protected]>
Acked-by: Uwe Kleine-K?nig <[email protected]>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Dec 03, 2015 at 09:51:44AM +0100, [email protected] wrote:
> From: Martin Wilck <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
/Jarkko
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
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 <[email protected]>
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 <[email protected]>
---
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
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
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
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
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
> > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
> > > 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> 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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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