| Message ID | 20241008-mv88e6xxx_leds_fwnode_put-v1-1-cfd7758cd176@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net, device property: fix led node releases in mv88e6xxx with new macro | expand |
On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote: > Introduce the scoped variant of the > fwnode_for_each_available_child_node() to automatically decrement the > child's refcount when it goes out of scope, removing the need for > explicit calls to fwnode_handle_put(). ... > +#define fwnode_for_each_available_child_node_scoped(fwnode, child) \ > + for (struct fwnode_handle *child __free(fwnode_handle) = \ > + fwnode_get_next_available_child_node(fwnode, NULL); child; \ > + child = fwnode_get_next_available_child_node(fwnode, child)) I like the wrapping you have done here. Can you align the device_for_each_child_node_scoped() to follow your variant? (probably in an additional patch) For this one Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hi Javier, On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote: > Introduce the scoped variant of the > fwnode_for_each_available_child_node() to automatically decrement the > child's refcount when it goes out of scope, removing the need for > explicit calls to fwnode_handle_put(). > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > include/linux/property.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/property.h b/include/linux/property.h > index 61fc20e5f81f..b37508ecf606 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node( > for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\ > child = fwnode_get_next_available_child_node(fwnode, child)) > > +#define fwnode_for_each_available_child_node_scoped(fwnode, child) \ > + for (struct fwnode_handle *child __free(fwnode_handle) = \ > + fwnode_get_next_available_child_node(fwnode, NULL); child; \ > + child = fwnode_get_next_available_child_node(fwnode, child)) > + On OF, the implementation of the .get_next_child_node() fwnode op is: static struct fwnode_handle * of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child) { return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), to_of_node(child))); } On ACPI we currently have .device_is_available() returning false but that probably should be returning true instead (it's been virtually unused previously). That makes fwnode_get_next_available_child_node() and fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably creating unavailable nodes would be useless on swnode, too. So my question is: what do we gain by adding all these fwnode_*available() helpers? > struct fwnode_handle *device_get_next_child_node(const struct device *dev, > struct fwnode_handle *child);
On 11/10/2024 07:39, Sakari Ailus wrote: > Hi Javier, > > On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote: >> Introduce the scoped variant of the >> fwnode_for_each_available_child_node() to automatically decrement the >> child's refcount when it goes out of scope, removing the need for >> explicit calls to fwnode_handle_put(). >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> --- >> include/linux/property.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/include/linux/property.h b/include/linux/property.h >> index 61fc20e5f81f..b37508ecf606 100644 >> --- a/include/linux/property.h >> +++ b/include/linux/property.h >> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node( >> for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\ >> child = fwnode_get_next_available_child_node(fwnode, child)) >> >> +#define fwnode_for_each_available_child_node_scoped(fwnode, child) \ >> + for (struct fwnode_handle *child __free(fwnode_handle) = \ >> + fwnode_get_next_available_child_node(fwnode, NULL); child; \ >> + child = fwnode_get_next_available_child_node(fwnode, child)) >> + > > On OF, the implementation of the .get_next_child_node() fwnode op is: > > static struct fwnode_handle * > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > struct fwnode_handle *child) > { > return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), > to_of_node(child))); > } > > On ACPI we currently have .device_is_available() returning false but that > probably should be returning true instead (it's been virtually unused > previously). > > That makes fwnode_get_next_available_child_node() and > fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably > creating unavailable nodes would be useless on swnode, too. > > So my question is: what do we gain by adding all these fwnode_*available() > helpers? > >> struct fwnode_handle *device_get_next_child_node(const struct device *dev, >> struct fwnode_handle *child); > Hi Sakari, thanks for your feedback. I thought that the difference is not in OF (which either way ends up calling __of_device_is_available()), but in ACPI. For fwnode_for_each_child_node(), the ACPI callback is acpi_get_next_subnode(), and I don't see that the device_is_available() callback is used in that case. For fwnode_for_each_available_child_node(), fwnode_get_next_available_child_node() is used, which checks fwnode_device_is_available(), which then calls device_is_available(). What's the catch? Thanks again and best regards, Javier Carrasco
Hi Javier, On Fri, Oct 11, 2024 at 10:34:32AM +0200, Javier Carrasco wrote: > On 11/10/2024 07:39, Sakari Ailus wrote: > > Hi Javier, > > > > On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote: > >> Introduce the scoped variant of the > >> fwnode_for_each_available_child_node() to automatically decrement the > >> child's refcount when it goes out of scope, removing the need for > >> explicit calls to fwnode_handle_put(). > >> > >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > >> --- > >> include/linux/property.h | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/include/linux/property.h b/include/linux/property.h > >> index 61fc20e5f81f..b37508ecf606 100644 > >> --- a/include/linux/property.h > >> +++ b/include/linux/property.h > >> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node( > >> for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\ > >> child = fwnode_get_next_available_child_node(fwnode, child)) > >> > >> +#define fwnode_for_each_available_child_node_scoped(fwnode, child) \ > >> + for (struct fwnode_handle *child __free(fwnode_handle) = \ > >> + fwnode_get_next_available_child_node(fwnode, NULL); child; \ > >> + child = fwnode_get_next_available_child_node(fwnode, child)) > >> + > > > > On OF, the implementation of the .get_next_child_node() fwnode op is: > > > > static struct fwnode_handle * > > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > > struct fwnode_handle *child) > > { > > return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), > > to_of_node(child))); > > } > > > > On ACPI we currently have .device_is_available() returning false but that > > probably should be returning true instead (it's been virtually unused > > previously). > > > > That makes fwnode_get_next_available_child_node() and > > fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably > > creating unavailable nodes would be useless on swnode, too. > > > > So my question is: what do we gain by adding all these fwnode_*available() > > helpers? > > > >> struct fwnode_handle *device_get_next_child_node(const struct device *dev, > >> struct fwnode_handle *child); > > > > Hi Sakari, thanks for your feedback. > > I thought that the difference is not in OF (which either way ends up > calling __of_device_is_available()), but in ACPI. > > For fwnode_for_each_child_node(), the ACPI callback is > acpi_get_next_subnode(), and I don't see that the device_is_available() > callback is used in that case. fwnode_get_next_available_child_node() also calls fwnode_device_is_available() and that returns false on all non-device nodes right now. As noted above, fwnode_device_is_available() should probably return true for non-device nodes on ACPI. I'll post a patch. > > For fwnode_for_each_available_child_node(), > fwnode_get_next_available_child_node() is used, which checks > fwnode_device_is_available(), which then calls device_is_available(). > > What's the catch?
On 11/10/2024 11:54, Sakari Ailus wrote: > Hi Javier, > > On Fri, Oct 11, 2024 at 10:34:32AM +0200, Javier Carrasco wrote: >> On 11/10/2024 07:39, Sakari Ailus wrote: >>> Hi Javier, >>> >>> On Tue, Oct 08, 2024 at 06:10:27PM +0200, Javier Carrasco wrote: >>>> Introduce the scoped variant of the >>>> fwnode_for_each_available_child_node() to automatically decrement the >>>> child's refcount when it goes out of scope, removing the need for >>>> explicit calls to fwnode_handle_put(). >>>> >>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >>>> --- >>>> include/linux/property.h | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/include/linux/property.h b/include/linux/property.h >>>> index 61fc20e5f81f..b37508ecf606 100644 >>>> --- a/include/linux/property.h >>>> +++ b/include/linux/property.h >>>> @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node( >>>> for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\ >>>> child = fwnode_get_next_available_child_node(fwnode, child)) >>>> >>>> +#define fwnode_for_each_available_child_node_scoped(fwnode, child) \ >>>> + for (struct fwnode_handle *child __free(fwnode_handle) = \ >>>> + fwnode_get_next_available_child_node(fwnode, NULL); child; \ >>>> + child = fwnode_get_next_available_child_node(fwnode, child)) >>>> + >>> >>> On OF, the implementation of the .get_next_child_node() fwnode op is: >>> >>> static struct fwnode_handle * >>> of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode, >>> struct fwnode_handle *child) >>> { >>> return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode), >>> to_of_node(child))); >>> } >>> >>> On ACPI we currently have .device_is_available() returning false but that >>> probably should be returning true instead (it's been virtually unused >>> previously). >>> >>> That makes fwnode_get_next_available_child_node() and >>> fwnode_get_next_child_node() equivalent on both ACPI and OF. Presumably >>> creating unavailable nodes would be useless on swnode, too. >>> >>> So my question is: what do we gain by adding all these fwnode_*available() >>> helpers? >>> >>>> struct fwnode_handle *device_get_next_child_node(const struct device *dev, >>>> struct fwnode_handle *child); >>> >> >> Hi Sakari, thanks for your feedback. >> >> I thought that the difference is not in OF (which either way ends up >> calling __of_device_is_available()), but in ACPI. >> >> For fwnode_for_each_child_node(), the ACPI callback is >> acpi_get_next_subnode(), and I don't see that the device_is_available() >> callback is used in that case. > > fwnode_get_next_available_child_node() also calls > fwnode_device_is_available() and that returns false on all non-device nodes > right now. As noted above, fwnode_device_is_available() should probably > return true for non-device nodes on ACPI. I'll post a patch. > fwnode_device_is_available() is indeed called in fwnode_get_next_available_child_node(), as I stated a couple of lines below. My question on the other hand was how that is called in fwnode_for_each_child_node(), as I could not see any call to check availability in acpi_get_next_subnode(). That is what confused me about the _available_ macros being the same as their counterparts without the _available_. Could you please clarify that? Thanks again. >> >> For fwnode_for_each_available_child_node(), >> fwnode_get_next_available_child_node() is used, which checks >> fwnode_device_is_available(), which then calls device_is_available(). >> >> What's the catch? >
diff --git a/include/linux/property.h b/include/linux/property.h index 61fc20e5f81f..b37508ecf606 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -168,6 +168,11 @@ struct fwnode_handle *fwnode_get_next_available_child_node( for (child = fwnode_get_next_available_child_node(fwnode, NULL); child;\ child = fwnode_get_next_available_child_node(fwnode, child)) +#define fwnode_for_each_available_child_node_scoped(fwnode, child) \ + for (struct fwnode_handle *child __free(fwnode_handle) = \ + fwnode_get_next_available_child_node(fwnode, NULL); child; \ + child = fwnode_get_next_available_child_node(fwnode, child)) + struct fwnode_handle *device_get_next_child_node(const struct device *dev, struct fwnode_handle *child);
Introduce the scoped variant of the fwnode_for_each_available_child_node() to automatically decrement the child's refcount when it goes out of scope, removing the need for explicit calls to fwnode_handle_put(). Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- include/linux/property.h | 5 +++++ 1 file changed, 5 insertions(+)