[PATCH 0/2] driver core: Refactor device unbind/probe fail clean-ups

[PATCH 0/2] driver core: Refactor device unbind/probe fail clean-ups

[Rob Herring]

The same sequences of device clean-up code are duplicated 3 times.
Changes to this code are prone to missing one of the copies. This
series refactors the code into 2 common functions as there's some slight
differences in the intermediate steps.

Rob

Rob Herring (2):
  driver core: Refactor multiple copies of device cleanup
  driver core: Refactor sysfs and drv/bus remove hooks

 drivers/base/dd.c | 80 ++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 50 deletions(-)

-- 
2.32.0

[PATCH 1/2] driver core: Refactor multiple copies of device cleanup

[Rob Herring]

There are 3 copies of the same device cleanup code used for probe failure,
testing re-probing, and device unbinding. Changes to this code often miss
at least one of the copies of the code. See commits d0243bbd5dd3 ("drivers
core: Free dma_range_map when driver probe failed") and d8f7a5484f21
("driver core: Free DMA range map when device is released") for example.

Let's refactor the code to its own function.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/base/dd.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f47cab21430f..19bae32955c1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -506,6 +506,19 @@ static ssize_t state_synced_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(state_synced);
 
+static void device_unbind_cleanup(struct device *dev)
+{
+	devres_release_all(dev);
+	arch_teardown_dma_ops(dev);
+	kfree(dev->dma_range_map);
+	dev->dma_range_map = NULL;
+	dev->driver = NULL;
+	dev_set_drvdata(dev, NULL);
+	if (dev->pm_domain && dev->pm_domain->dismiss)
+		dev->pm_domain->dismiss(dev);
+	pm_runtime_reinit(dev);
+	dev_pm_set_driver_flags(dev, 0);
+}
 
 static int call_driver_probe(struct device *dev, struct device_driver *drv)
 {
@@ -628,16 +641,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 		else if (drv->remove)
 			drv->remove(dev);
 
-		devres_release_all(dev);
-		arch_teardown_dma_ops(dev);
-		kfree(dev->dma_range_map);
-		dev->dma_range_map = NULL;
 		driver_sysfs_remove(dev);
-		dev->driver = NULL;
-		dev_set_drvdata(dev, NULL);
-		if (dev->pm_domain && dev->pm_domain->dismiss)
-			dev->pm_domain->dismiss(dev);
-		pm_runtime_reinit(dev);
+		device_unbind_cleanup(dev);
 
 		goto re_probe;
 	}
@@ -667,16 +672,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 					     BUS_NOTIFY_DRIVER_NOT_BOUND, dev);
 pinctrl_bind_failed:
 	device_links_no_driver(dev);
-	devres_release_all(dev);
-	arch_teardown_dma_ops(dev);
-	kfree(dev->dma_range_map);
-	dev->dma_range_map = NULL;
-	dev->driver = NULL;
-	dev_set_drvdata(dev, NULL);
-	if (dev->pm_domain && dev->pm_domain->dismiss)
-		dev->pm_domain->dismiss(dev);
-	pm_runtime_reinit(dev);
-	dev_pm_set_driver_flags(dev, 0);
+	device_unbind_cleanup(dev);
 done:
 	return ret;
 }
@@ -1209,17 +1205,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 			drv->remove(dev);
 
 		device_links_driver_cleanup(dev);
-
-		devres_release_all(dev);
-		arch_teardown_dma_ops(dev);
-		kfree(dev->dma_range_map);
-		dev->dma_range_map = NULL;
-		dev->driver = NULL;
-		dev_set_drvdata(dev, NULL);
-		if (dev->pm_domain && dev->pm_domain->dismiss)
-			dev->pm_domain->dismiss(dev);
-		pm_runtime_reinit(dev);
-		dev_pm_set_driver_flags(dev, 0);
+		device_unbind_cleanup(dev);
 
 		klist_remove(&dev->p->knode_driver);
 		device_pm_check_callbacks(dev);
-- 
2.32.0

[PATCH 2/2] driver core: Refactor sysfs and drv/bus remove hooks

[Rob Herring]

There are 3 copies of the same device sysfs cleanup and drv/bus remove()
hooks used for probe failure, testing re-probing, and device unbinding.

Let's refactor the code to its own function.

Signed-off-by: Rob Herring <robh@kernel.org>
---
Note that driver_sysfs_remove() remains a separate call because in
__device_release_driver() it is called with a pm runtime get/put and
before the unbind notifier. Browsing the history, it doesn't seem like
there's any particular reason for this ordering. We also have other
sysfs teardown done later. It would be more logical to do all the
sysfs teardown at once. Thoughts?
---
 drivers/base/dd.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 19bae32955c1..e7ede3fb1774 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -520,6 +520,17 @@ static void device_unbind_cleanup(struct device *dev)
 	dev_pm_set_driver_flags(dev, 0);
 }
 
