[v1] device property: Add scoped fwnode child node iterators

[PATCH] device property: Add scoped fwnode child node iterators

Posted by Jean-François Lessard 1 month ago

Add scoped versions of fwnode child node iterators that automatically
handle reference counting cleanup using the __free() attribute:

- fwnode_for_each_child_node_scoped()
- fwnode_for_each_named_child_node_scoped()
- fwnode_for_each_available_child_node_scoped()

These macros follow the same pattern as existing scoped iterators in the
kernel, ensuring fwnode references are automatically released when the
iterator variable goes out of scope. This prevents resource leaks and
eliminates the need for manual cleanup in error paths.

The implementation mirrors the non-scoped variants but uses
__free(fwnode_handle) for automatic resource management, providing a safer
and more convenient interface for drivers iterating over firmware node
children.

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---

Notes:
    checkpatch reports false positives that are intentionally ignored:
    COMPLEX_MACRO, MACRO_ARG_REUSE, MACRO_ARG_PRECEDENCE
    This is a standard iterator pattern following kernel conventions.

 include/linux/property.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 82f0cb3ab..279c244db 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -176,6 +176,20 @@ 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_child_node_scoped(fwnode, child)		\
+	for (struct fwnode_handle *child __free(fwnode_handle) =	\
+		fwnode_get_next_child_node(fwnode, NULL);		\
+	     child; child = fwnode_get_next_child_node(fwnode, child))
+
+#define fwnode_for_each_named_child_node_scoped(fwnode, child, name)	\
+	fwnode_for_each_child_node_scoped(fwnode, child)		\
+		for_each_if(fwnode_name_eq(child, name))
+
+#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);
 
-- 
2.43.0

Re: [PATCH] device property: Add scoped fwnode child node iterators

Posted by Greg Kroah-Hartman 1 month ago

On Thu, Aug 28, 2025 at 10:17:59PM -0400, Jean-François Lessard wrote:
> Add scoped versions of fwnode child node iterators that automatically
> handle reference counting cleanup using the __free() attribute:
> 
> - fwnode_for_each_child_node_scoped()
> - fwnode_for_each_named_child_node_scoped()
> - fwnode_for_each_available_child_node_scoped()
> 
> These macros follow the same pattern as existing scoped iterators in the
> kernel, ensuring fwnode references are automatically released when the
> iterator variable goes out of scope. This prevents resource leaks and
> eliminates the need for manual cleanup in error paths.
> 
> The implementation mirrors the non-scoped variants but uses
> __free(fwnode_handle) for automatic resource management, providing a safer
> and more convenient interface for drivers iterating over firmware node
> children.
> 
> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> ---
> 
> Notes:
>     checkpatch reports false positives that are intentionally ignored:
>     COMPLEX_MACRO, MACRO_ARG_REUSE, MACRO_ARG_PRECEDENCE
>     This is a standard iterator pattern following kernel conventions.
> 
>  include/linux/property.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 82f0cb3ab..279c244db 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -176,6 +176,20 @@ 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_child_node_scoped(fwnode, child)		\
> +	for (struct fwnode_handle *child __free(fwnode_handle) =	\
> +		fwnode_get_next_child_node(fwnode, NULL);		\
> +	     child; child = fwnode_get_next_child_node(fwnode, child))
> +
> +#define fwnode_for_each_named_child_node_scoped(fwnode, child, name)	\
> +	fwnode_for_each_child_node_scoped(fwnode, child)		\
> +		for_each_if(fwnode_name_eq(child, name))
> +
> +#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);
>  

We need a real user of this before we can add them, so please do that as
part of a patch series.

thanks

greg k-h

Re: [PATCH] device property: Add scoped fwnode child node iterators

Posted by Jean-François Lessard 1 month ago

Le 29 août 2025 00 h 23 min 22 s HAE, Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit :
>On Thu, Aug 28, 2025 at 10:17:59PM -0400, Jean-François Lessard wrote:
>> Add scoped versions of fwnode child node iterators that automatically
>> handle reference counting cleanup using the __free() attribute:
>> 
>> - fwnode_for_each_child_node_scoped()
>> - fwnode_for_each_named_child_node_scoped()
>> - fwnode_for_each_available_child_node_scoped()
>> 
>> These macros follow the same pattern as existing scoped iterators in the
>> kernel, ensuring fwnode references are automatically released when the
>> iterator variable goes out of scope. This prevents resource leaks and
>> eliminates the need for manual cleanup in error paths.
>> 
>> The implementation mirrors the non-scoped variants but uses
>> __free(fwnode_handle) for automatic resource management, providing a safer
>> and more convenient interface for drivers iterating over firmware node
>> children.
>> 
>> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
>> ---
>> 
>> Notes:
>>     checkpatch reports false positives that are intentionally ignored:
>>     COMPLEX_MACRO, MACRO_ARG_REUSE, MACRO_ARG_PRECEDENCE
>>     This is a standard iterator pattern following kernel conventions.
>> 
>>  include/linux/property.h | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/include/linux/property.h b/include/linux/property.h
>> index 82f0cb3ab..279c244db 100644
>> --- a/include/linux/property.h
>> +++ b/include/linux/property.h
>> @@ -176,6 +176,20 @@ 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_child_node_scoped(fwnode, child)		\
>> +	for (struct fwnode_handle *child __free(fwnode_handle) =	\
>> +		fwnode_get_next_child_node(fwnode, NULL);		\
>> +	     child; child = fwnode_get_next_child_node(fwnode, child))
>> +
>> +#define fwnode_for_each_named_child_node_scoped(fwnode, child, name)	\
>> +	fwnode_for_each_child_node_scoped(fwnode, child)		\
>> +		for_each_if(fwnode_name_eq(child, name))
>> +
>> +#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);
>>  
>
>We need a real user of this before we can add them, so please do that as
>part of a patch series.
>

I understand the "no dead code" policy, but I found existing manual
implementations of this exact pattern in the current kernel.

For example, drivers/i2c/i2c-core-slave.c already does:

		struct fwnode_handle *child __free(fwnode_handle) = NULL;
...
		fwnode_for_each_child_node(fwnode, child) {
...
		}

This suggests developers are already wanting this functionality but
implementing it manually.

If included with my driver series, I would only add the
fwnode_for_each_available_child_node_scoped() variant that I actually use,
avoiding any dead code.

Best Regards

Jean-François Lessard

Re: [PATCH] device property: Add scoped fwnode child node iterators

Posted by Andy Shevchenko 1 month ago

On Fri, Aug 29, 2025 at 07:34:17AM -0400, Jean-François Lessard wrote:
> Le 29 août 2025 00 h 23 min 22 s HAE, Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit :
> >On Thu, Aug 28, 2025 at 10:17:59PM -0400, Jean-François Lessard wrote:

...

> >We need a real user of this before we can add them, so please do that as
> >part of a patch series.
> >
> 
> I understand the "no dead code" policy, but I found existing manual
> implementations of this exact pattern in the current kernel.
> 
> For example, drivers/i2c/i2c-core-slave.c already does:
> 
> 		struct fwnode_handle *child __free(fwnode_handle) = NULL;
> ...
> 		fwnode_for_each_child_node(fwnode, child) {
> ...
> 		}
> 
> This suggests developers are already wanting this functionality but
> implementing it manually.
> 
> If included with my driver series, I would only add the
> fwnode_for_each_available_child_node_scoped() variant that I actually use,

I fully agree with Greg, please make it series out of two, I will review both
as soon as it appears in my mailbox.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] device property: Add scoped fwnode child node iterators

Posted by Greg Kroah-Hartman 1 month ago

On Fri, Aug 29, 2025 at 07:34:17AM -0400, Jean-François Lessard wrote:
> Le 29 août 2025 00 h 23 min 22 s HAE, Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit :
> >On Thu, Aug 28, 2025 at 10:17:59PM -0400, Jean-François Lessard wrote:
> >> Add scoped versions of fwnode child node iterators that automatically
> >> handle reference counting cleanup using the __free() attribute:
> >> 
> >> - fwnode_for_each_child_node_scoped()
> >> - fwnode_for_each_named_child_node_scoped()
> >> - fwnode_for_each_available_child_node_scoped()
> >> 
> >> These macros follow the same pattern as existing scoped iterators in the
> >> kernel, ensuring fwnode references are automatically released when the
> >> iterator variable goes out of scope. This prevents resource leaks and
> >> eliminates the need for manual cleanup in error paths.
> >> 
> >> The implementation mirrors the non-scoped variants but uses
> >> __free(fwnode_handle) for automatic resource management, providing a safer
> >> and more convenient interface for drivers iterating over firmware node
> >> children.
> >> 
> >> Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
> >> ---
> >> 
> >> Notes:
> >>     checkpatch reports false positives that are intentionally ignored:
> >>     COMPLEX_MACRO, MACRO_ARG_REUSE, MACRO_ARG_PRECEDENCE
> >>     This is a standard iterator pattern following kernel conventions.
> >> 
> >>  include/linux/property.h | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >> 
> >> diff --git a/include/linux/property.h b/include/linux/property.h
> >> index 82f0cb3ab..279c244db 100644
> >> --- a/include/linux/property.h
> >> +++ b/include/linux/property.h
> >> @@ -176,6 +176,20 @@ 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_child_node_scoped(fwnode, child)		\
> >> +	for (struct fwnode_handle *child __free(fwnode_handle) =	\
> >> +		fwnode_get_next_child_node(fwnode, NULL);		\
> >> +	     child; child = fwnode_get_next_child_node(fwnode, child))
> >> +
> >> +#define fwnode_for_each_named_child_node_scoped(fwnode, child, name)	\
> >> +	fwnode_for_each_child_node_scoped(fwnode, child)		\
> >> +		for_each_if(fwnode_name_eq(child, name))
> >> +
> >> +#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);
> >>  
> >
> >We need a real user of this before we can add them, so please do that as
> >part of a patch series.
> >
> 
> I understand the "no dead code" policy, but I found existing manual
> implementations of this exact pattern in the current kernel.
> 
> For example, drivers/i2c/i2c-core-slave.c already does:
> 
> 		struct fwnode_handle *child __free(fwnode_handle) = NULL;
> ...
> 		fwnode_for_each_child_node(fwnode, child) {
> ...
> 		}
> 
> This suggests developers are already wanting this functionality but
> implementing it manually.

Great, then add it here and fix up drivers/i2c/i2c-core-slave.c to use
it!

thanks,

greg k-h