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 <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Vivek Gautam <[email protected]>
Cc: Naveen Krishna Chatradhi <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Roger Quadros <[email protected]>
Cc: George Cherian <[email protected]>
Cc: Felipe Balbi <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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
Hello Sebastian,
------- Original Message -------
Sender : Sebastian Andrzej Siewior<[email protected]>
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 <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Vivek Gautam <[email protected]>
Cc: Naveen Krishna Chatradhi <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Roger Quadros <[email protected]>
Cc: George Cherian <[email protected]>
Cc: Felipe Balbi <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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> </p><p> </p>Thanks & Best Regards,
Naveen Krishna Ch
SE @ Samsung-B.LAB????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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<[email protected]>
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 <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Vivek Gautam <[email protected]>
Cc: Naveen Krishna Chatradhi <[email protected]>
Cc: Kukjin Kim <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Roger Quadros <[email protected]>
Cc: George Cherian <[email protected]>
Cc: Felipe Balbi <[email protected]>
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
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> </p><p> </p>Thanks & Best Regards,
Naveen Krishna Ch
SE @ Samsung-B.LAB????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Vivek Gautam <[email protected]>
> Cc: Naveen Krishna Chatradhi <[email protected]>
> Cc: Kukjin Kim <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: Roger Quadros <[email protected]>
> Cc: George Cherian <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> 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 */
>
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
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 <[email protected]>
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 <[email protected]>
Rob
On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <[email protected]> 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.
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
Hello Sebastian,
------- Original Message -------
Sender : Sebastian Andrzej Siewior<[email protected]>
Date : Jul 22, 2013 13:55 (GMT+05:30)
Title : Re: [PATCH] of: provide of_platform_unpopulate()
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
regulator_disable(info->vdd);
has to happen before we free and remove the devices.
The dts part is not upstreamed yet.
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.
Yes, my devices under ADC are 3rd party thermistors and they don't have memory resources.
May be your explanation seems right.
Thanks,
Naveen
> Regards,
> Naveen
Sebastian
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 07/21/2013 06:44 PM, Grant Likely wrote:
> On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <[email protected]> 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
On Mon, 22 Jul 2013 16:16:07 -0500, Rob Herring <[email protected]> wrote:
> On 07/21/2013 06:44 PM, Grant Likely wrote:
> > On Sun, Jul 21, 2013 at 9:48 PM, Rob Herring <[email protected]> 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.
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.
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.
* 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
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/[email protected]/msg63678.html
Rob