[PATCH 1/3] driver: fix device remove order

[PATCH 1/3] driver: fix device remove order

[Sascha Hauer]

The active list is supposed to collect active devices in the
opposite order they are probed. This is used to remove the
devices in the correct order in devices_shutdown. The order
is wrong though when in a drivers probe function other devices
are registered. To get the order right we have to add the new
device to the active list before it is probed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/base/driver.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 3363b56..453966b 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -85,14 +85,15 @@ int device_probe(struct device_d *dev)
 
 	pinctrl_select_state_default(dev);
 
+	list_add(&dev->active, &active);
+
 	ret = dev->bus->probe(dev);
 	if (ret) {
+		list_del(&dev->active);
 		dev_err(dev, "probe failed: %s\n", strerror(-ret));
 		return ret;
 	}
 
-	list_add(&dev->active, &active);
-
 	return 0;
 }
 
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] driver: Call remove function only when available

[Sascha Hauer]

The bus implementations currently call the drivers remove
hook unconditionally, but this hook is seldomly populated. Only call
it when it's actually populated.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/efi/efi/efi-device.c | 3 ++-
 drivers/base/platform.c   | 3 ++-
 drivers/i2c/i2c.c         | 3 ++-
 drivers/pci/bus.c         | 3 ++-
 drivers/spi/spi.c         | 3 ++-
 drivers/w1/w1.c           | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/efi/efi/efi-device.c b/arch/efi/efi/efi-device.c
index 788bb71..7db8e48 100644
--- a/arch/efi/efi/efi-device.c
+++ b/arch/efi/efi/efi-device.c
@@ -328,7 +328,8 @@ static void efi_bus_remove(struct device_d *dev)
 	struct efi_driver *efidrv = to_efi_driver(dev->driver);
 	struct efi_device *efidev = to_efi_device(dev);
 
-	return efidrv->remove(efidev);
+	if (efidrv->remove)
+		efidrv->remove(efidev);
 }
 
 struct bus_type efi_bus = {
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e053ec7..85bdfb0 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -29,7 +29,8 @@ static int platform_probe(struct device_d *dev)
 
 static void platform_remove(struct device_d *dev)
 {
-	dev->driver->remove(dev);
+	if (dev->driver->remove)
+		dev->driver->remove(dev);
 }
 
 int platform_driver_register(struct driver_d *drv)
diff --git a/drivers/i2c/i2c.c b/drivers/i2c/i2c.c
index 9873957..7a6bde0 100644
--- a/drivers/i2c/i2c.c
+++ b/drivers/i2c/i2c.c
@@ -472,7 +472,8 @@ static int i2c_probe(struct device_d *dev)
 
 static void i2c_remove(struct device_d *dev)
 {
-	dev->driver->remove(dev);
+	if (dev->driver->remove)
+		dev->driver->remove(dev);
 }
 
 struct bus_type i2c_bus = {
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 866ab08..d6c5496 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -51,7 +51,8 @@ static void pci_remove(struct device_d *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pci_driver *pdrv = to_pci_driver(dev->driver);
 
-	pdrv->remove(pdev);
+	if (pdrv->remove)
+		pdrv->remove(pdev);
 }
 
 struct bus_type pci_bus = {
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8bddd98..ba23cf7 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -311,7 +311,8 @@ static int spi_probe(struct device_d *dev)
 
 static void spi_remove(struct device_d *dev)
 {
-	dev->driver->remove(dev);
+	if (dev->driver->remove)
+		dev->driver->remove(dev);
 }
 
 struct bus_type spi_bus = {
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index bbef470..ff57386 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -392,7 +392,8 @@ static void w1_bus_remove(struct device_d *_dev)
 	struct w1_driver *drv = to_w1_driver(_dev->driver);
 	struct w1_device *dev = to_w1_device(_dev);
 
-	return drv->remove(dev);
+	if (drv->remove)
+		drv->remove(dev);
 }
 
 struct bus_type w1_bustype= {
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] driver: Call bus->remove instead of driver->remove

[Sascha Hauer]

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@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);
 	}
 }
 
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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@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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

[Sascha Hauer]

Hi Ezequiel,

On Sat, May 02, 2015 at 01:50:42PM -0300, Ezequiel Garcia wrote:
> 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@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.

Too bad that happened. I believe the following fixes this, but I have no
hardware to test this on. Could you give this a test? I'll delay the
outstanding release until this is solved.

Sascha

----------------------------8<---------------------------

From 15422e3435b0151f1b5d753b19a8578210da9d04 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 4 May 2015 08:16:20 +0200
Subject: [PATCH] net: phy: Do not double remove phy device

This fixes: 80264a8 driver: Call bus->remove instead of driver->remove

On mvebu it happens that:

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)

Fix this by setting the mdio busses phy_map[phy->addr] to NULL when
unregistering the phy device, so that mdiobus_unregister no longer
finds a valid phy_device when iterating over the busses device list.

Reported-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/phy/mdio_bus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index f526cfc..0959c45 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -331,12 +331,14 @@ static void mdio_bus_remove(struct device_d *_dev)
 {
 	struct phy_device *dev = to_phy_device(_dev);
 	struct phy_driver *drv = to_phy_driver(_dev->driver);
+	struct mii_bus *bus = dev->bus;
 
 	if (drv->remove)
 		drv->remove(dev);
 
 	free(dev->cdev.name);
 	devfs_remove(&dev->cdev);
+	bus->phy_map[dev->addr] = NULL;
 }
 
 struct bus_type mdio_bus_type = {
-- 
2.1.4

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

[Ezequiel Garcia]

On 05/04/2015 03:29 AM, Sascha Hauer wrote:
> Hi Ezequiel,
> 
> On Sat, May 02, 2015 at 01:50:42PM -0300, Ezequiel Garcia wrote:
>> 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@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.
> 
> Too bad that happened. I believe the following fixes this, but I have no
> hardware to test this on. Could you give this a test? I'll delay the
> outstanding release until this is solved.
> 
> Sascha
> 
> ----------------------------8<---------------------------
> 
> From 15422e3435b0151f1b5d753b19a8578210da9d04 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 4 May 2015 08:16:20 +0200
> Subject: [PATCH] net: phy: Do not double remove phy device
> 
> This fixes: 80264a8 driver: Call bus->remove instead of driver->remove
> 
> On mvebu it happens that:
> 
> 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)
> 
> Fix this by setting the mdio busses phy_map[phy->addr] to NULL when
> unregistering the phy device, so that mdiobus_unregister no longer
> finds a valid phy_device when iterating over the busses device list.
> 
> Reported-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Thanks for the quick fix,
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox