| Message ID | 1448911632-20070-3-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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" <martin.wilck@ts.fujitsu.com> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++----------------- > 1 file changed, 101 insertions(+), 60 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 0a2d94f3d679..0e5c282aa37e 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -60,8 +60,6 @@ enum tis_int_flags { > }; > > enum tis_defaults { > - TIS_MEM_BASE = 0xFED40000, > - TIS_MEM_LEN = 0x5000, > TIS_SHORT_TIMEOUT = 750, /* ms */ > TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > }; > @@ -72,12 +70,6 @@ struct tpm_info { > int irq; > }; > > -static struct tpm_info tis_default_info = { > - .start = TIS_MEM_BASE, > - .len = TIS_MEM_LEN, > - .irq = 0, > -}; > - > /* Some timeout values are needed before it is known whether the chip is > * TPM 1.0 or TPM 2.0. > */ > @@ -847,7 +839,6 @@ out_err: > return rc; > } > > -#ifdef CONFIG_PM_SLEEP > static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > { > u32 intmask; > @@ -889,11 +880,9 @@ static int tpm_tis_resume(struct device *dev) > > return 0; > } > -#endif > > static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); > > -#ifdef CONFIG_PNP > static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > const struct pnp_device_id *pnp_id) > { > @@ -908,14 +897,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > else > tpm_info.irq = -1; > > -#ifdef CONFIG_ACPI > if (pnp_acpi_device(pnp_dev)) { > if (is_itpm(pnp_acpi_device(pnp_dev))) > itpm = true; > > - acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle; > + acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev); > } > -#endif > > return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle); > } > @@ -956,7 +943,6 @@ static struct pnp_driver tis_pnp_driver = { > module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id, > sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444); > MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe"); > -#endif > > #ifdef CONFIG_ACPI > static int tpm_check_resource(struct acpi_resource *ares, void *data) > @@ -1029,80 +1015,135 @@ static struct acpi_driver tis_acpi_driver = { > }; > #endif > > +static struct platform_device *force_pdev; > + > +static int tpm_tis_plat_probe(struct platform_device *pdev) > +{ > + struct tpm_info tpm_info = {}; > + struct resource *res; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (res == NULL) { 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
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" <martin.wilck@ts.fujitsu.com> > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > > --- > > drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++----------------- > > 1 file changed, 101 insertions(+), 60 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > > index 0a2d94f3d679..0e5c282aa37e 100644 > > --- a/drivers/char/tpm/tpm_tis.c > > +++ b/drivers/char/tpm/tpm_tis.c > > @@ -60,8 +60,6 @@ enum tis_int_flags { > > }; > > > > enum tis_defaults { > > - TIS_MEM_BASE = 0xFED40000, > > - TIS_MEM_LEN = 0x5000, > > TIS_SHORT_TIMEOUT = 750, /* ms */ > > TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > > }; > > @@ -72,12 +70,6 @@ struct tpm_info { > > int irq; > > }; > > > > -static struct tpm_info tis_default_info = { > > - .start = TIS_MEM_BASE, > > - .len = TIS_MEM_LEN, > > - .irq = 0, > > -}; > > - > > /* Some timeout values are needed before it is known whether the chip is > > * TPM 1.0 or TPM 2.0. > > */ > > @@ -847,7 +839,6 @@ out_err: > > return rc; > > } > > > > -#ifdef CONFIG_PM_SLEEP > > static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > > { > > u32 intmask; > > @@ -889,11 +880,9 @@ static int tpm_tis_resume(struct device *dev) > > > > return 0; > > } > > -#endif > > > > static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); > > > > -#ifdef CONFIG_PNP > > static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > > const struct pnp_device_id *pnp_id) > > { > > @@ -908,14 +897,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > > else > > tpm_info.irq = -1; > > > > -#ifdef CONFIG_ACPI > > if (pnp_acpi_device(pnp_dev)) { > > if (is_itpm(pnp_acpi_device(pnp_dev))) > > itpm = true; > > > > - acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle; > > + acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev); > > } > > -#endif > > > > return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle); > > } > > @@ -956,7 +943,6 @@ static struct pnp_driver tis_pnp_driver = { > > module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id, > > sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444); > > MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe"); > > -#endif > > > > #ifdef CONFIG_ACPI > > static int tpm_check_resource(struct acpi_resource *ares, void *data) > > @@ -1029,80 +1015,135 @@ static struct acpi_driver tis_acpi_driver = { > > }; > > #endif > > > > +static struct platform_device *force_pdev; > > + > > +static int tpm_tis_plat_probe(struct platform_device *pdev) > > +{ > > + struct tpm_info tpm_info = {}; > > + struct resource *res; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (res == NULL) { > > 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 ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
On Mo, 2015-11-30 at 12:27 -0700, Jason Gunthorpe wrote: > The TPM core has long assumed that every device has a driver attached, > however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are > called unconditionally") breaks that assumption. > > Rework the TPM setup to create a platform device with resources and > then allow the driver core to naturally bind and probe it through the > normal mechanisms. All this structure is needed anyhow to enable TPM > for OF environments. > > Finally, since the entire flow is changing convert the init/exit to use > the modern ifdef-less coding style when possible > > Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> I tested this on my system, deliberately reverting my own fix for platform_driver_probe() beforehand. It works, no panic any more. The patch introduces one user-visible change, because now the ACPI and PnP drivers are registered even with "force=1". This causes my TPM to be show up twice in sysfs: /sys/bus/acpi/drivers/tpm_tis/MSFT0101:00 -> ../../../../devices/LNXSYSTM:00/LNXSYBUS:00/MSFT0101:00 /sys/bus/platform/drivers/tpm_tis/tpm_tis -> ../../../../devices/platform/tpm_tis Only the platform device is actually bound to the physical device, though. I'm not sure if I like this. Regards Martin ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
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 ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
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 ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
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 ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
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 ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
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);
The TPM core has long assumed that every device has a driver attached, however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally") breaks that assumption. Rework the TPM setup to create a platform device with resources and then allow the driver core to naturally bind and probe it through the normal mechanisms. All this structure is needed anyhow to enable TPM for OF environments. Finally, since the entire flow is changing convert the init/exit to use the modern ifdef-less coding style when possible Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++----------------- 1 file changed, 101 insertions(+), 60 deletions(-)