[PATCH] of: Add of_device_destroy_children() function

[PATCH] of: Add of_device_destroy_children() function

[Sylwester Nawrocki]

This patch adds a helper function to unregister devices which
were created by an of_platform_populate() call. The pattern
used here can already be found in multiple drivers. This helper
can now be used instead of repeating similar code in drivers.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---

This patch has been tested on ARM only (on Exynos4412 Trats2 board).

 drivers/of/device.c       |   24 ++++++++++++++++++++++++
 include/linux/of_device.h |    3 +++
 2 files changed, 27 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index dafb973..9303197 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -190,3 +190,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)

 	return 0;
 }
+
+static int __remove_child_device(struct device *dev, void *unused)
+{
+	if (of_match_node(of_default_bus_match_table, dev->of_node))
+		of_device_destroy_children(dev);
+
+	device_unregister(dev);
+	return 0;
+}
+
+/**
+ * of_device_destroy_children - unregister @parent's child devices
+ * @parent: the parent device to start with
+ *
+ * Destroy all child devices of the @parent device, any grandchildren
+ * compatible with values listed in the of_default_bus_match_table will
+ * also be unregistered recursively.  This function can be used to
+ * destroy devices created by an of_platform_populate() call.
+ */
+void of_device_destroy_children(struct device *parent)
+{
+	device_for_each_child(parent, NULL, __remove_child_device);
+}
+EXPORT_SYMBOL(of_device_destroy_children);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index ef37021..0c41e0b 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -32,6 +32,7 @@ extern void of_dev_put(struct platform_device *dev);
 extern int of_device_add(struct platform_device *pdev);
 extern int of_device_register(struct platform_device *ofdev);
 extern void of_device_unregister(struct platform_device *ofdev);
+extern void of_device_destroy_children(struct device *parent);

 extern ssize_t of_device_get_modalias(struct device *dev,
 					char *str, ssize_t len);