+static void device_remove(struct device *dev)
+{
+	device_remove_file(dev, &dev_attr_state_synced);
+	device_remove_groups(dev, dev->driver->dev_groups);
+
+	if (dev->bus && dev->bus->remove)
+		dev->bus->remove(dev);
+	else if (dev->driver->remove)
+		dev->driver->remove(dev);
+}
+
 static int call_driver_probe(struct device *dev, struct device_driver *drv)
 {
 	int ret = 0;
@@ -633,14 +644,7 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (test_remove) {
 		test_remove = false;
 
-		device_remove_file(dev, &dev_attr_state_synced);
-		device_remove_groups(dev, drv->dev_groups);
-
-		if (dev->bus->remove)
-			dev->bus->remove(dev);
-		else if (drv->remove)
-			drv->remove(dev);
-
+		device_remove(dev);
 		driver_sysfs_remove(dev);
 		device_unbind_cleanup(dev);
 
@@ -658,12 +662,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	goto done;
 
 dev_sysfs_state_synced_failed:
-	device_remove_groups(dev, drv->dev_groups);
 dev_groups_failed:
-	if (dev->bus->remove)
-		dev->bus->remove(dev);
-	else if (drv->remove)
-		drv->remove(dev);
+	device_remove(dev);
 probe_failed:
 	driver_sysfs_remove(dev);
 sysfs_failed:
@@ -1196,13 +1196,7 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 
 		pm_runtime_put_sync(dev);
 
-		device_remove_file(dev, &dev_attr_state_synced);
-		device_remove_groups(dev, drv->dev_groups);
-
-		if (dev->bus && dev->bus->remove)
-			dev->bus->remove(dev);
-		else if (drv->remove)
-			drv->remove(dev);
+		device_remove(dev);
 
 		device_links_driver_cleanup(dev);
 		device_unbind_cleanup(dev);
-- 
2.32.0

Re: [PATCH 2/2] driver core: Refactor sysfs and drv/bus remove hooks

[Greg Kroah-Hartman]

On Wed, Feb 23, 2022 at 04:52:57PM -0600, Rob Herring wrote:
> There are 3 copies of the same device sysfs cleanup and drv/bus remove()
> hooks used for probe failure, testing re-probing, and device unbinding.
> 
> Let's refactor the code to its own function.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Note that driver_sysfs_remove() remains a separate call because in
> __device_release_driver() it is called with a pm runtime get/put and
> before the unbind notifier. Browsing the history, it doesn't seem like
> there's any particular reason for this ordering. We also have other
> sysfs teardown done later. It would be more logical to do all the
> sysfs teardown at once. Thoughts?

I think it should be unified, so you can send a follow-on patch to do
that.

I'll go queue these changes up now, thanks for the cleanups!

greg k-h

Re: [PATCH 0/2] driver core: Refactor device unbind/probe fail clean-ups

[Greg Kroah-Hartman]

On Wed, Feb 23, 2022 at 04:52:55PM -0600, Rob Herring wrote:
> The same sequences of device clean-up code are duplicated 3 times.
> Changes to this code are prone to missing one of the copies. This
> series refactors the code into 2 common functions as there's some slight
> differences in the intermediate steps.
> 
> Rob
> 
> Rob Herring (2):
>   driver core: Refactor multiple copies of device cleanup
>   driver core: Refactor sysfs and drv/bus remove hooks
> 
>  drivers/base/dd.c | 80 ++++++++++++++++++-----------------------------
>  1 file changed, 30 insertions(+), 50 deletions(-)
> 
> -- 
> 2.32.0
> 

What branch/tree did you make these against?  They do not apply at all
to my "driver-core-next" branch of the driver-core.git tree on
git.kernel.org :(

thanks,

greg k-h

Re: [PATCH 0/2] driver core: Refactor device unbind/probe fail clean-ups

[Rob Herring]

On Thu, Feb 24, 2022 at 3:09 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Feb 23, 2022 at 04:52:55PM -0600, Rob Herring wrote:
> > The same sequences of device clean-up code are duplicated 3 times.
> > Changes to this code are prone to missing one of the copies. This
> > series refactors the code into 2 common functions as there's some slight
> > differences in the intermediate steps.
> >
> > Rob
> >
> > Rob Herring (2):
> >   driver core: Refactor multiple copies of device cleanup
> >   driver core: Refactor sysfs and drv/bus remove hooks
> >
> >  drivers/base/dd.c | 80 ++++++++++++++++++-----------------------------
> >  1 file changed, 30 insertions(+), 50 deletions(-)
> >
> > --
> > 2.32.0
> >
>
> What branch/tree did you make these against?  They do not apply at all
> to my "driver-core-next" branch of the driver-core.git tree on
> git.kernel.org :(

linux-next. It is dependent on commit d8f7a5484f21 ("driver core: Free
DMA range map when device is released") in your driver-core-linus
branch.

Rob

Re: [PATCH 0/2] driver core: Refactor device unbind/probe fail clean-ups

[Greg Kroah-Hartman]

On Thu, Feb 24, 2022 at 07:24:48AM -0600, Rob Herring wrote:
> On Thu, Feb 24, 2022 at 3:09 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Feb 23, 2022 at 04:52:55PM -0600, Rob Herring wrote:
> > > The same sequences of device clean-up code are duplicated 3 times.
> > > Changes to this code are prone to missing one of the copies. This
> > > series refactors the code into 2 common functions as there's some slight
> > > differences in the intermediate steps.
> > >
> > > Rob
> > >
> > > Rob Herring (2):
> > >   driver core: Refactor multiple copies of device cleanup
> > >   driver core: Refactor sysfs and drv/bus remove hooks
> > >
> > >  drivers/base/dd.c | 80 ++++++++++++++++++-----------------------------
> > >  1 file changed, 30 insertions(+), 50 deletions(-)
> > >
> > > --
> > > 2.32.0
> > >
> >
> > What branch/tree did you make these against?  They do not apply at all
> > to my "driver-core-next" branch of the driver-core.git tree on
> > git.kernel.org :(
> 
> linux-next. It is dependent on commit d8f7a5484f21 ("driver core: Free
> DMA range map when device is released") in your driver-core-linus
> branch.

Ah, my fault, I'll wait for the branch to be merged and then apply these
next week.

thanks,

greg k-h