| Message ID | 20201119152059.2631650-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] spi: fix resource leak for drivers without .remove callback | expand |
On Thu, Nov 19, 2020 at 04:20:57PM +0100, Uwe Kleine-König wrote: > Consider an spi driver with a .probe but without a .remove callback (e.g. > rtc-ds1347). The function spi_drv_probe() is called to bind a device and > so some init routines like dev_pm_domain_attach() are used. As there is > no remove callback spi_drv_remove() isn't called at unbind time however > and so calling dev_pm_domain_detach() is missed and the pm domain keeps > active. > To fix this always use either both or none of the functions and make > them handle the callback not being set. Why would we want to tie configuring PM domains to either of these functions? We certainly don't want to force drivers to have empty remove functions to trigger cleanup of domains, this would be counterintuitive and this stuff should be transparent to the driver.
On Thu, Nov 19, 2020 at 03:24:16PM +0000, Mark Brown wrote: > On Thu, Nov 19, 2020 at 04:20:57PM +0100, Uwe Kleine-König wrote: > > Consider an spi driver with a .probe but without a .remove callback (e.g. > > rtc-ds1347). The function spi_drv_probe() is called to bind a device and > > so some init routines like dev_pm_domain_attach() are used. As there is > > no remove callback spi_drv_remove() isn't called at unbind time however > > and so calling dev_pm_domain_detach() is missed and the pm domain keeps > > active. > > > To fix this always use either both or none of the functions and make > > them handle the callback not being set. > > Why would we want to tie configuring PM domains to either of these > functions? We certainly don't want to force drivers to have empty > remove functions to trigger cleanup of domains, this would be > counterintuitive and this stuff should be transparent to the driver. Yes, I thought that this is not the final fix. I just sent the minimal change to prevent the imbalance. So if I understand correctly, I will have to respin with the following squashed into patch 1: diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 5bdc66f08ee1..5becf6c2c409 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -445,10 +445,8 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv) { sdrv->driver.owner = owner; sdrv->driver.bus = &spi_bus_type; - if (sdrv->probe || sdrv->remove) { - sdrv->driver.probe = spi_drv_probe; - sdrv->driver.remove = spi_drv_remove; - } + sdrv->driver.probe = spi_drv_probe; + sdrv->driver.remove = spi_drv_remove; if (sdrv->shutdown) sdrv->driver.shutdown = spi_drv_shutdown; return driver_register(&sdrv->driver); (Not sure this makes a difference in real life, are there drivers without a .probe callback?) Best regards Uwe
On Thu, Nov 19, 2020 at 04:35:40PM +0100, Uwe Kleine-König wrote: > Yes, I thought that this is not the final fix. I just sent the minimal > change to prevent the imbalance. So if I understand correctly, I will > have to respin with the following squashed into patch 1: > - if (sdrv->probe || sdrv->remove) { > - sdrv->driver.probe = spi_drv_probe; > - sdrv->driver.remove = spi_drv_remove; > - } > + sdrv->driver.probe = spi_drv_probe; > + sdrv->driver.remove = spi_drv_remove; > if (sdrv->shutdown) > sdrv->driver.shutdown = spi_drv_shutdown; > return driver_register(&sdrv->driver); I think so, I'd need to see the full patch to check of course. > (Not sure this makes a difference in real life, are there drivers > without a .probe callback?) Your changelog seemed to say that it would make remove mandatory.
Hello Mark, On Thu, Nov 19, 2020 at 03:41:39PM +0000, Mark Brown wrote: > On Thu, Nov 19, 2020 at 04:35:40PM +0100, Uwe Kleine-König wrote: > > > Yes, I thought that this is not the final fix. I just sent the minimal > > change to prevent the imbalance. So if I understand correctly, I will > > have to respin with the following squashed into patch 1: > > > - if (sdrv->probe || sdrv->remove) { > > - sdrv->driver.probe = spi_drv_probe; > > - sdrv->driver.remove = spi_drv_remove; > > - } > > + sdrv->driver.probe = spi_drv_probe; > > + sdrv->driver.remove = spi_drv_remove; > > if (sdrv->shutdown) > > sdrv->driver.shutdown = spi_drv_shutdown; > > return driver_register(&sdrv->driver); > > I think so, I'd need to see the full patch to check of course. ok. > > (Not sure this makes a difference in real life, are there drivers > > without a .probe callback?) > > Your changelog seemed to say that it would make remove mandatory. No, that's not what the patch did. It made unconditional use of spi_drv_remove(), but an spi_driver without .remove() was still ok. I will reword to make this clearer. Best regards Uwe
On Thu, Nov 19, 2020 at 05:04:12PM +0100, Uwe Kleine-König wrote: > On Thu, Nov 19, 2020 at 03:41:39PM +0000, Mark Brown wrote: > > On Thu, Nov 19, 2020 at 04:35:40PM +0100, Uwe Kleine-König wrote: > > > (Not sure this makes a difference in real life, are there drivers > > > without a .probe callback?) > > Your changelog seemed to say that it would make remove mandatory. > No, that's not what the patch did. It made unconditional use of > spi_drv_remove(), but an spi_driver without .remove() was still ok. I > will reword to make this clearer. Ah, OK - I hadn't read the patch closely as the description sounded wrong.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 0cab239d8e7f..5bdc66f08ee1 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -405,9 +405,11 @@ static int spi_drv_probe(struct device *dev) if (ret) return ret; - ret = sdrv->probe(spi); - if (ret) - dev_pm_domain_detach(dev, true); + if (sdrv->probe) { + ret = sdrv->probe(spi); + if (ret) + dev_pm_domain_detach(dev, true); + } return ret; } @@ -415,9 +417,10 @@ static int spi_drv_probe(struct device *dev) static int spi_drv_remove(struct device *dev) { const struct spi_driver *sdrv = to_spi_driver(dev->driver); - int ret; + int ret = 0; - ret = sdrv->remove(to_spi_device(dev)); + if (sdrv->remove) + ret = sdrv->remove(to_spi_device(dev)); dev_pm_domain_detach(dev, true); return ret; @@ -442,10 +445,10 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv) { sdrv->driver.owner = owner; sdrv->driver.bus = &spi_bus_type; - if (sdrv->probe) + if (sdrv->probe || sdrv->remove) { sdrv->driver.probe = spi_drv_probe; - if (sdrv->remove) sdrv->driver.remove = spi_drv_remove; + } if (sdrv->shutdown) sdrv->driver.shutdown = spi_drv_shutdown; return driver_register(&sdrv->driver);
Consider an spi driver with a .probe but without a .remove callback (e.g. rtc-ds1347). The function spi_drv_probe() is called to bind a device and so some init routines like dev_pm_domain_attach() are used. As there is no remove callback spi_drv_remove() isn't called at unbind time however and so calling dev_pm_domain_detach() is missed and the pm domain keeps active. To fix this always use either both or none of the functions and make them handle the callback not being set. Fixes: 33cf00e57082 ("spi: attach/detach SPI device to the ACPI power domain") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/spi/spi.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) base-commit: 09162bc32c880a791c6c0668ce0745cf7958f576 prerequisite-patch-id: 81a6cc3e9e110aa4427c029e2f950f83a91c9a22 prerequisite-patch-id: 9c80bf49810461fd0e1a35b0a134675bc4333678 prerequisite-patch-id: bfb025de56c9e52ea068686a64f5f5ed6b29ab4f prerequisite-patch-id: b490ddb3c51a92f058e252faf89c4cadd4a0e415 prerequisite-patch-id: 9c94ab8c9f66d0330549d1ecdc18e6ac97f8a7e4