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
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
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" <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
---
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);
--
2.1.4
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 <[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) {
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/ |
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.
> 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 | 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) {
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.
> +
> + 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
Is this added ifdef intended to be in this commit?
> +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:
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/ |
Hi
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.
>
> > 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 | 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) {
>
> 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.
>
> > +
> > + 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
>
> Is this added ifdef intended to be in this commit?
>
> > +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:
>
> 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.
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
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" <[email protected]>
> Signed-off-by: Jason Gunthorpe <[email protected]>
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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
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
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
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