@@ -64,6 +65,8 @@ static inline int of_driver_match_device(struct device *dev,
 static inline void of_device_uevent(struct device *dev,
 			struct kobj_uevent_env *env) { }

+static inline void of_device_destroy_children(struct device *parent) { }
+
 static inline int of_device_get_modalias(struct device *dev,
 				   char *str, ssize_t len)
 {
--
1.7.9.5

Re: [PATCH] of: Add of_device_destroy_children() function

[Jason Gunthorpe]

On Thu, May 08, 2014 at 06:37:49PM +0200, Sylwester Nawrocki wrote:
> This patch adds a helper function to unregister devices which
> were created by an of_platform_populate() call. The pattern
> used here can already be found in multiple drivers. This helper
> can now be used instead of repeating similar code in drivers.

I have a driver that does this as well, and what I found is that the
remove must be in reverse order from the create or things explode, and
that assumes the DT is topologically sorted according to dependency
(so no deferred probe).

AFAIK, there is no analog to deferred probe for removal, and
attempting to remove, say, a GPIO driver while an I2C bit bang is using
it just fails.

Jason

Re: [PATCH] of: Add of_device_destroy_children() function

[Sylwester Nawrocki]

On 08/05/14 22:33, Jason Gunthorpe wrote:
> On Thu, May 08, 2014 at 06:37:49PM +0200, Sylwester Nawrocki wrote:
>> This patch adds a helper function to unregister devices which
>> were created by an of_platform_populate() call. The pattern
>> used here can already be found in multiple drivers. This helper
>> can now be used instead of repeating similar code in drivers.
> 
> I have a driver that does this as well, and what I found is that the
> remove must be in reverse order from the create or things explode, and
> that assumes the DT is topologically sorted according to dependency
> (so no deferred probe).
> 
> AFAIK, there is no analog to deferred probe for removal, and
> attempting to remove, say, a GPIO driver while an I2C bit bang is using
> it just fails.

Thanks for the feedback, I knew I could be missing some of nasty
details like this. Looks like we need a complete implementation
of of_platform_unpopulate(). Since the are cases where the remove
order is insignificant, I'm wondering whether it still would be
useful to have a helper like device_unregister_children() which
would remove only direct children of a device ? At least this
solves my current problem.

Since the dependencies will likely never be fully described in DT
I guess we would need to create a list while actually creating
devices, to be able to walk in reverse order while destroying them.

--
Regards,
Sylwester

Re: [PATCH] of: Add of_device_destroy_children() function

[Grant Likely]

On Thu, 8 May 2014 14:33:39 -0600, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Thu, May 08, 2014 at 06:37:49PM +0200, Sylwester Nawrocki wrote:
> > This patch adds a helper function to unregister devices which
> > were created by an of_platform_populate() call. The pattern
> > used here can already be found in multiple drivers. This helper
> > can now be used instead of repeating similar code in drivers.
> 
> I have a driver that does this as well, and what I found is that the
> remove must be in reverse order from the create or things explode, and
> that assumes the DT is topologically sorted according to dependency
> (so no deferred probe).

That is the tip of a much larger problem that we don't have any good way
to solve. There is no dependency tracking beyond the nature Linux driver
model tree. For example, the removal of a GPIO driver has no way to
tell users that it is going away, and so there is no way to force a
driver remove when it happens.

If we created a managed api for requesting resource (ie.
devm_request_gpio()), then it would be possible for the gpio core to
force a remove event on any driver that doesn't have the ability to
gracefully handle a remove.

The exact same problem exists for IRQs, clocks, regulators, or pretty
much any cross-tree dependency.  :-(

> AFAIK, there is no analog to deferred probe for removal, and
> attempting to remove, say, a GPIO driver while an I2C bit bang is using
> it just fails.

Indeed, it is a completely different operation.

g.

Re: [PATCH] of: Add of_device_destroy_children() function

[Grant Likely]

On Thu, 08 May 2014 18:37:49 +0200, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> This patch adds a helper function to unregister devices which
> were created by an of_platform_populate() call. The pattern
> used here can already be found in multiple drivers. This helper
> can now be used instead of repeating similar code in drivers.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---

Pawel has also submitted a patch for this. That patch is a little more
careful about how it removes nodes, so I'll be going with that one.

g.

> 
> This patch has been tested on ARM only (on Exynos4412 Trats2 board).
> 
>  drivers/of/device.c       |   24 ++++++++++++++++++++++++
>  include/linux/of_device.h |    3 +++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index dafb973..9303197 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -190,3 +190,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> 
>  	return 0;
>  }
> +
> +static int __remove_child_device(struct device *dev, void *unused)
> +{
> +	if (of_match_node(of_default_bus_match_table, dev->of_node))
> +		of_device_destroy_children(dev);
> +
> +	device_unregister(dev);
> +	return 0;
> +}
> +
> +/**
> + * of_device_destroy_children - unregister @parent's child devices
> + * @parent: the parent device to start with
> + *
> + * Destroy all child devices of the @parent device, any grandchildren
> + * compatible with values listed in the of_default_bus_match_table will
> + * also be unregistered recursively.  This function can be used to
> + * destroy devices created by an of_platform_populate() call.
> + */
> +void of_device_destroy_children(struct device *parent)
> +{
> +	device_for_each_child(parent, NULL, __remove_child_device);
> +}
> +EXPORT_SYMBOL(of_device_destroy_children);
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index ef37021..0c41e0b 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -32,6 +32,7 @@ extern void of_dev_put(struct platform_device *dev);
>  extern int of_device_add(struct platform_device *pdev);
>  extern int of_device_register(struct platform_device *ofdev);
>  extern void of_device_unregister(struct platform_device *ofdev);
> +extern void of_device_destroy_children(struct device *parent);
> 
>  extern ssize_t of_device_get_modalias(struct device *dev,
>  					char *str, ssize_t len);
> @@ -64,6 +65,8 @@ static inline int of_driver_match_device(struct device *dev,
>  static inline void of_device_uevent(struct device *dev,
>  			struct kobj_uevent_env *env) { }
> 
> +static inline void of_device_destroy_children(struct device *parent) { }
> +
>  static inline int of_device_get_modalias(struct device *dev,
>  				   char *str, ssize_t len)
>  {
> --
> 1.7.9.5
> 

Re: [PATCH] of: Add of_device_destroy_children() function

[Sylwester Nawrocki]

On 14/05/14 12:27, Grant Likely wrote:
> On Thu, 08 May 2014 18:37:49 +0200, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>> > This patch adds a helper function to unregister devices which
>> > were created by an of_platform_populate() call. The pattern
>> > used here can already be found in multiple drivers. This helper
>> > can now be used instead of repeating similar code in drivers.
>> > 
>> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> > ---
>
> Pawel has also submitted a patch for this. That patch is a little more
> careful about how it removes nodes, so I'll be going with that one.

OK, it's good to know we have already a good solution. I'll give a try
Pawel's patch then.

Thanks,
Sylwester