| Message ID | 20220607202058.8304-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] driver core: Introduce device_find_first_child() helper | expand |
On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > There are several places in the kernel where this kind of functionality is > being used. Provide a generic helper for such cases. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/base/core.c | 24 ++++++++++++++++++++++++ > include/linux/device.h | 1 + > 2 files changed, 25 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 7cd789c4985d..972bfe975cd0 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -3832,6 +3832,30 @@ struct device *device_find_child_by_name(struct device *parent, > } > EXPORT_SYMBOL_GPL(device_find_child_by_name); > > +/** > + * device_find_first_child - device iterator for locating the fist child device. > + * @parent: parent struct device > + * > + * This is similar to the device_find_child() function above, but it > + * returns a reference to the first child device. > + * > + * NOTE: you will need to drop the reference with put_device() after use. > + */ > +struct device *device_find_first_child(struct device *parent) > +{ > + struct klist_iter i; > + struct device *child; > + > + if (!parent) > + return NULL; > + > + klist_iter_init(&parent->p->klist_children, &i); > + child = get_device(next_device(&i)); > + klist_iter_exit(&i); > + return child; > +} > +EXPORT_SYMBOL_GPL(device_find_first_child); I would define it as static int match_first(struct device *dev, void *) { return 1; } struct device *device_find_first_child(struct device *parent) { return device_find_first_child(parent, NULL, match_first); } EXPORT_SYMBOL_GPL(device_find_first_child); which is not that much more overhead. > + > int __init devices_init(void) > { > devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL); > diff --git a/include/linux/device.h b/include/linux/device.h > index dc941997795c..20171a4358df 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -905,6 +905,7 @@ struct device *device_find_child(struct device *dev, void *data, > int (*match)(struct device *dev, void *data)); > struct device *device_find_child_by_name(struct device *parent, > const char *name); > +struct device *device_find_first_child(struct device *parent); > int device_rename(struct device *dev, const char *new_name); > int device_move(struct device *dev, struct device *new_parent, > enum dpm_order dpm_order); > -- > 2.35.1 >
On Tue, Jun 07, 2022 at 11:20:57PM +0300, Andy Shevchenko wrote: > There are several places in the kernel where this kind of functionality is > being used. Provide a generic helper for such cases. This feels really wrong/broken. There should not be any specific ordering of children in the tree. What subsystem relies on this such that they require this? thanks, greg k-h
On Wed, Jun 08, 2022 at 01:29:08PM +0200, Rafael J. Wysocki wrote: > On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: ... > I would define it as > > static int match_first(struct device *dev, void *) > { > return 1; > } > > struct device *device_find_first_child(struct device *parent) > { > return device_find_first_child(parent, NULL, match_first); > } > EXPORT_SYMBOL_GPL(device_find_first_child); > > which is not that much more overhead. With this we actually may simply provide a match function and it will make the clean ups (like patch 2 in the series) almost the same without introducing a device core call. Something like int device_match_any_for_find(struct device *dev, void *unused) { return 1; } As I replied to Greg it's pity we can't use device_match_any()...
On Wed, Jun 08, 2022 at 02:53:28PM +0300, Andy Shevchenko wrote: > On Wed, Jun 08, 2022 at 01:29:08PM +0200, Rafael J. Wysocki wrote: > > On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > I would define it as > > > > static int match_first(struct device *dev, void *) > > { > > return 1; > > } > > > > struct device *device_find_first_child(struct device *parent) > > { > > return device_find_first_child(parent, NULL, match_first); > > } > > EXPORT_SYMBOL_GPL(device_find_first_child); > > > > which is not that much more overhead. > > With this we actually may simply provide a match function and it will make the > clean ups (like patch 2 in the series) almost the same without introducing a > device core call. > > Something like > > int device_match_any_for_find(struct device *dev, void *unused) > { > return 1; > } > > As I replied to Greg it's pity we can't use device_match_any()... int device_match_any(struct device *dev, const void *unused) How is that not ok to use here? thanks, greg k-h
On Wed, Jun 8, 2022 at 2:04 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jun 08, 2022 at 02:53:28PM +0300, Andy Shevchenko wrote: > > On Wed, Jun 08, 2022 at 01:29:08PM +0200, Rafael J. Wysocki wrote: > > > On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > ... > > > > > I would define it as > > > > > > static int match_first(struct device *dev, void *) > > > { > > > return 1; > > > } > > > > > > struct device *device_find_first_child(struct device *parent) > > > { > > > return device_find_first_child(parent, NULL, match_first); > > > } > > > EXPORT_SYMBOL_GPL(device_find_first_child); > > > > > > which is not that much more overhead. > > > > With this we actually may simply provide a match function and it will make the > > clean ups (like patch 2 in the series) almost the same without introducing a > > device core call. > > > > Something like > > > > int device_match_any_for_find(struct device *dev, void *unused) > > { > > return 1; > > } > > > > As I replied to Greg it's pity we can't use device_match_any()... > > int device_match_any(struct device *dev, const void *unused) > > How is that not ok to use here? Because of the const that will be frowned upon by the compiler. We need to define another device_match_any_relaxed() taking (void *) as the second argument for this.
On Wed, Jun 8, 2022 at 1:54 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Jun 08, 2022 at 01:29:08PM +0200, Rafael J. Wysocki wrote: > > On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > I would define it as > > > > static int match_first(struct device *dev, void *) > > { > > return 1; > > } > > > > struct device *device_find_first_child(struct device *parent) > > { > > return device_find_first_child(parent, NULL, match_first); > > } > > EXPORT_SYMBOL_GPL(device_find_first_child); > > > > which is not that much more overhead. > > With this we actually may simply provide a match function and it will make the > clean ups (like patch 2 in the series) almost the same without introducing a > device core call. That works too, but IMO it would be a bit cleaner to have the wrapper defined as a proper function. > > Something like > > int device_match_any_for_find(struct device *dev, void *unused) > { > return 1; > } > > As I replied to Greg it's pity we can't use device_match_any()... Well, that only is a matter of adding one more variant of _match_any_ ...
On Wed, Jun 08, 2022 at 02:15:19PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 8, 2022 at 2:04 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Jun 08, 2022 at 02:53:28PM +0300, Andy Shevchenko wrote: > > > On Wed, Jun 08, 2022 at 01:29:08PM +0200, Rafael J. Wysocki wrote: > > > > On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > ... > > > > > > > I would define it as > > > > > > > > static int match_first(struct device *dev, void *) > > > > { > > > > return 1; > > > > } > > > > > > > > struct device *device_find_first_child(struct device *parent) > > > > { > > > > return device_find_first_child(parent, NULL, match_first); > > > > } > > > > EXPORT_SYMBOL_GPL(device_find_first_child); > > > > > > > > which is not that much more overhead. > > > > > > With this we actually may simply provide a match function and it will make the > > > clean ups (like patch 2 in the series) almost the same without introducing a > > > device core call. > > > > > > Something like > > > > > > int device_match_any_for_find(struct device *dev, void *unused) > > > { > > > return 1; > > > } > > > > > > As I replied to Greg it's pity we can't use device_match_any()... > > > > int device_match_any(struct device *dev, const void *unused) > > > > How is that not ok to use here? > > Because of the const that will be frowned upon by the compiler. > > We need to define another device_match_any_relaxed() taking (void *) > as the second argument for this. Or we could cast it away :)
On Wed, Jun 8, 2022 at 2:23 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jun 08, 2022 at 02:15:19PM +0200, Rafael J. Wysocki wrote: > > On Wed, Jun 8, 2022 at 2:04 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Wed, Jun 08, 2022 at 02:53:28PM +0300, Andy Shevchenko wrote: > > > > On Wed, Jun 08, 2022 at 01:29:08PM +0200, Rafael J. Wysocki wrote: > > > > > On Tue, Jun 7, 2022 at 10:22 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > ... > > > > > > > > > I would define it as > > > > > > > > > > static int match_first(struct device *dev, void *) > > > > > { > > > > > return 1; > > > > > } > > > > > > > > > > struct device *device_find_first_child(struct device *parent) > > > > > { > > > > > return device_find_first_child(parent, NULL, match_first); > > > > > } > > > > > EXPORT_SYMBOL_GPL(device_find_first_child); > > > > > > > > > > which is not that much more overhead. > > > > > > > > With this we actually may simply provide a match function and it will make the > > > > clean ups (like patch 2 in the series) almost the same without introducing a > > > > device core call. > > > > > > > > Something like > > > > > > > > int device_match_any_for_find(struct device *dev, void *unused) > > > > { > > > > return 1; > > > > } > > > > > > > > As I replied to Greg it's pity we can't use device_match_any()... > > > > > > int device_match_any(struct device *dev, const void *unused) > > > > > > How is that not ok to use here? > > > > Because of the const that will be frowned upon by the compiler. > > > > We need to define another device_match_any_relaxed() taking (void *) > > as the second argument for this. > > Or we could cast it away :) I'm not sure what exactly you mean. The function pointer signature doesn't match here.
diff --git a/drivers/base/core.c b/drivers/base/core.c index 7cd789c4985d..972bfe975cd0 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -3832,6 +3832,30 @@ struct device *device_find_child_by_name(struct device *parent, } EXPORT_SYMBOL_GPL(device_find_child_by_name); +/** + * device_find_first_child - device iterator for locating the fist child device. + * @parent: parent struct device + * + * This is similar to the device_find_child() function above, but it + * returns a reference to the first child device. + * + * NOTE: you will need to drop the reference with put_device() after use. + */ +struct device *device_find_first_child(struct device *parent) +{ + struct klist_iter i; + struct device *child; + + if (!parent) + return NULL; + + klist_iter_init(&parent->p->klist_children, &i); + child = get_device(next_device(&i)); + klist_iter_exit(&i); + return child; +} +EXPORT_SYMBOL_GPL(device_find_first_child); + int __init devices_init(void) { devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL); diff --git a/include/linux/device.h b/include/linux/device.h index dc941997795c..20171a4358df 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -905,6 +905,7 @@ struct device *device_find_child(struct device *dev, void *data, int (*match)(struct device *dev, void *data)); struct device *device_find_child_by_name(struct device *parent, const char *name); +struct device *device_find_first_child(struct device *parent); int device_rename(struct device *dev, const char *new_name); int device_move(struct device *dev, struct device *new_parent, enum dpm_order dpm_order);
There are several places in the kernel where this kind of functionality is being used. Provide a generic helper for such cases. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/base/core.c | 24 ++++++++++++++++++++++++ include/linux/device.h | 1 + 2 files changed, 25 insertions(+)