| Message ID | 1374257691-31981-1-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/19/2013 01:14 PM, Sebastian Andrzej Siewior wrote: > So I called of_platform_populate() on a device to get each child device > probed and on rmmod and I need to reverse its doing. After a quick grep > I did what others did as well and rmmod ended in: > > | Unable to handle kernel NULL pointer dereference at virtual address 00000018 > | PC is at release_resource+0x18/0x80 > | Process rmmod (pid: 2005, stack limit = 0xedc30238) > | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac) > | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18) > > The problem is that platform_device_del() "releases" each ressource in its > tree. This does not work on platform_devices created by OF becuase they > were never added via insert_resource(). As a consequence old->parent in > __release_resource() is NULL and we explode while accessing ->child. > So I either I do something completly wrong _or_ nobody here tested the > rmmod path of their driver. Wouldn't the correct fix be to call insert_resource somehow? The problem I have is that while of_platform_populate is all about parsing the DT and creating devices, the removal side has nothing to do with DT. So this should not be in the DT code. I think the core device code should be able to handle removal if the device creation side is done correctly. It looks to me like of_device_add either needs to call platform_device_add rather than device_add. I think the device name setting in platform_device_add should be a nop. If not, a check that the name is already set could be added. Rob > > This patch provides a common function to unregister / remove devices > which added to the system via of_platform_populate(). While this works > now on my test case I have not tested any of the driver I modify here so > feedback is greatly appreciated. > > Cc: Tony Lindgren <tony@atomide.com> > Cc: Doug Anderson <dianders@chromium.org> > Cc: Vivek Gautam <gautam.vivek@samsung.com> > Cc: Naveen Krishna Chatradhi <ch.naveen@samsung.com> > Cc: Kukjin Kim <kgene.kim@samsung.com> > Cc: Kishon Vijay Abraham I <kishon@ti.com> > Cc: Roger Quadros <rogerq@ti.com> > Cc: George Cherian <george.cherian@ti.com> > Cc: Felipe Balbi <balbi@ti.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/bus/omap-ocp2scp.c | 13 ++----------- > drivers/iio/adc/exynos_adc.c | 15 ++------------- > drivers/mfd/omap-usb-host.c | 9 +-------- > drivers/of/platform.c | 22 ++++++++++++++++++++++ > drivers/usb/dwc3/dwc3-exynos.c | 11 +---------- > drivers/usb/dwc3/dwc3-omap.c | 12 +----------- > include/linux/of_platform.h | 4 ++++ > 7 files changed, 33 insertions(+), 53 deletions(-) > > diff --git a/drivers/bus/omap-ocp2scp.c b/drivers/bus/omap-ocp2scp.c > index 5511f98..510bb9e 100644 > --- a/drivers/bus/omap-ocp2scp.c > +++ b/drivers/bus/omap-ocp2scp.c > @@ -23,15 +23,6 @@ > #include <linux/of.h> > #include <linux/of_platform.h> > > -static int ocp2scp_remove_devices(struct device *dev, void *c) > -{ > - struct platform_device *pdev = to_platform_device(dev); > - > - platform_device_unregister(pdev); > - > - return 0; > -} > - > static int omap_ocp2scp_probe(struct platform_device *pdev) > { > int ret; > @@ -51,7 +42,7 @@ static int omap_ocp2scp_probe(struct platform_device *pdev) > return 0; > > err0: > - device_for_each_child(&pdev->dev, NULL, ocp2scp_remove_devices); > + of_platform_unpopulate(&pdev->dev); > > return ret; > } > @@ -59,7 +50,7 @@ static int omap_ocp2scp_probe(struct platform_device *pdev) > static int omap_ocp2scp_remove(struct platform_device *pdev) > { > pm_runtime_disable(&pdev->dev); > - device_for_each_child(&pdev->dev, NULL, ocp2scp_remove_devices); > + of_platform_unpopulate(&pdev->dev); > > return 0; > } > diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c > index 9809fc9..10248e1 100644 > --- a/drivers/iio/adc/exynos_adc.c > +++ b/drivers/iio/adc/exynos_adc.c > @@ -216,15 +216,6 @@ static const struct iio_chan_spec exynos_adc_iio_channels[] = { > ADC_CHANNEL(9, "adc9"), > }; > > -static int exynos_adc_remove_devices(struct device *dev, void *c) > -{ > - struct platform_device *pdev = to_platform_device(dev); > - > - platform_device_unregister(pdev); > - > - return 0; > -} > - > static void exynos_adc_hw_init(struct exynos_adc *info) > { > u32 con1, con2; > @@ -357,8 +348,7 @@ static int exynos_adc_probe(struct platform_device *pdev) > return 0; > > err_of_populate: > - device_for_each_child(&pdev->dev, NULL, > - exynos_adc_remove_devices); > + of_platform_unpopulate(&pdev->dev); > regulator_disable(info->vdd); > clk_disable_unprepare(info->clk); > err_iio_dev: > @@ -375,8 +365,7 @@ static int exynos_adc_remove(struct platform_device *pdev) > struct iio_dev *indio_dev = platform_get_drvdata(pdev); > struct exynos_adc *info = iio_priv(indio_dev); > > - device_for_each_child(&pdev->dev, NULL, > - exynos_adc_remove_devices); > + of_platform_unpopulate(&pdev->dev); > regulator_disable(info->vdd); > clk_disable_unprepare(info->clk); > writel(0, info->enable_reg); > diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c > index 759fae3..bb26cea 100644 > --- a/drivers/mfd/omap-usb-host.c > +++ b/drivers/mfd/omap-usb-host.c > @@ -832,13 +832,6 @@ static int usbhs_omap_probe(struct platform_device *pdev) > return ret; > } > > -static int usbhs_omap_remove_child(struct device *dev, void *data) > -{ > - dev_info(dev, "unregistering\n"); > - platform_device_unregister(to_platform_device(dev)); > - return 0; > -} > - > /** > * usbhs_omap_remove - shutdown processing for UHH & TLL HCDs > * @pdev: USB Host Controller being removed > @@ -871,7 +864,7 @@ static int usbhs_omap_remove(struct platform_device *pdev) > pm_runtime_disable(&pdev->dev); > > /* remove children */ > - device_for_each_child(&pdev->dev, NULL, usbhs_omap_remove_child); > + of_platform_unpopulate(&pdev->dev); > return 0; > } > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index e0a6514..9cbb0c3 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -473,4 +473,26 @@ int of_platform_populate(struct device_node *root, > return rc; > } > EXPORT_SYMBOL_GPL(of_platform_populate); > + > +static int of_remove_populated_child(struct device *dev, void *d) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + > + of_device_unregister(pdev); > + return 0; > +} > +/** > + * of_platform_unpopulate() - Remove populated devices. > + * @parent: parent of the populated devices. > + * > + * The reverse of of_platform_populate() and can only be used a parent was > + * specified while invoking the former. > + */ > +void of_platform_unpopulate(struct device *parent) > +{ > + if (WARN_ON(!parent)) > + return; > + device_for_each_child(parent, NULL, of_remove_populated_child); > +} > +EXPORT_SYMBOL_GPL(of_platform_unpopulate); > #endif /* CONFIG_OF_ADDRESS */ > diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c > index 8ce9d7f..2bf5664 100644 > --- a/drivers/usb/dwc3/dwc3-exynos.c > +++ b/drivers/usb/dwc3/dwc3-exynos.c > @@ -86,15 +86,6 @@ static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos) > return ret; > } > > -static int dwc3_exynos_remove_child(struct device *dev, void *unused) > -{ > - struct platform_device *pdev = to_platform_device(dev); > - > - platform_device_unregister(pdev); > - > - return 0; > -} > - > static int dwc3_exynos_probe(struct platform_device *pdev) > { > struct dwc3_exynos *exynos; > @@ -164,7 +155,7 @@ static int dwc3_exynos_remove(struct platform_device *pdev) > { > struct dwc3_exynos *exynos = platform_get_drvdata(pdev); > > - device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child); > + of_platform_unpopulate(&pdev->dev); > platform_device_unregister(exynos->usb2_phy); > platform_device_unregister(exynos->usb3_phy); > > diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c > index 077f110..8688613 100644 > --- a/drivers/usb/dwc3/dwc3-omap.c > +++ b/drivers/usb/dwc3/dwc3-omap.c > @@ -327,15 +327,6 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) > return IRQ_HANDLED; > } > > -static int dwc3_omap_remove_core(struct device *dev, void *c) > -{ > - struct platform_device *pdev = to_platform_device(dev); > - > - platform_device_unregister(pdev); > - > - return 0; > -} > - > static void dwc3_omap_enable_irqs(struct dwc3_omap *omap) > { > u32 reg; > @@ -529,8 +520,7 @@ static int dwc3_omap_remove(struct platform_device *pdev) > dwc3_omap_disable_irqs(omap); > pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > - device_for_each_child(&pdev->dev, NULL, dwc3_omap_remove_core); > - > + of_platform_unpopulate(&pdev->dev); > return 0; > } > > diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h > index 05cb4a9..e354f9c 100644 > --- a/include/linux/of_platform.h > +++ b/include/linux/of_platform.h > @@ -72,6 +72,7 @@ extern int of_platform_populate(struct device_node *root, > const struct of_device_id *matches, > const struct of_dev_auxdata *lookup, > struct device *parent); > +extern void of_platform_unpopulate(struct device *parent); > #else > static inline int of_platform_populate(struct device_node *root, > const struct of_device_id *matches, > @@ -80,6 +81,9 @@ static inline int of_platform_populate(struct device_node *root, > { > return -ENODEV; > } > +static inline void of_platform_unpopulate(struct device *parent) > +{ > +} > #endif > > #endif /* _LINUX_OF_PLATFORM_H */ > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/21/2013 04:42 PM, Rob Herring wrote: > Wouldn't the correct fix be to call insert_resource somehow? Yes unless there was a reason this wasn't done in the first place. > The problem > I have is that while of_platform_populate is all about parsing the DT > and creating devices, the removal side has nothing to do with DT. So > this should not be in the DT code. I think the core device code should > be able to handle removal if the device creation side is done correctly. If there is no need to use the special removal function (in case we add insert_ressource) then yes. What about a pointer in of_platform_populate()'s comment referring to the removal function? > > It looks to me like of_device_add either needs to call > platform_device_add rather than device_add. I think the device name > setting in platform_device_add should be a nop. If not, a check that the > name is already set could be added. It does actually the same thing as platform_device_add except the "dynamic device id" and the resource insert if I remember correctly. If you guys prefer the platdorm_device_add() path including insert_ressource() I can try this. > > Rob Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/21/2013 09:42 AM, Rob Herring wrote: > On 07/19/2013 01:14 PM, Sebastian Andrzej Siewior wrote: >> So I called of_platform_populate() on a device to get each child device >> probed and on rmmod and I need to reverse its doing. After a quick grep >> I did what others did as well and rmmod ended in: >> >> | Unable to handle kernel NULL pointer dereference at virtual address 00000018 >> | PC is at release_resource+0x18/0x80 >> | Process rmmod (pid: 2005, stack limit = 0xedc30238) >> | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac) >> | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18) >> >> The problem is that platform_device_del() "releases" each ressource in its >> tree. This does not work on platform_devices created by OF becuase they >> were never added via insert_resource(). As a consequence old->parent in >> __release_resource() is NULL and we explode while accessing ->child. >> So I either I do something completly wrong _or_ nobody here tested the >> rmmod path of their driver. > > Wouldn't the correct fix be to call insert_resource somehow? The problem > I have is that while of_platform_populate is all about parsing the DT > and creating devices, the removal side has nothing to do with DT. So > this should not be in the DT code. I think the core device code should > be able to handle removal if the device creation side is done correctly. > > It looks to me like of_device_add either needs to call > platform_device_add rather than device_add. I think the device name > setting in platform_device_add should be a nop. If not, a check that the > name is already set could be added. > BTW, it looks like Grant has attempted this already: commit 02bbde7849e68e193cefaa1885fe0df0f03c9fcd Author: Grant Likely <grant.likely@secretlab.ca> Date: Sun Feb 17 20:03:27 2013 +0000 Revert "of: use platform_device_add" This reverts commit aac73f34542bc7ae4317928d2eabfeb21d247323. That commit causes two kinds of breakage; it breaks registration of AMBA devices when one of the parent nodes already contains overlapping resource regions, and it breaks calls to request_region() by device drivers in certain conditions where there are overlapping memory regions. Both of these problems can probably be fixed, but it is better to back out the commit and get a proper fix designed before trying again. Signed-off-by: Grant Likely <grant.likely@secretlab.ca> Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <robherring2@gmail.com> wrote: > On 07/21/2013 09:42 AM, Rob Herring wrote: >> On 07/19/2013 01:14 PM, Sebastian Andrzej Siewior wrote: >>> So I called of_platform_populate() on a device to get each child device >>> probed and on rmmod and I need to reverse its doing. After a quick grep >>> I did what others did as well and rmmod ended in: >>> >>> | Unable to handle kernel NULL pointer dereference at virtual address 00000018 >>> | PC is at release_resource+0x18/0x80 >>> | Process rmmod (pid: 2005, stack limit = 0xedc30238) >>> | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac) >>> | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18) >>> >>> The problem is that platform_device_del() "releases" each ressource in its >>> tree. This does not work on platform_devices created by OF becuase they >>> were never added via insert_resource(). As a consequence old->parent in >>> __release_resource() is NULL and we explode while accessing ->child. >>> So I either I do something completly wrong _or_ nobody here tested the >>> rmmod path of their driver. >> >> Wouldn't the correct fix be to call insert_resource somehow? The problem >> I have is that while of_platform_populate is all about parsing the DT >> and creating devices, the removal side has nothing to do with DT. So >> this should not be in the DT code. I think the core device code should >> be able to handle removal if the device creation side is done correctly. >> >> It looks to me like of_device_add either needs to call >> platform_device_add rather than device_add. I think the device name >> setting in platform_device_add should be a nop. If not, a check that the >> name is already set could be added. >> > > BTW, it looks like Grant has attempted this already: Yup, things broke badly. Unfortunately the of_platform_device and platform_device history doesn't treat resources in the same way. I would like to merge the code, but I haven't been able to figure out a clean way to do it. Looks like we do need the unpopulate function. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/21/2013 06:44 PM, Grant Likely wrote: > On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <robherring2@gmail.com> wrote: >> On 07/21/2013 09:42 AM, Rob Herring wrote: >>> On 07/19/2013 01:14 PM, Sebastian Andrzej Siewior wrote: >>>> So I called of_platform_populate() on a device to get each child device >>>> probed and on rmmod and I need to reverse its doing. After a quick grep >>>> I did what others did as well and rmmod ended in: >>>> >>>> | Unable to handle kernel NULL pointer dereference at virtual address 00000018 >>>> | PC is at release_resource+0x18/0x80 >>>> | Process rmmod (pid: 2005, stack limit = 0xedc30238) >>>> | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac) >>>> | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18) >>>> >>>> The problem is that platform_device_del() "releases" each ressource in its >>>> tree. This does not work on platform_devices created by OF becuase they >>>> were never added via insert_resource(). As a consequence old->parent in >>>> __release_resource() is NULL and we explode while accessing ->child. >>>> So I either I do something completly wrong _or_ nobody here tested the >>>> rmmod path of their driver. >>> >>> Wouldn't the correct fix be to call insert_resource somehow? The problem >>> I have is that while of_platform_populate is all about parsing the DT >>> and creating devices, the removal side has nothing to do with DT. So >>> this should not be in the DT code. I think the core device code should >>> be able to handle removal if the device creation side is done correctly. >>> >>> It looks to me like of_device_add either needs to call >>> platform_device_add rather than device_add. I think the device name >>> setting in platform_device_add should be a nop. If not, a check that the >>> name is already set could be added. >>> >> >> BTW, it looks like Grant has attempted this already: > > Yup, things broke badly. Unfortunately the of_platform_device and > platform_device history doesn't treat resources in the same way. I > would like to merge the code, but I haven't been able to figure out a > clean way to do it. Looks like we do need the unpopulate function. Was there more breakage than imx6 and amba devices? Your first version had a fallback case for powerpc. Couldn't we do just allow that for more than just powerpc? I'd much rather see some work-around within the core DT code with a warning to prevent more proliferation than putting this into drivers. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 22 Jul 2013 16:16:07 -0500, Rob Herring <robherring2@gmail.com> wrote: > On 07/21/2013 06:44 PM, Grant Likely wrote: > > On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <robherring2@gmail.com> wrote: > >> On 07/21/2013 09:42 AM, Rob Herring wrote: > >>> On 07/19/2013 01:14 PM, Sebastian Andrzej Siewior wrote: > >>>> So I called of_platform_populate() on a device to get each child device > >>>> probed and on rmmod and I need to reverse its doing. After a quick grep > >>>> I did what others did as well and rmmod ended in: > >>>> > >>>> | Unable to handle kernel NULL pointer dereference at virtual address 00000018 > >>>> | PC is at release_resource+0x18/0x80 > >>>> | Process rmmod (pid: 2005, stack limit = 0xedc30238) > >>>> | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac) > >>>> | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18) > >>>> > >>>> The problem is that platform_device_del() "releases" each ressource in its > >>>> tree. This does not work on platform_devices created by OF becuase they > >>>> were never added via insert_resource(). As a consequence old->parent in > >>>> __release_resource() is NULL and we explode while accessing ->child. > >>>> So I either I do something completly wrong _or_ nobody here tested the > >>>> rmmod path of their driver. > >>> > >>> Wouldn't the correct fix be to call insert_resource somehow? The problem > >>> I have is that while of_platform_populate is all about parsing the DT > >>> and creating devices, the removal side has nothing to do with DT. So > >>> this should not be in the DT code. I think the core device code should > >>> be able to handle removal if the device creation side is done correctly. > >>> > >>> It looks to me like of_device_add either needs to call > >>> platform_device_add rather than device_add. I think the device name > >>> setting in platform_device_add should be a nop. If not, a check that the > >>> name is already set could be added. > >>> > >> > >> BTW, it looks like Grant has attempted this already: > > > > Yup, things broke badly. Unfortunately the of_platform_device and > > platform_device history doesn't treat resources in the same way. I > > would like to merge the code, but I haven't been able to figure out a > > clean way to do it. Looks like we do need the unpopulate function. > > Was there more breakage than imx6 and amba devices? Your first version > had a fallback case for powerpc. Couldn't we do just allow that for more > than just powerpc? I'd much rather see some work-around within the core > DT code with a warning to prevent more proliferation than putting this > into drivers. It's tricky stuff. I've not figured out a solution I'm happy with. Trying to figure out when to apply a work around is hard because the resource reservation makes assumptions about the memory range layout that doesn't match the assumptions made by device tree code. One /possible/ option is to not add the resources to the devices at all when the device is registered and instead resolve them right at bind time. Jean Christophe proposed doing this already to solve a different problem; obtaining resources that require other drivers to be probed first. If the resources are resolved at .probe() time, then the resource registration problem should also go away. The downside to that approach is that it makes each deferred probe more expensive; potentially a *lot* more expensive depending on how much work the xlate functions have to do. It would be worth prototyping though to see how well it works. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-07-19 at 20:14 +0200, Sebastian Andrzej Siewior wrote: > The problem is that platform_device_del() "releases" each ressource in its > tree. This does not work on platform_devices created by OF becuase they > were never added via insert_resource(). As a consequence old->parent in > __release_resource() is NULL and we explode while accessing ->child. > So I either I do something completly wrong _or_ nobody here tested the > rmmod path of their driver. But that's wrong. I am not familar with all that new code, but from step up, not having the resources in the resource tree is a bad idea to begin with.... > This patch provides a common function to unregister / remove devices > which added to the system via of_platform_populate(). While this works > now on my test case I have not tested any of the driver I modify here so > feedback is greatly appreciated. Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-07-22 at 00:44 +0100, Grant Likely wrote: > > BTW, it looks like Grant has attempted this already: > > Yup, things broke badly. Unfortunately the of_platform_device and > platform_device history doesn't treat resources in the same way. I > would like to merge the code, but I haven't been able to figure out a > clean way to do it. Looks like we do need the unpopulate function. What is the exact problem Grant ? Care to give me an example ? Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Grant Likely | 2013-07-24 15:19:58 [+0100]: >> Was there more breakage than imx6 and amba devices? Your first version >> had a fallback case for powerpc. Couldn't we do just allow that for more >> than just powerpc? I'd much rather see some work-around within the core >> DT code with a warning to prevent more proliferation than putting this >> into drivers. > >It's tricky stuff. I've not figured out a solution I'm happy with. >Trying to figure out when to apply a work around is hard because the >resource reservation makes assumptions about the memory range layout >that doesn't match the assumptions made by device tree code. I can't really follow. Do you have a simple at hand? >One /possible/ option is to not add the resources to the devices at all >when the device is registered and instead resolve them right at bind >time. Jean Christophe proposed doing this already to solve a different >problem; obtaining resources that require other drivers to be probed >first. If the resources are resolved at .probe() time, then the resource >registration problem should also go away. > >The downside to that approach is that it makes each deferred probe more >expensive; potentially a *lot* more expensive depending on how much work >the xlate functions have to do. It would be worth prototyping though to >see how well it works. So you say defer the io ressources until the device-tree device is actually probed. I don't really understand why that defer part should solve the problem but I would try and see how it goes. Jean-Christophe proposed that only, that means no patches yet, right? >g. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/29/2013 04:33 AM, Benjamin Herrenschmidt wrote: > On Mon, 2013-07-22 at 00:44 +0100, Grant Likely wrote: >>> BTW, it looks like Grant has attempted this already: >> >> Yup, things broke badly. Unfortunately the of_platform_device and >> platform_device history doesn't treat resources in the same way. I >> would like to merge the code, but I haven't been able to figure out a >> clean way to do it. Looks like we do need the unpopulate function. > > What is the exact problem Grant ? Care to give me an example ? > See this thread: http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg63678.html Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/bus/omap-ocp2scp.c b/drivers/bus/omap-ocp2scp.c index 5511f98..510bb9e 100644 --- a/drivers/bus/omap-ocp2scp.c +++ b/drivers/bus/omap-ocp2scp.c @@ -23,15 +23,6 @@ #include <linux/of.h> #include <linux/of_platform.h> -static int ocp2scp_remove_devices(struct device *dev, void *c) -{ - struct platform_device *pdev = to_platform_device(dev); - - platform_device_unregister(pdev); - - return 0; -} - static int omap_ocp2scp_probe(struct platform_device *pdev) { int ret; @@ -51,7 +42,7 @@ static int omap_ocp2scp_probe(struct platform_device *pdev) return 0; err0: - device_for_each_child(&pdev->dev, NULL, ocp2scp_remove_devices); + of_platform_unpopulate(&pdev->dev); return ret; } @@ -59,7 +50,7 @@ static int omap_ocp2scp_probe(struct platform_device *pdev) static int omap_ocp2scp_remove(struct platform_device *pdev) { pm_runtime_disable(&pdev->dev); - device_for_each_child(&pdev->dev, NULL, ocp2scp_remove_devices); + of_platform_unpopulate(&pdev->dev); return 0; } diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c index 9809fc9..10248e1 100644 --- a/drivers/iio/adc/exynos_adc.c +++ b/drivers/iio/adc/exynos_adc.c @@ -216,15 +216,6 @@ static const struct iio_chan_spec exynos_adc_iio_channels[] = { ADC_CHANNEL(9, "adc9"), }; -static int exynos_adc_remove_devices(struct device *dev, void *c) -{ - struct platform_device *pdev = to_platform_device(dev); - - platform_device_unregister(pdev); - - return 0; -} - static void exynos_adc_hw_init(struct exynos_adc *info) { u32 con1, con2; @@ -357,8 +348,7 @@ static int exynos_adc_probe(struct platform_device *pdev) return 0; err_of_populate: - device_for_each_child(&pdev->dev, NULL, - exynos_adc_remove_devices); + of_platform_unpopulate(&pdev->dev); regulator_disable(info->vdd); clk_disable_unprepare(info->clk); err_iio_dev: @@ -375,8 +365,7 @@ static int exynos_adc_remove(struct platform_device *pdev) struct iio_dev *indio_dev = platform_get_drvdata(pdev); struct exynos_adc *info = iio_priv(indio_dev); - device_for_each_child(&pdev->dev, NULL, - exynos_adc_remove_devices); + of_platform_unpopulate(&pdev->dev); regulator_disable(info->vdd); clk_disable_unprepare(info->clk); writel(0, info->enable_reg); diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c index 759fae3..bb26cea 100644 --- a/drivers/mfd/omap-usb-host.c +++ b/drivers/mfd/omap-usb-host.c @@ -832,13 +832,6 @@ static int usbhs_omap_probe(struct platform_device *pdev) return ret; } -static int usbhs_omap_remove_child(struct device *dev, void *data) -{ - dev_info(dev, "unregistering\n"); - platform_device_unregister(to_platform_device(dev)); - return 0; -} - /** * usbhs_omap_remove - shutdown processing for UHH & TLL HCDs * @pdev: USB Host Controller being removed @@ -871,7 +864,7 @@ static int usbhs_omap_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); /* remove children */ - device_for_each_child(&pdev->dev, NULL, usbhs_omap_remove_child); + of_platform_unpopulate(&pdev->dev); return 0; } diff --git a/drivers/of/platform.c b/drivers/of/platform.c index e0a6514..9cbb0c3 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -473,4 +473,26 @@ int of_platform_populate(struct device_node *root, return rc; } EXPORT_SYMBOL_GPL(of_platform_populate); + +static int of_remove_populated_child(struct device *dev, void *d) +{ + struct platform_device *pdev = to_platform_device(dev); + + of_device_unregister(pdev); + return 0; +} +/** + * of_platform_unpopulate() - Remove populated devices. + * @parent: parent of the populated devices. + * + * The reverse of of_platform_populate() and can only be used a parent was + * specified while invoking the former. + */ +void of_platform_unpopulate(struct device *parent) +{ + if (WARN_ON(!parent)) + return; + device_for_each_child(parent, NULL, of_remove_populated_child); +} +EXPORT_SYMBOL_GPL(of_platform_unpopulate); #endif /* CONFIG_OF_ADDRESS */ diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c index 8ce9d7f..2bf5664 100644 --- a/drivers/usb/dwc3/dwc3-exynos.c +++ b/drivers/usb/dwc3/dwc3-exynos.c @@ -86,15 +86,6 @@ static int dwc3_exynos_register_phys(struct dwc3_exynos *exynos) return ret; } -static int dwc3_exynos_remove_child(struct device *dev, void *unused) -{ - struct platform_device *pdev = to_platform_device(dev); - - platform_device_unregister(pdev); - - return 0; -} - static int dwc3_exynos_probe(struct platform_device *pdev) { struct dwc3_exynos *exynos; @@ -164,7 +155,7 @@ static int dwc3_exynos_remove(struct platform_device *pdev) { struct dwc3_exynos *exynos = platform_get_drvdata(pdev); - device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child); + of_platform_unpopulate(&pdev->dev); platform_device_unregister(exynos->usb2_phy); platform_device_unregister(exynos->usb3_phy); diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c index 077f110..8688613 100644 --- a/drivers/usb/dwc3/dwc3-omap.c +++ b/drivers/usb/dwc3/dwc3-omap.c @@ -327,15 +327,6 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) return IRQ_HANDLED; } -static int dwc3_omap_remove_core(struct device *dev, void *c) -{ - struct platform_device *pdev = to_platform_device(dev); - - platform_device_unregister(pdev); - - return 0; -} - static void dwc3_omap_enable_irqs(struct dwc3_omap *omap) { u32 reg; @@ -529,8 +520,7 @@ static int dwc3_omap_remove(struct platform_device *pdev) dwc3_omap_disable_irqs(omap); pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); - device_for_each_child(&pdev->dev, NULL, dwc3_omap_remove_core); - + of_platform_unpopulate(&pdev->dev); return 0; } diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h index 05cb4a9..e354f9c 100644 --- a/include/linux/of_platform.h +++ b/include/linux/of_platform.h @@ -72,6 +72,7 @@ extern int of_platform_populate(struct device_node *root, const struct of_device_id *matches, const struct of_dev_auxdata *lookup, struct device *parent); +extern void of_platform_unpopulate(struct device *parent); #else static inline int of_platform_populate(struct device_node *root, const struct of_device_id *matches, @@ -80,6 +81,9 @@ static inline int of_platform_populate(struct device_node *root, { return -ENODEV; } +static inline void of_platform_unpopulate(struct device *parent) +{ +} #endif #endif /* _LINUX_OF_PLATFORM_H */
So I called of_platform_populate() on a device to get each child device probed and on rmmod and I need to reverse its doing. After a quick grep I did what others did as well and rmmod ended in: | Unable to handle kernel NULL pointer dereference at virtual address 00000018 | PC is at release_resource+0x18/0x80 | Process rmmod (pid: 2005, stack limit = 0xedc30238) | [<c003add0>] (release_resource+0x18/0x80) from [<c0300e08>] (platform_device_del+0x78/0xac) | [<c0300e08>] (platform_device_del+0x78/0xac) from [<c0301358>] (platform_device_unregister+0xc/0x18) The problem is that platform_device_del() "releases" each ressource in its tree. This does not work on platform_devices created by OF becuase they were never added via insert_resource(). As a consequence old->parent in __release_resource() is NULL and we explode while accessing ->child. So I either I do something completly wrong _or_ nobody here tested the rmmod path of their driver. This patch provides a common function to unregister / remove devices which added to the system via of_platform_populate(). While this works now on my test case I have not tested any of the driver I modify here so feedback is greatly appreciated. Cc: Tony Lindgren <tony@atomide.com> Cc: Doug Anderson <dianders@chromium.org> Cc: Vivek Gautam <gautam.vivek@samsung.com> Cc: Naveen Krishna Chatradhi <ch.naveen@samsung.com> Cc: Kukjin Kim <kgene.kim@samsung.com> Cc: Kishon Vijay Abraham I <kishon@ti.com> Cc: Roger Quadros <rogerq@ti.com> Cc: George Cherian <george.cherian@ti.com> Cc: Felipe Balbi <balbi@ti.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/bus/omap-ocp2scp.c | 13 ++----------- drivers/iio/adc/exynos_adc.c | 15 ++------------- drivers/mfd/omap-usb-host.c | 9 +-------- drivers/of/platform.c | 22 ++++++++++++++++++++++ drivers/usb/dwc3/dwc3-exynos.c | 11 +---------- drivers/usb/dwc3/dwc3-omap.c | 12 +----------- include/linux/of_platform.h | 4 ++++ 7 files changed, 33 insertions(+), 53 deletions(-)