Messages in this thread Patch in this message | | | Date | Fri, 27 Nov 2015 11:11:25 +0100 | From | Uwe Kleine-König <> | Subject | Re: [PATCH] base/platform: fix panic when probe function is NULL |
| |
Hello Martin,
On Thu, Nov 26, 2015 at 08:01:34PM +0100, martin.wilck@ts.fujitsu.com wrote: > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > Since b8b2c7d845d5, platform_drv_probe() is called for all platform > devices. If drv->probe is NULL, and dev_pm_domain_attach() fails, > platform_drv_probe() will return the error code from dev_pm_domain_attach().
Correct, this is an unintended change of behaviour introduced in b8b2c7d845d5.
> 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 are missing. > > This may cause a panic later. For example, inserting the tpm_tis > driver with parameter "force=1" (i.e. registering tpm_tis as a platform > driver) will panic in tpmm_chip_alloc() because dev->driver is NULL: > > chip->cdev.owner = chip->pdev->driver->owner;
This sounds like a separate issue though. Looking at init_tis there is:
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);
tpm_tis_init calls tpmm_chip_alloc which barfs when pdev (i.e. the return value of platform_device_register_simple above) isn't bound. It is not allowed to assume that the device is bound after the above function calls.
So I'd say drop the paragraph about tpm_tis and the change is fine.
> This patch fixes this by returning success in platform_drv_probe() if > "just" dev_pm_domain_attach() had failed. This restores the semantics > of platform_device_register_XXX() if the associated platform driver has > no "probe" function. > > Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain > callbacks are called unconditionally") >
I think line breaks in the Fixes: line are frowned on. Also usually there is no empty line between Fixes: and S-o-b:.
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > --- > drivers/base/platform.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 1dd6d3b..c994e76 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -513,10 +513,14 @@ 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;
An else that has a } should also have a {, according to checkpatch and Documentation/CodingStyle. You can write it alternatively as:
if (ret != -EPROBE_DEFER) { if (drv->probe) ret = drv->probe(dev); else ret = 0;
if (ret) dev_pm_domain_detach(_dev, true); }
.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
| |