Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove)

[Ezequiel Garcia]

Hi Sascha,

On 03/17/2015 03:25 AM, Sascha Hauer wrote:
> In devices_shutdown we should call the busses remove function
> which in turn calls the drivers remove function. Otherwise for
> example PCI devices never get removed since they do not have
> a remove function but a pcidev->remove function instead.
> 
> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> ---
>  drivers/base/driver.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index 453966b..590c97c 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -399,8 +399,8 @@ void devices_shutdown(void)
>  	struct device_d *dev;
>  
>  	list_for_each_entry(dev, &active, active) {
> -		if (dev->driver->remove)
> -			dev->driver->remove(dev);
> +		if (dev->bus->remove)
> +			dev->bus->remove(dev);
>  	}
>  }
>  
> 

This commit caused a regression on the Openblocks A6 board, which now freezes
when calling devices_shutdown. The problem is that calling bus->remove makes
the remove() of the PHY driver called twice.

This happened because the mdio bus device (mdio-mvebu) was registered first,
and the phy0 device was be registered later, and attached to the mdio bus.

Upon device shutdown, when iterating through the active device list,
the phy0 device is removed before mdio-mvebu. Then, when the mdio bus device
is removed, the phy0 device is removed again, here:

mdio_bus_remove(on mdio-mvebu)
  mvebu_mdio_remove
    mdiobus_unregister
      unregister_device
        mdio_bus_remove(on phy0)
        
This is currently the case for the Kirkwood Openblocks A6 board, where a double
free(dev->cdev.name) in mdio_bus_remove causes a silent freeze when booting
Linux.

I tried something like this:

@@ -446,12 +450,14 @@ const char *dev_id(const struct device_d *dev)
  void devices_shutdown(void)
 {
-	struct device_d *dev;
+	struct device_d *dev, *tmpdev;
+	struct bus_type *bus;
+
+	for_each_bus(bus)
+		list_for_each_entry_safe(dev, tmpdev, &(bus)->device_list, bus_list)
+			if (dev->is_active && dev->bus->remove)
+				dev->bus->remove(dev);
 -	list_for_each_entry(dev, &active, active) {
-		if (dev->bus->remove)
-			dev->bus->remove(dev);
-	}
 }

But then realise this messes the remove order, and so will probably
break some other case :(

--
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar