[PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[Uwe Kleine-König]

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(-)

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);

base-commit: 09162bc32c880a791c6c0668ce0745cf7958f576
prerequisite-patch-id: 81a6cc3e9e110aa4427c029e2f950f83a91c9a22
prerequisite-patch-id: 9c80bf49810461fd0e1a35b0a134675bc4333678
prerequisite-patch-id: bfb025de56c9e52ea068686a64f5f5ed6b29ab4f
prerequisite-patch-id: b490ddb3c51a92f058e252faf89c4cadd4a0e415
prerequisite-patch-id: 9c94ab8c9f66d0330549d1ecdc18e6ac97f8a7e4
-- 
2.28.0

[PATCH 2/3] spi: Use bus_type functions for probe, remove and shutdown

[Uwe Kleine-König]

The eventual goal is to get rid of the callbacks in struct
device_driver. Other than not using driver callbacks there should be no
side effect of this patch.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5bdc66f08ee1..e8c0a000ee19 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -374,16 +374,7 @@ static int spi_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return add_uevent_var(env, "MODALIAS=%s%s", SPI_MODULE_PREFIX, spi->modalias);
 }
 
-struct bus_type spi_bus_type = {
-	.name		= "spi",
-	.dev_groups	= spi_dev_groups,
-	.match		= spi_match_device,
-	.uevent		= spi_uevent,
-};
-EXPORT_SYMBOL_GPL(spi_bus_type);
-
-
-static int spi_drv_probe(struct device *dev)
+static int spi_probe(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
 	struct spi_device		*spi = to_spi_device(dev);
@@ -414,7 +405,7 @@ static int spi_drv_probe(struct device *dev)
 	return ret;
 }
 
-static int spi_drv_remove(struct device *dev)
+static int spi_remove(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
 	int ret = 0;
@@ -426,13 +417,25 @@ static int spi_drv_remove(struct device *dev)
 	return ret;
 }
 
-static void spi_drv_shutdown(struct device *dev)
+static void spi_shutdown(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
 
-	sdrv->shutdown(to_spi_device(dev));
+	if (sdrv->shutdown)
+		sdrv->shutdown(to_spi_device(dev));
 }
 
+struct bus_type spi_bus_type = {
+	.name		= "spi",
+	.dev_groups	= spi_dev_groups,
+	.match		= spi_match_device,
+	.uevent		= spi_uevent,
+	.probe		= spi_probe,
+	.remove		= spi_remove,
+	.shutdown	= spi_shutdown,
+};
+EXPORT_SYMBOL_GPL(spi_bus_type);
+
 /**
  * __spi_register_driver - register a SPI driver
  * @owner: owner module of the driver to register
@@ -445,12 +448,6 @@ 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;
-	}
-	if (sdrv->shutdown)
-		sdrv->driver.shutdown = spi_drv_shutdown;
 	return driver_register(&sdrv->driver);
 }
 EXPORT_SYMBOL_GPL(__spi_register_driver);
-- 
2.28.0

[PATCH 3/3] spi: Warn when a driver's remove callback returns an error

[Uwe Kleine-König]

The driver core ignores the return value of struct device_driver::remove
(because in general there is nothing that can be done about that). So
add a warning when an spi driver returns an error.

This simplifies the quest to make struct device_driver::remove return void.
A consequent change would be to make struct spi_driver::remove return void,
but I'm keeping this quest for later (or someone else).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e8c0a000ee19..6b7c19bf7715 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -408,13 +408,20 @@ static int spi_probe(struct device *dev)
 static int spi_remove(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
-	int ret = 0;
 
-	if (sdrv->remove)
+	if (sdrv->remove) {
+		int ret;
+
 		ret = sdrv->remove(to_spi_device(dev));
+		if (ret)
+			dev_warn(dev,
+				 "Failed to unbind driver (%pe), ignoring\n",
+				 ERR_PTR(ret));
+	}
+
 	dev_pm_domain_detach(dev, true);
 
-	return ret;
+	return 0;
 }
 
 static void spi_shutdown(struct device *dev)
-- 
2.28.0

Re: [PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: fix resource leak for drivers without .remove callback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]