Re: [PATCH] of: provide of_platform_unpopulate()

Re: [PATCH] of: provide of_platform_unpopulate()

[NAVEEN KRISHNA CHATRADHI]

Hello Sebastian,

I just did one more testing.

In case of iio/adc/exynos_adc.c there is a bug in the remove path.
If I fix the bug in the driver, with below patch

--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -375,14 +375,14 @@ 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);
        regulator_disable(info->vdd);
        clk_disable_unprepare(info->clk);
        writel(0, info->enable_reg);
        iio_device_unregister(indio_dev);
        free_irq(info->irq, info);
        iio_device_free(indio_dev);
+       device_for_each_child(&pdev->dev, NULL,
+                               exynos_adc_remove_devices);

Even without your fix, I could configure it as a module and the rmmod, insmod are working fine. (no crash)

Regards,
Naveen
------- Original Message -------
Sender : Sebastian Andrzej Siewior<bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> 
Date   : Jul 19, 2013 23:44 (GMT+05:30)
Title  : [PATCH] of: provide of_platform_unpopulate()

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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Cc: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Vivek Gautam <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Kukjin Kim <kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Cc: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
Cc: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
Cc: George Cherian <george.cherian-l0cyMroinI0@public.gmane.org>
Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 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
");
-	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 */
-- 
1.8.3.2

<p>&nbsp;</p><p>&nbsp;</p>Thanks & Best Regards,
Naveen Krishna Ch
SE @ Samsung-B.LAB

Re: [PATCH] of: provide of_platform_unpopulate()

[Sebastian Andrzej Siewior]

On 07/20/2013 07:42 AM, NAVEEN KRISHNA CHATRADHI wrote:
> Hello Sebastian,

Hello Naveen,

> 
> I just did one more testing.
> 
> In case of iio/adc/exynos_adc.c there is a bug in the remove path.
> If I fix the bug in the driver, with below patch
> 
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -375,14 +375,14 @@ 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);
>         regulator_disable(info->vdd);
>         clk_disable_unprepare(info->clk);
>         writel(0, info->enable_reg);
>         iio_device_unregister(indio_dev);
>         free_irq(info->irq, info);
>         iio_device_free(indio_dev);
> +       device_for_each_child(&pdev->dev, NULL,
> +                               exynos_adc_remove_devices);
> 
> Even without your fix, I could configure it as a module and the rmmod, insmod are working fine. (no crash)

I have no idea why you moved it. I haven't found any .dts with this
binding but from the binding document I would assume that you do not
have any memory resources and therefore you don't see that crash.

> Regards,
> Naveen

Sebastian

Re: [PATCH] of: provide of_platform_unpopulate()

[NAVEEN KRISHNA CHATRADHI]

Hello Sebastian,

------- Original Message -------
Sender : Sebastian Andrzej Siewior<bigeasy@linutronix.de> 
Date   : Jul 19, 2013 23:44 (GMT+05:30)
Title  : [PATCH] of: provide of_platform_unpopulate()

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.
I've tested this on the exynos_adc driver under iio/adc/
and found a bug in the driver. When I fix the bug, your change happily rmmods the module.

Will send the patch separately.
Thanks for pointing out the bug for me.

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
");
-	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 */
-- 
1.8.3.2

<p>&nbsp;</p><p>&nbsp;</p>Thanks & Best Regards,
Naveen Krishna Ch
SE @ Samsung-B.LAB

[PATCH] of: provide of_platform_unpopulate()

[Sebastian Andrzej Siewior]

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(-)

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 */
-- 
1.8.3.2

Re: [PATCH] of: provide of_platform_unpopulate()

[Rob Herring]

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-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Cc: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Vivek Gautam <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Kukjin Kim <kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
> Cc: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Cc: George Cherian <george.cherian-l0cyMroinI0@public.gmane.org>
> Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
>  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 */
> 

Re: [PATCH] of: provide of_platform_unpopulate()

[Sebastian Andrzej Siewior]

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

Re: [PATCH] of: provide of_platform_unpopulate()

[Rob Herring]

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

Re: [PATCH] of: provide of_platform_unpopulate()

[Grant Likely]

On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

Re: [PATCH] of: provide of_platform_unpopulate()

[Rob Herring]

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

Re: [PATCH] of: provide of_platform_unpopulate()

[Grant Likely]

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.

Re: [PATCH] of: provide of_platform_unpopulate()

[Sebastian Andrzej Siewior]

* 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

Re: [PATCH] of: provide of_platform_unpopulate()

[Benjamin Herrenschmidt]

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.

Re: [PATCH] of: provide of_platform_unpopulate()

[Rob Herring]

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

Re: [PATCH] of: provide of_platform_unpopulate()

[Benjamin Herrenschmidt]

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.