| Message ID | 1399567069-3174-1-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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 >
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
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) {