* [PATCH 1/3] driver: fix device remove order @ 2015-03-17 6:25 Sascha Hauer 2015-03-17 6:25 ` [PATCH 2/3] driver: Call remove function only when available Sascha Hauer 2015-03-17 6:25 ` [PATCH 3/3] driver: Call bus->remove instead of driver->remove Sascha Hauer 0 siblings, 2 replies; 6+ messages in thread From: Sascha Hauer @ 2015-03-17 6:25 UTC (permalink / raw) To: Barebox List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] driver: Call remove function only when available 2015-03-17 6:25 [PATCH 1/3] driver: fix device remove order Sascha Hauer @ 2015-03-17 6:25 ` Sascha Hauer 2015-03-17 6:25 ` [PATCH 3/3] driver: Call bus->remove instead of driver->remove Sascha Hauer 1 sibling, 0 replies; 6+ messages in thread From: Sascha Hauer @ 2015-03-17 6:25 UTC (permalink / raw) To: Barebox List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] driver: Call bus->remove instead of driver->remove 2015-03-17 6:25 [PATCH 1/3] driver: fix device remove order Sascha Hauer 2015-03-17 6:25 ` [PATCH 2/3] driver: Call remove function only when available Sascha Hauer @ 2015-03-17 6:25 ` Sascha Hauer 2015-05-02 16:50 ` Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove) Ezequiel Garcia 1 sibling, 1 reply; 6+ messages in thread From: Sascha Hauer @ 2015-03-17 6:25 UTC (permalink / raw) To: Barebox List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove) 2015-03-17 6:25 ` [PATCH 3/3] driver: Call bus->remove instead of driver->remove Sascha Hauer @ 2015-05-02 16:50 ` Ezequiel Garcia 2015-05-04 6:29 ` Sascha Hauer 0 siblings, 1 reply; 6+ messages in thread From: Ezequiel Garcia @ 2015-05-02 16:50 UTC (permalink / raw) To: Sascha Hauer, Barebox List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove) 2015-05-02 16:50 ` Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove) Ezequiel Garcia @ 2015-05-04 6:29 ` Sascha Hauer 2015-05-05 11:44 ` Ezequiel Garcia 0 siblings, 1 reply; 6+ messages in thread From: Sascha Hauer @ 2015-05-04 6:29 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: Barebox List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove) 2015-05-04 6:29 ` Sascha Hauer @ 2015-05-05 11:44 ` Ezequiel Garcia 0 siblings, 0 replies; 6+ messages in thread From: Ezequiel Garcia @ 2015-05-05 11:44 UTC (permalink / raw) To: Sascha Hauer; +Cc: Barebox List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-05-05 11:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-17 6:25 [PATCH 1/3] driver: fix device remove order Sascha Hauer 2015-03-17 6:25 ` [PATCH 2/3] driver: Call remove function only when available Sascha Hauer 2015-03-17 6:25 ` [PATCH 3/3] driver: Call bus->remove instead of driver->remove Sascha Hauer 2015-05-02 16:50 ` Regression on next (Re: [PATCH 3/3] driver: Call bus->remove instead of driver->remove) Ezequiel Garcia 2015-05-04 6:29 ` Sascha Hauer 2015-05-05 11:44 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox