[PATCH 0/2] device property: introduce fwnode_for_each_available_child_node_scoped()

[PATCH 0/2] device property: introduce fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

The _scoped() version of the fwnode_for_each_available_child_node()
follows the approach recently taken for other loops that handle child
nodes like for_each_child_of_node_scoped() or
device_for_each_child_node_scoped(), which are based on the __free()
auto cleanup handler to remove the need for fwnode_handle_put() on
early loop exits.

This new variant has been tested with the LTC2992, which currently uses
the non-scoped variant. There is one error path that does not decrement
the refcount of the child node, which can be fixed by using the new
macro. The bug was introduced in a later modification of the loop, which
shows how useful an automatic cleanup solution can be in many uses of
the non-scoped version.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      device property: introduce fwnode_for_each_available_child_node_scoped()
      hwmon: (ltc2992) use fwnode_for_each_available_child_node_scoped()

 drivers/hwmon/ltc2992.c  | 11 +++--------
 include/linux/property.h |  5 +++++
 2 files changed, 8 insertions(+), 8 deletions(-)
---
base-commit: 124cfbcd6d185d4f50be02d5f5afe61578916773
change-id: 20240521-fwnode_for_each_available_child_node_scoped-8f1f09d3a10c

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/2] device property: introduce fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

Add a scoped version of fwnode_for_each_available_child_node() following
the approach recently taken for other loops that handle child nodes like
for_each_child_of_node_scoped() or device_for_each_child_node_scoped(),
which are based on the __free() auto cleanup handler to remove the need
for fwnode_handle_put() on early loop exits.

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..bcc3dda5a9d8 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);
 

-- 
2.40.1

[PATCH 2/2] hwmon: (ltc2992) use fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

The error path from a zero value of the "shunt-resistor-micro-ohms"
property does not decrement the refcount of the child node.

Instead of adding the missing fwnode_handle_put(), a safer fix for
future modifications is using the _scoped version of the macro,
which removes the need for fwnode_handle_put() in all error paths.

The macro defines the child node internally, which removes the need for
the current child node declaration as well.

Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/ltc2992.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
index 229aed15d5ca..3feee400ecf8 100644
--- a/drivers/hwmon/ltc2992.c
+++ b/drivers/hwmon/ltc2992.c
@@ -855,24 +855,19 @@ static const struct regmap_config ltc2992_regmap_config = {
 static int ltc2992_parse_dt(struct ltc2992_state *st)
 {
 	struct fwnode_handle *fwnode;
-	struct fwnode_handle *child;
 	u32 addr;
 	u32 val;
 	int ret;
 
 	fwnode = dev_fwnode(&st->client->dev);
 
-	fwnode_for_each_available_child_node(fwnode, child) {
+	fwnode_for_each_available_child_node_scoped(fwnode, child) {
 		ret = fwnode_property_read_u32(child, "reg", &addr);
-		if (ret < 0) {
-			fwnode_handle_put(child);
+		if (ret < 0)
 			return ret;
-		}
 
-		if (addr > 1) {
-			fwnode_handle_put(child);
+		if (addr > 1)
 			return -EINVAL;
-		}
 
 		ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
 		if (!ret) {

-- 
2.40.1

Re: [PATCH 1/2] device property: introduce fwnode_for_each_available_child_node_scoped()

[Andy Shevchenko]

On Wed, May 22, 2024 at 11:18:07AM +0200, Javier Carrasco wrote:
> Add a scoped version of fwnode_for_each_available_child_node() following
> the approach recently taken for other loops that handle child nodes like
> for_each_child_of_node_scoped() or device_for_each_child_node_scoped(),
> which are based on the __free() auto cleanup handler to remove the need
> for fwnode_handle_put() on early loop exits.

OK as long as we have users.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] hwmon: (ltc2992) use fwnode_for_each_available_child_node_scoped()

[Andy Shevchenko]

On Wed, May 22, 2024 at 11:18:08AM +0200, Javier Carrasco wrote:
> The error path from a zero value of the "shunt-resistor-micro-ohms"
> property does not decrement the refcount of the child node.
> 
> Instead of adding the missing fwnode_handle_put(), a safer fix for
> future modifications is using the _scoped version of the macro,
> which removes the need for fwnode_handle_put() in all error paths.

Not sure if the fix for easier backporting is better, up to Guenter and Greg.
If the above is the case, you will need

1) backportable fix;
2) the patch 1 as of this series;
3) conversion patch.

> The macro defines the child node internally, which removes the need for
> the current child node declaration as well.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
in case it will go the current way.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] device property: introduce fwnode_for_each_available_child_node_scoped()

[Sakari Ailus]

Hi Javier,

On Wed, May 22, 2024 at 11:18:07AM +0200, Javier Carrasco wrote:
> Add a scoped version of fwnode_for_each_available_child_node() following
> the approach recently taken for other loops that handle child nodes like
> for_each_child_of_node_scoped() or device_for_each_child_node_scoped(),
> which are based on the __free() auto cleanup handler to remove the need
> for fwnode_handle_put() on early loop exits.
> 
> 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..bcc3dda5a9d8 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);
>  

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH 2/2] hwmon: (ltc2992) use fwnode_for_each_available_child_node_scoped()

[Guenter Roeck]

On 5/22/24 02:18, Javier Carrasco wrote:
> The error path from a zero value of the "shunt-resistor-micro-ohms"
> property does not decrement the refcount of the child node.
> 
> Instead of adding the missing fwnode_handle_put(), a safer fix for
> future modifications is using the _scoped version of the macro,
> which removes the need for fwnode_handle_put() in all error paths.
> 
> The macro defines the child node internally, which removes the need for
> the current child node declaration as well.
> 
> Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

I really don't like fixes which depend on an API change.

Guenter

> ---
>   drivers/hwmon/ltc2992.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> index 229aed15d5ca..3feee400ecf8 100644
> --- a/drivers/hwmon/ltc2992.c
> +++ b/drivers/hwmon/ltc2992.c
> @@ -855,24 +855,19 @@ static const struct regmap_config ltc2992_regmap_config = {
>   static int ltc2992_parse_dt(struct ltc2992_state *st)
>   {
>   	struct fwnode_handle *fwnode;
> -	struct fwnode_handle *child;
>   	u32 addr;
>   	u32 val;
>   	int ret;
>   
>   	fwnode = dev_fwnode(&st->client->dev);
>   
> -	fwnode_for_each_available_child_node(fwnode, child) {
> +	fwnode_for_each_available_child_node_scoped(fwnode, child) {
>   		ret = fwnode_property_read_u32(child, "reg", &addr);
> -		if (ret < 0) {
> -			fwnode_handle_put(child);
> +		if (ret < 0)
>   			return ret;
> -		}
>   
> -		if (addr > 1) {
> -			fwnode_handle_put(child);
> +		if (addr > 1)
>   			return -EINVAL;
> -		}
>   
>   		ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
>   		if (!ret) {
> 

Re: [PATCH 2/2] hwmon: (ltc2992) use fwnode_for_each_available_child_node_scoped()

[Javier Carrasco]

On 22/05/2024 21:08, Guenter Roeck wrote:
> On 5/22/24 02:18, Javier Carrasco wrote:
>> The error path from a zero value of the "shunt-resistor-micro-ohms"
>> property does not decrement the refcount of the child node.
>>
>> Instead of adding the missing fwnode_handle_put(), a safer fix for
>> future modifications is using the _scoped version of the macro,
>> which removes the need for fwnode_handle_put() in all error paths.
>>
>> The macro defines the child node internally, which removes the need for
>> the current child node declaration as well.
>>
>> Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero")
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> 
> I really don't like fixes which depend on an API change.
> 
> Guenter
> 

We could split the fix into two steps as suggested by Andy, where the
fix would simply add the missing fwnode_handle_put(), adding the
corresponding tag to include it in the stable kernels (the bug was
introduced with v6.7). By doing that, the new macro would not need to be
backported, which is probably a safer approach.

The patch with the new macro would be left as it is, and another patch
would convert the the loop to use the new macro to avoid that a similar
bug arises again. That would be this 2/2 + dropping the additional
fwnode_handle_put().

Best regards,
Javier Carrasco

Re: [PATCH 2/2] hwmon: (ltc2992) use fwnode_for_each_available_child_node_scoped()

[Guenter Roeck]

On 5/22/24 12:46, Javier Carrasco wrote:
> On 22/05/2024 21:08, Guenter Roeck wrote:
>> On 5/22/24 02:18, Javier Carrasco wrote:
>>> The error path from a zero value of the "shunt-resistor-micro-ohms"
>>> property does not decrement the refcount of the child node.
>>>
>>> Instead of adding the missing fwnode_handle_put(), a safer fix for
>>> future modifications is using the _scoped version of the macro,
>>> which removes the need for fwnode_handle_put() in all error paths.
>>>
>>> The macro defines the child node internally, which removes the need for
>>> the current child node declaration as well.
>>>
>>> Fixes: 10b029020487 ("hwmon: (ltc2992) Avoid division by zero")
>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>
>> I really don't like fixes which depend on an API change.
>>
>> Guenter
>>
> 
> We could split the fix into two steps as suggested by Andy, where the
> fix would simply add the missing fwnode_handle_put(), adding the
> corresponding tag to include it in the stable kernels (the bug was
> introduced with v6.7). By doing that, the new macro would not need to be
> backported, which is probably a safer approach.
> 
> The patch with the new macro would be left as it is, and another patch
> would convert the the loop to use the new macro to avoid that a similar
> bug arises again. That would be this 2/2 + dropping the additional
> fwnode_handle_put().
> 

Please do.

Thanks,
Guenter


> Best regards,
> Javier Carrasco
>