[PATCH 0/6] use device_for_each_child_node() to access device child nodes

[PATCH 0/6] use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

This series aims to clarify the use cases of:

- device_for_each_child_node[_scoped]()
- fwnode_for_each_available_child_node[_scoped]()

to access firmware nodes.

There have been multiple discussions [1][2] about what the first macro
implies in the sense of availability, and a number of users have opted
for the second macro in cases where the first one should have been
preferred.

The second macro is intended to be used over child nodes of a firmware
node, not direct child nodes of the device node. Instead, those users
retrieve the fwnode member from the device struct just to have access to
a macro that explicitly indicates node availability.

That workaround is not necessary because `device_for_each_child_node()`
implies availability for the existing backends (ACPI, DT, swnode).

This series does not cover other points discussed in [2] like addressing
uses of `fwnode_for_each_child_node()` where `device_*` should have been
used, using the `_avaialble_` variant of the fwnode loop whenever
possible, or adding new `_scoped` macros. Such points will be covered by
subsequent series to keep focus on the "availability" issue.

The conversion has been validated with an LTC2992 hwmon sensor, which is
one of the affected drivers. The rest of the drivers could only be
compiled and checked with static-analysis tools.

Link: https://lore.kernel.org/all/20211205190101.26de4a57@jic23-huawei/ [1]
Link: https://lore.kernel.org/all/20240523-fwnode_for_each_available_child_node_scoped-v2-0-701f3a03f2fb@gmail.com/ [2]

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (6):
      device property: document device_for_each_child_node macro
      hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes
      leds: bd2606mvv: use device_for_each_child_node() to access device child nodes
      leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes
      leds: pca995x: use device_for_each_child_node() to access device child nodes
      net: mvpp2: use device_for_each_child_node() to access device child nodes

 drivers/hwmon/ltc2992.c                         | 19 ++++----------
 drivers/leds/leds-bd2606mvv.c                   |  7 +++--
 drivers/leds/leds-is31fl319x.c                  | 34 ++++++++-----------------
 drivers/leds/leds-pca995x.c                     | 15 ++++-------
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 +++-------
 include/linux/property.h                        | 10 ++++++++
 6 files changed, 38 insertions(+), 60 deletions(-)
---
base-commit: 0b58e108042b0ed28a71cd7edf5175999955b233
change-id: 20240701-device_for_each_child_node-available-1c1eca4b6495

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

[PATCH 1/6] device property: document device_for_each_child_node macro

[Javier Carrasco]

There have been some misconceptions about this macro, which iterates
over available child nodes from different backends.

As that is not obvious by its name, some users have opted for the
`fwnode_for_each_available_child_node()` macro instead.
That requires an unnecessary, explicit access to the fwnode member
of the device structure.

Passing the device to `device_for_each_child_node()` is shorter,
reflects more clearly the nature of the child nodes, and renders the
same result.

In general, `fwnode_for_each_available_child_node()` should be used
whenever the parent node of the children to iterate over is a firmware
node, and not the device itself.

Document the `device_for_each_child node(dev, child)` macro to clarify
its functionality.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 include/linux/property.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 61fc20e5f81f..ba8a3dc54786 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
 struct fwnode_handle *device_get_next_child_node(const struct device *dev,
 						 struct fwnode_handle *child);
 
+/**
+ * device_for_each_child_node - iterate over available child nodes of a device
+ * @dev: Pointer to the struct device
+ * @child: Pointer to an available child node in each loop iteration, if found
+ *
+ * Unavailable nodes are skipped i.e. this macro is implicitly _available_.
+ * The reference to the child node must be dropped on early exits.
+ * See fwnode_handle_put().
+ * For a scoped version of this macro, use device_for_each_child_node_scoped().
+ */
 #define device_for_each_child_node(dev, child)				\
 	for (child = device_get_next_child_node(dev, NULL); child;	\
 	     child = device_get_next_child_node(dev, child))

-- 
2.40.1

Re: [PATCH 1/6] device property: document device_for_each_child_node macro

[Jonathan Cameron]

On Sat, 06 Jul 2024 17:23:33 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> There have been some misconceptions about this macro, which iterates
> over available child nodes from different backends.
> 
> As that is not obvious by its name, some users have opted for the
> `fwnode_for_each_available_child_node()` macro instead.
> That requires an unnecessary, explicit access to the fwnode member
> of the device structure.
> 
> Passing the device to `device_for_each_child_node()` is shorter,
> reflects more clearly the nature of the child nodes, and renders the
> same result.
> 
> In general, `fwnode_for_each_available_child_node()` should be used
> whenever the parent node of the children to iterate over is a firmware
> node, and not the device itself.
> 
> Document the `device_for_each_child node(dev, child)` macro to clarify
> its functionality.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

LGTM but I think needs at least a DT and ACPI ack.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

One trivial tweak inline.

> ---
>  include/linux/property.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 61fc20e5f81f..ba8a3dc54786 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node(
>  struct fwnode_handle *device_get_next_child_node(const struct device *dev,
>  						 struct fwnode_handle *child);
>  
> +/**
> + * device_for_each_child_node - iterate over available child nodes of a device
> + * @dev: Pointer to the struct device
> + * @child: Pointer to an available child node in each loop iteration, if found

If it's not found then there are no loop iterations. So could drop the ,if found
I think.

> + *
> + * Unavailable nodes are skipped i.e. this macro is implicitly _available_.
> + * The reference to the child node must be dropped on early exits.
> + * See fwnode_handle_put().
> + * For a scoped version of this macro, use device_for_each_child_node_scoped().
> + */
>  #define device_for_each_child_node(dev, child)				\
>  	for (child = device_get_next_child_node(dev, NULL); child;	\
>  	     child = device_get_next_child_node(dev, child))
> 

[PATCH 2/6] hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

In this case, the child nodes are not required outside the loop, and
the scoped version of the macro can be used to remove the repetitive
`goto put` pattern.

Use `device_for_each_child_node_scoped_scoped()` to indicate device's
direct child nodes.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/ltc2992.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
index d4a93223cd3b..541fa09dc6e7 100644
--- a/drivers/hwmon/ltc2992.c
+++ b/drivers/hwmon/ltc2992.c
@@ -854,33 +854,24 @@ 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) {
+	device_for_each_child_node_scoped(&st->client->dev, 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) {
-			if (!val) {
-				fwnode_handle_put(child);
+			if (!val)
 				return dev_err_probe(&st->client->dev, -EINVAL,
 						     "shunt resistor value cannot be zero\n");
-			}
+
 			st->r_sense_uohm[addr] = val;
 		}
 	}

-- 
2.40.1

Re: [PATCH 2/6] hwmon: (ltc2992) use device_for_each_child_node_scoped() to access child nodes

[Jonathan Cameron]

On Sat, 06 Jul 2024 17:23:34 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> In this case, the child nodes are not required outside the loop, and
> the scoped version of the macro can be used to remove the repetitive
> `goto put` pattern.
> 
> Use `device_for_each_child_node_scoped_scoped()` to indicate device's
> direct child nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
One thing that is possibly a follow up suggestion but could maybe
be done in this patch.  Either way.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/hwmon/ltc2992.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> index d4a93223cd3b..541fa09dc6e7 100644
> --- a/drivers/hwmon/ltc2992.c
> +++ b/drivers/hwmon/ltc2992.c
> @@ -854,33 +854,24 @@ 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;

I'd take the opportunity for
	struct device *dev = &st->client->dev;

Given there are several users and this modifies one of them.

>  	u32 addr;
>  	u32 val;
>  	int ret;
>  
> -	fwnode = dev_fwnode(&st->client->dev);
> -
> -	fwnode_for_each_available_child_node(fwnode, child) {
> +	device_for_each_child_node_scoped(&st->client->dev, 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) {
> -			if (!val) {
> -				fwnode_handle_put(child);
> +			if (!val)
>  				return dev_err_probe(&st->client->dev, -EINVAL,
>  						     "shunt resistor value cannot be zero\n");
> -			}
> +
>  			st->r_sense_uohm[addr] = val;
>  		}
>  	}
> 

[PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

Use `device_for_each_child_node()` to indicate device's direct child
nodes.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-bd2606mvv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
index 3fda712d2f80..4f38b7b4d9d1 100644
--- a/drivers/leds/leds-bd2606mvv.c
+++ b/drivers/leds/leds-bd2606mvv.c
@@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = {
 
 static int bd2606mvv_probe(struct i2c_client *client)
 {
-	struct fwnode_handle *np, *child;
+	struct fwnode_handle *child;
 	struct device *dev = &client->dev;
 	struct bd2606mvv_priv *priv;
 	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
@@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
 	int err, reg;
 	int i;
 
-	np = dev_fwnode(dev);
-	if (!np)
+	if (!dev_fwnode(dev))
 		return -ENODEV;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, priv);
 
-	fwnode_for_each_available_child_node(np, child) {
+	device_for_each_child_node(dev, child) {
 		struct bd2606mvv_led *led;
 
 		err = fwnode_property_read_u32(child, "reg", &reg);

-- 
2.40.1

Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Jonathan Cameron]

On Sat, 06 Jul 2024 17:23:35 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> Use `device_for_each_child_node()` to indicate device's direct child
> nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Why not the scoped variant?
There look to be two error paths in there which would be simplified.

> ---
>  drivers/leds/leds-bd2606mvv.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
> index 3fda712d2f80..4f38b7b4d9d1 100644
> --- a/drivers/leds/leds-bd2606mvv.c
> +++ b/drivers/leds/leds-bd2606mvv.c
> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = {
>  
>  static int bd2606mvv_probe(struct i2c_client *client)
>  {
> -	struct fwnode_handle *np, *child;
> +	struct fwnode_handle *child;
>  	struct device *dev = &client->dev;
>  	struct bd2606mvv_priv *priv;
>  	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>  	int err, reg;
>  	int i;
>  
> -	np = dev_fwnode(dev);
> -	if (!np)
> +	if (!dev_fwnode(dev))
>  		return -ENODEV;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>  
>  	i2c_set_clientdata(client, priv);
>  
> -	fwnode_for_each_available_child_node(np, child) {
> +	device_for_each_child_node(dev, child) {
>  		struct bd2606mvv_led *led;
>  
>  		err = fwnode_property_read_u32(child, "reg", &reg);
> 

Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

On 07/07/2024 18:57, Jonathan Cameron wrote:
> On Sat, 06 Jul 2024 17:23:35 +0200
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> 
>> The iterated nodes are direct children of the device node, and the
>> `device_for_each_child_node()` macro accounts for child node
>> availability.
>>
>> `fwnode_for_each_available_child_node()` is meant to access the child
>> nodes of an fwnode, and therefore not direct child nodes of the device
>> node.
>>
>> Use `device_for_each_child_node()` to indicate device's direct child
>> nodes.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Why not the scoped variant?
> There look to be two error paths in there which would be simplified.
> 

I did not use the scoped variant because "child" is used outside the loop.

On the other hand, I think an fwnode_handle_get() is missing for every
"led_fwnodes[reg] = child" because a simple assignment does not
increment the refcount.

After adding fwnode_handle_get(), the scoped variant could be used, and
the call to fwnode_handle_put() would act on led_fwnodes[reg] instead.

>> ---
>>  drivers/leds/leds-bd2606mvv.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
>> index 3fda712d2f80..4f38b7b4d9d1 100644
>> --- a/drivers/leds/leds-bd2606mvv.c
>> +++ b/drivers/leds/leds-bd2606mvv.c
>> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = {
>>  
>>  static int bd2606mvv_probe(struct i2c_client *client)
>>  {
>> -	struct fwnode_handle *np, *child;
>> +	struct fwnode_handle *child;
>>  	struct device *dev = &client->dev;
>>  	struct bd2606mvv_priv *priv;
>>  	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
>> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>  	int err, reg;
>>  	int i;
>>  
>> -	np = dev_fwnode(dev);
>> -	if (!np)
>> +	if (!dev_fwnode(dev))
>>  		return -ENODEV;
>>  
>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>  
>>  	i2c_set_clientdata(client, priv);
>>  
>> -	fwnode_for_each_available_child_node(np, child) {
>> +	device_for_each_child_node(dev, child) {
>>  		struct bd2606mvv_led *led;
>>  
>>  		err = fwnode_property_read_u32(child, "reg", &reg);
>>
> 

Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Jonathan Cameron]

No

On 8 July 2024 09:14:44 BST, Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>On 07/07/2024 18:57, Jonathan Cameron wrote:
>> On Sat, 06 Jul 2024 17:23:35 +0200
>> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>> 
>>> The iterated nodes are direct children of the device node, and the
>>> `device_for_each_child_node()` macro accounts for child node
>>> availability.
>>>
>>> `fwnode_for_each_available_child_node()` is meant to access the child
>>> nodes of an fwnode, and therefore not direct child nodes of the device
>>> node.
>>>
>>> Use `device_for_each_child_node()` to indicate device's direct child
>>> nodes.
>>>
>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> Why not the scoped variant?
>> There look to be two error paths in there which would be simplified.
>> 
>
>I did not use the scoped variant because "child" is used outside the loop.

Ah missed that.  Good sign that things are wrong...

>
>On the other hand, I think an fwnode_handle_get() is missing for every
>"led_fwnodes[reg] = child" because a simple assignment does not
>increment the refcount.

Yes. Looks like a bug to me as well.


>
>After adding fwnode_handle_get(), the scoped variant could be used, and
>the call to fwnode_handle_put() would act on led_fwnodes[reg] instead.

There looks to be another bug as it only frees one handle on error.  Right now it shouldnt free any but once you fix that you will need to free any not freed otherwise.

Can it be squashed into one loop?

J


>
>>> ---
>>>  drivers/leds/leds-bd2606mvv.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
>>> index 3fda712d2f80..4f38b7b4d9d1 100644
>>> --- a/drivers/leds/leds-bd2606mvv.c
>>> +++ b/drivers/leds/leds-bd2606mvv.c
>>> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = {
>>>  
>>>  static int bd2606mvv_probe(struct i2c_client *client)
>>>  {
>>> -	struct fwnode_handle *np, *child;
>>> +	struct fwnode_handle *child;
>>>  	struct device *dev = &client->dev;
>>>  	struct bd2606mvv_priv *priv;
>>>  	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
>>> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>>  	int err, reg;
>>>  	int i;
>>>  
>>> -	np = dev_fwnode(dev);
>>> -	if (!np)
>>> +	if (!dev_fwnode(dev))
>>>  		return -ENODEV;
>>>  
>>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>>  
>>>  	i2c_set_clientdata(client, priv);
>>>  
>>> -	fwnode_for_each_available_child_node(np, child) {
>>> +	device_for_each_child_node(dev, child) {
>>>  		struct bd2606mvv_led *led;
>>>  
>>>  		err = fwnode_property_read_u32(child, "reg", &reg);
>>>
>> 
>

Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

On 08/07/2024 10:14, Javier Carrasco wrote:
> On 07/07/2024 18:57, Jonathan Cameron wrote:
>> On Sat, 06 Jul 2024 17:23:35 +0200
>> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>>
>>> The iterated nodes are direct children of the device node, and the
>>> `device_for_each_child_node()` macro accounts for child node
>>> availability.
>>>
>>> `fwnode_for_each_available_child_node()` is meant to access the child
>>> nodes of an fwnode, and therefore not direct child nodes of the device
>>> node.
>>>
>>> Use `device_for_each_child_node()` to indicate device's direct child
>>> nodes.
>>>
>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> Why not the scoped variant?
>> There look to be two error paths in there which would be simplified.
>>
> 
> I did not use the scoped variant because "child" is used outside the loop.
> 
> On the other hand, I think an fwnode_handle_get() is missing for every
> "led_fwnodes[reg] = child" because a simple assignment does not
> increment the refcount.
> 
> After adding fwnode_handle_get(), the scoped variant could be used, and
> the call to fwnode_handle_put() would act on led_fwnodes[reg] instead.
> 

Actually, the whole trouble comes from doing the processing in two
consecutive loops, where the second loop accesses a child node that gets
released at the end of the first one. It seems that some code got moved
from one loop to a new one between two versions of the patchset.

@Andreas Kemnade: I see that you had a single loop until v4 (the driver
got applied with v6), and then you split it into two loops, but I don't
see any mention to this modification in the change log.

What was the reason for this modification? Apparently, similar drivers
do everything in one loop to avoid such issues.

Maybe refactoring to have a single loop again (if possible) would be the
cleanest solution. Otherwise a get/put mechanism might be necessary.

>>> ---
>>>  drivers/leds/leds-bd2606mvv.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
>>> index 3fda712d2f80..4f38b7b4d9d1 100644
>>> --- a/drivers/leds/leds-bd2606mvv.c
>>> +++ b/drivers/leds/leds-bd2606mvv.c
>>> @@ -69,7 +69,7 @@ static const struct regmap_config bd2606mvv_regmap = {
>>>  
>>>  static int bd2606mvv_probe(struct i2c_client *client)
>>>  {
>>> -	struct fwnode_handle *np, *child;
>>> +	struct fwnode_handle *child;
>>>  	struct device *dev = &client->dev;
>>>  	struct bd2606mvv_priv *priv;
>>>  	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
>>> @@ -77,8 +77,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>>  	int err, reg;
>>>  	int i;
>>>  
>>> -	np = dev_fwnode(dev);
>>> -	if (!np)
>>> +	if (!dev_fwnode(dev))
>>>  		return -ENODEV;
>>>  
>>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> @@ -94,7 +93,7 @@ static int bd2606mvv_probe(struct i2c_client *client)
>>>  
>>>  	i2c_set_clientdata(client, priv);
>>>  
>>> -	fwnode_for_each_available_child_node(np, child) {
>>> +	device_for_each_child_node(dev, child) {
>>>  		struct bd2606mvv_led *led;
>>>  
>>>  		err = fwnode_property_read_u32(child, "reg", &reg);
>>>
>>
> 

Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Andreas Kemnade]

On Mon, 8 Jul 2024 17:45:43 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 08/07/2024 10:14, Javier Carrasco wrote:
> > On 07/07/2024 18:57, Jonathan Cameron wrote:  
> >> On Sat, 06 Jul 2024 17:23:35 +0200
> >> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >>  
> >>> The iterated nodes are direct children of the device node, and the
> >>> `device_for_each_child_node()` macro accounts for child node
> >>> availability.
> >>>
> >>> `fwnode_for_each_available_child_node()` is meant to access the
> >>> child nodes of an fwnode, and therefore not direct child nodes of
> >>> the device node.
> >>>
> >>> Use `device_for_each_child_node()` to indicate device's direct
> >>> child nodes.
> >>>
> >>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> >> Why not the scoped variant?
> >> There look to be two error paths in there which would be
> >> simplified. 
> > 
> > I did not use the scoped variant because "child" is used outside
> > the loop.
> > 
> > On the other hand, I think an fwnode_handle_get() is missing for
> > every "led_fwnodes[reg] = child" because a simple assignment does
> > not increment the refcount.
> > 
> > After adding fwnode_handle_get(), the scoped variant could be used,
> > and the call to fwnode_handle_put() would act on led_fwnodes[reg]
> > instead. 
> 
> Actually, the whole trouble comes from doing the processing in two
> consecutive loops, where the second loop accesses a child node that
> gets released at the end of the first one. It seems that some code
> got moved from one loop to a new one between two versions of the
> patchset.
> 
> @Andreas Kemnade: I see that you had a single loop until v4 (the
> driver got applied with v6), and then you split it into two loops,
> but I don't see any mention to this modification in the change log.
> 
> What was the reason for this modification? Apparently, similar drivers
> do everything in one loop to avoid such issues.
> 
The reason for two loops is that we check in the first loop whether
broghtness can be individually controlled so we can set max_brightness
in the second loop. I had the assumption that max_brightness should be
set before registering leds.

Some LEDs share brightness register, in the case where leds are defined
with a shared register, we revert to on-off.

> Maybe refactoring to have a single loop again (if possible) would be
> the cleanest solution. Otherwise a get/put mechanism might be
> necessary.
> 
I had no idea how to do it the time I wrote the patch.

Regards,
Andreas

Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

On 12/07/2024 23:06, Andreas Kemnade wrote:
> On Mon, 8 Jul 2024 17:45:43 +0200
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> 
>> On 08/07/2024 10:14, Javier Carrasco wrote:
>> What was the reason for this modification? Apparently, similar drivers
>> do everything in one loop to avoid such issues.
>>
> The reason for two loops is that we check in the first loop whether
> broghtness can be individually controlled so we can set max_brightness
> in the second loop. I had the assumption that max_brightness should be
> set before registering leds.
> 
> Some LEDs share brightness register, in the case where leds are defined
> with a shared register, we revert to on-off.
> 
>> Maybe refactoring to have a single loop again (if possible) would be
>> the cleanest solution. Otherwise a get/put mechanism might be
>> necessary.
>>
> I had no idea how to do it the time I wrote the patch.
> 
> Regards,
> Andreas

Then we could leave the two loops, and fix them. I am thinking of something
like this:

 static int bd2606mvv_probe(struct i2c_client *client)
 {
-	struct fwnode_handle *child;
 	struct device *dev = &client->dev;
 	struct bd2606mvv_priv *priv;
 	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
 	int active_pairs[BD2606_MAX_LEDS / 2] = { 0 };
 	int err, reg;
-	int i;
+	int i, j;

 	if (!dev_fwnode(dev))
 		return -ENODEV;
@@ -93,20 +92,18 @@ static int bd2606mvv_probe(struct i2c_client *client)

 	i2c_set_clientdata(client, priv);

-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct bd2606mvv_led *led;

 		err = fwnode_property_read_u32(child, "reg", &reg);
-		if (err) {
-			fwnode_handle_put(child);
+		if (err)
 			return err;
-		}
-		if (reg < 0 || reg >= BD2606_MAX_LEDS || led_fwnodes[reg]) {
-			fwnode_handle_put(child);
+
+		if (reg < 0 || reg >= BD2606_MAX_LEDS || led_fwnodes[reg])
 			return -EINVAL;
-		}
+
 		led = &priv->leds[reg];
-		led_fwnodes[reg] = child;
+		led_fwnodes[reg] = fwnode_handle_get(child);
 		active_pairs[reg / 2]++;
 		led->priv = priv;
 		led->led_no = reg;
@@ -129,7 +126,8 @@ static int bd2606mvv_probe(struct i2c_client *client)
 						     &priv->leds[i].ldev,
 						     &init_data);
 		if (err < 0) {
-			fwnode_handle_put(child);
+			for (j = i; j < BD2606_MAX_LEDS; j++)
+				fwnode_handle_put(led_fwnodes[j]);
 			return dev_err_probe(dev, err,
 					     "couldn't register LED %s\n",
 					     priv->leds[i].ldev.name);



Thanks to the call to fwnode_get_handle(child), the child nodes get their
refcount incremented to be used in the second loop, where all child nodes that
have not been registered are released in case of error.

The first loop becomes a scoped one, keeping the `child` variable from being
accessed anywhere else.

Any feedback before I send a v2 with this is very welcome.

Best regards,
Javier Carrasco

[PATCH 4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

In this case, the child nodes are not required outside the loop, and
the scoped version of the macro can be used to remove the repetitive
`goto put` pattern.

Use `device_for_each_child_node_scoped_scoped()` to indicate device's
direct child nodes.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-is31fl319x.c | 34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
index 5e1a4d39a107..27bfab3da479 100644
--- a/drivers/leds/leds-is31fl319x.c
+++ b/drivers/leds/leds-is31fl319x.c
@@ -392,7 +392,7 @@ static int is31fl319x_parse_child_fw(const struct device *dev,
 
 static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 {
-	struct fwnode_handle *fwnode = dev_fwnode(dev), *child;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	int count;
 	int ret;
 
@@ -404,7 +404,7 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 	is31->cdef = device_get_match_data(dev);
 
 	count = 0;
-	fwnode_for_each_available_child_node(fwnode, child)
+	device_for_each_child_node_scoped(dev, child)
 		count++;
 
 	dev_dbg(dev, "probing with %d leds defined in DT\n", count);
@@ -414,33 +414,25 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 				     "Number of leds defined must be between 1 and %u\n",
 				     is31->cdef->num_leds);
 
-	fwnode_for_each_available_child_node(fwnode, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct is31fl319x_led *led;
 		u32 reg;
 
 		ret = fwnode_property_read_u32(child, "reg", &reg);
-		if (ret) {
-			ret = dev_err_probe(dev, ret, "Failed to read led 'reg' property\n");
-			goto put_child_node;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to read led 'reg' property\n");
 
-		if (reg < 1 || reg > is31->cdef->num_leds) {
-			ret = dev_err_probe(dev, -EINVAL, "invalid led reg %u\n", reg);
-			goto put_child_node;
-		}
+		if (reg < 1 || reg > is31->cdef->num_leds)
+			return dev_err_probe(dev, -EINVAL, "invalid led reg %u\n", reg);
 
 		led = &is31->leds[reg - 1];
 
-		if (led->configured) {
-			ret = dev_err_probe(dev, -EINVAL, "led %u is already configured\n", reg);
-			goto put_child_node;
-		}
+		if (led->configured)
+			return dev_err_probe(dev, -EINVAL, "led %u is already configured\n", reg);
 
 		ret = is31fl319x_parse_child_fw(dev, child, led, is31);
-		if (ret) {
-			ret = dev_err_probe(dev, ret, "led %u DT parsing failed\n", reg);
-			goto put_child_node;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "led %u DT parsing failed\n", reg);
 
 		led->configured = true;
 	}
@@ -454,10 +446,6 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 	}
 
 	return 0;
-
-put_child_node:
-	fwnode_handle_put(child);
-	return ret;
 }
 
 static inline int is31fl3190_microamp_to_cs(struct device *dev, u32 microamp)

-- 
2.40.1

Re: [PATCH 4/6] leds: is31fl319x: use device_for_each_child_node_scoped() to access child nodes

[Jonathan Cameron]

On Sat, 06 Jul 2024 17:23:36 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> In this case, the child nodes are not required outside the loop, and
> the scoped version of the macro can be used to remove the repetitive
> `goto put` pattern.
> 
> Use `device_for_each_child_node_scoped_scoped()` to indicate device's
> direct child nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

Use `device_for_each_child_node()` to indicate device's direct child
nodes.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/leds/leds-pca995x.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
index 78215dff1499..a5b32eaba724 100644
--- a/drivers/leds/leds-pca995x.c
+++ b/drivers/leds/leds-pca995x.c
@@ -102,7 +102,7 @@ static const struct regmap_config pca995x_regmap = {
 static int pca995x_probe(struct i2c_client *client)
 {
 	struct fwnode_handle *led_fwnodes[PCA995X_MAX_OUTPUTS] = { 0 };
-	struct fwnode_handle *np, *child;
+	struct fwnode_handle *child;
 	struct device *dev = &client->dev;
 	struct pca995x_chip *chip;
 	struct pca995x_led *led;
@@ -110,8 +110,7 @@ static int pca995x_probe(struct i2c_client *client)
 
 	btype = (unsigned long)device_get_match_data(&client->dev);
 
-	np = dev_fwnode(dev);
-	if (!np)
+	if (!dev_fwnode(dev))
 		return -ENODEV;
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
@@ -125,17 +124,13 @@ static int pca995x_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, chip);
 
-	fwnode_for_each_available_child_node(np, child) {
+	device_for_each_child_node(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &reg);
-		if (ret) {
-			fwnode_handle_put(child);
+		if (ret)
 			return ret;
-		}
 
-		if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg]) {
-			fwnode_handle_put(child);
+		if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg])
 			return -EINVAL;
-		}
 
 		led = &chip->leds[reg];
 		led_fwnodes[reg] = child;

-- 
2.40.1

Re: [PATCH 5/6] leds: pca995x: use device_for_each_child_node() to access device child nodes

[Jonathan Cameron]

On Sat, 06 Jul 2024 17:23:37 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> Use `device_for_each_child_node()` to indicate device's direct child
> nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 6/6] net: mvpp2: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

The iterated nodes are direct children of the device node, and the
`device_for_each_child_node()` macro accounts for child node
availability.

`fwnode_for_each_available_child_node()` is meant to access the child
nodes of an fwnode, and therefore not direct child nodes of the device
node.

The child nodes within mvpp2_probe are not accessed outside the lopps,
and the socped version of the macro can be used to automatically
decrement the refcount on early exits.

Use `device_for_each_child_node()` and its scoped variant to indicate
device's direct child nodes.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 9adf4301c9b1..97f1faab6f28 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -7417,8 +7417,6 @@ static int mvpp2_get_sram(struct platform_device *pdev,
 
 static int mvpp2_probe(struct platform_device *pdev)
 {
-	struct fwnode_handle *fwnode = pdev->dev.fwnode;
-	struct fwnode_handle *port_fwnode;
 	struct mvpp2 *priv;
 	struct resource *res;
 	void __iomem *base;
@@ -7591,7 +7589,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 	}
 
 	/* Map DTS-active ports. Should be done before FIFO mvpp2_init */
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
 		if (!fwnode_property_read_u32(port_fwnode, "port-id", &i))
 			priv->port_map |= BIT(i);
 	}
@@ -7614,7 +7612,7 @@ static int mvpp2_probe(struct platform_device *pdev)
 		goto err_axi_clk;
 
 	/* Initialize ports */
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
 		err = mvpp2_port_probe(pdev, port_fwnode, priv);
 		if (err < 0)
 			goto err_port_probe;
@@ -7653,10 +7651,8 @@ static int mvpp2_probe(struct platform_device *pdev)
 	return 0;
 
 err_port_probe:
-	fwnode_handle_put(port_fwnode);
-
 	i = 0;
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
 		if (priv->port_list[i])
 			mvpp2_port_remove(priv->port_list[i]);
 		i++;
@@ -7677,13 +7673,12 @@ static int mvpp2_probe(struct platform_device *pdev)
 static void mvpp2_remove(struct platform_device *pdev)
 {
 	struct mvpp2 *priv = platform_get_drvdata(pdev);
-	struct fwnode_handle *fwnode = pdev->dev.fwnode;
 	int i = 0, poolnum = MVPP2_BM_POOLS_NUM;
 	struct fwnode_handle *port_fwnode;
 
 	mvpp2_dbgfs_cleanup(priv);
 
-	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
+	device_for_each_child_node(&pdev->dev, port_fwnode) {
 		if (priv->port_list[i]) {
 			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
 			mvpp2_port_remove(priv->port_list[i]);

-- 
2.40.1

Re: [PATCH 6/6] net: mvpp2: use device_for_each_child_node() to access device child nodes

[Jonathan Cameron]

On Sat, 06 Jul 2024 17:23:38 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> The child nodes within mvpp2_probe are not accessed outside the lopps,
> and the socped version of the macro can be used to automatically
> decrement the refcount on early exits.
> 
> Use `device_for_each_child_node()` and its scoped variant to indicate
> device's direct child nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 6/6] net: mvpp2: use device_for_each_child_node() to access device child nodes

[Russell King (Oracle)]

On Sat, Jul 06, 2024 at 05:23:38PM +0200, Javier Carrasco wrote:
> The iterated nodes are direct children of the device node, and the
> `device_for_each_child_node()` macro accounts for child node
> availability.
> 
> `fwnode_for_each_available_child_node()` is meant to access the child
> nodes of an fwnode, and therefore not direct child nodes of the device
> node.
> 
> The child nodes within mvpp2_probe are not accessed outside the lopps,

"lopps" ?

> and the socped version of the macro can be used to automatically

"socped" ?

> decrement the refcount on early exits.
> 
> Use `device_for_each_child_node()` and its scoped variant to indicate
> device's direct child nodes.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 9adf4301c9b1..97f1faab6f28 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -7417,8 +7417,6 @@ static int mvpp2_get_sram(struct platform_device *pdev,
>  
>  static int mvpp2_probe(struct platform_device *pdev)
>  {
> -	struct fwnode_handle *fwnode = pdev->dev.fwnode;
> -	struct fwnode_handle *port_fwnode;
>  	struct mvpp2 *priv;
>  	struct resource *res;
>  	void __iomem *base;
> @@ -7591,7 +7589,7 @@ static int mvpp2_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Map DTS-active ports. Should be done before FIFO mvpp2_init */
> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>  		if (!fwnode_property_read_u32(port_fwnode, "port-id", &i))
>  			priv->port_map |= BIT(i);
>  	}
> @@ -7614,7 +7612,7 @@ static int mvpp2_probe(struct platform_device *pdev)
>  		goto err_axi_clk;
>  
>  	/* Initialize ports */
> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>  		err = mvpp2_port_probe(pdev, port_fwnode, priv);
>  		if (err < 0)
>  			goto err_port_probe;
> @@ -7653,10 +7651,8 @@ static int mvpp2_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_port_probe:
> -	fwnode_handle_put(port_fwnode);
> -
>  	i = 0;
> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>  		if (priv->port_list[i])
>  			mvpp2_port_remove(priv->port_list[i]);
>  		i++;
> @@ -7677,13 +7673,12 @@ static int mvpp2_probe(struct platform_device *pdev)
>  static void mvpp2_remove(struct platform_device *pdev)
>  {
>  	struct mvpp2 *priv = platform_get_drvdata(pdev);
> -	struct fwnode_handle *fwnode = pdev->dev.fwnode;
>  	int i = 0, poolnum = MVPP2_BM_POOLS_NUM;
>  	struct fwnode_handle *port_fwnode;
>  
>  	mvpp2_dbgfs_cleanup(priv);
>  
> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
> +	device_for_each_child_node(&pdev->dev, port_fwnode) {
>  		if (priv->port_list[i]) {
>  			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
>  			mvpp2_port_remove(priv->port_list[i]);

This loop is just silly. There is no need to iterate the child nodes.
port_fwnode is not used, and the loop boils down to:

	for (i = 0; i < priv->port_count; i++) {
		mutex_destroy(&priv->port_list[i]->gather_stats_lock);
		mvpp2_port_remove(priv->port_list[i]);
	}

Not only is walking the child nodes not necessary, but checking whether
the pointer is NULL is also unnecessary. mvpp2_port_probe() populates
the array using:

        priv->port_list[priv->port_count++] = port;

and "port" can not be NULL here, so we're guaranteed that all port_list
entries for 0..priv->port_count will be non-NULL, and the driver makes
this assumption in multiple places.

In fact, I'd say that using fwnode_for_each_available_child_node() or
device_for_each_child_node() is buggy here if the availability of the
children change - it could leave ports not cleaned up.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 6/6] net: mvpp2: use device_for_each_child_node() to access device child nodes

[Javier Carrasco]

On 29/07/2024 10:23, Russell King (Oracle) wrote:
> On Sat, Jul 06, 2024 at 05:23:38PM +0200, Javier Carrasco wrote:
>> The iterated nodes are direct children of the device node, and the
>> `device_for_each_child_node()` macro accounts for child node
>> availability.
>>
>> `fwnode_for_each_available_child_node()` is meant to access the child
>> nodes of an fwnode, and therefore not direct child nodes of the device
>> node.
>>
>> The child nodes within mvpp2_probe are not accessed outside the lopps,
> 
> "lopps" ?
> 
>> and the socped version of the macro can be used to automatically
> 
> "socped" ?
> 

I'll fix the typos for v3.

>> decrement the refcount on early exits.
>>
>> Use `device_for_each_child_node()` and its scoped variant to indicate
>> device's direct child nodes.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> index 9adf4301c9b1..97f1faab6f28 100644
>> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> @@ -7417,8 +7417,6 @@ static int mvpp2_get_sram(struct platform_device *pdev,
>>  
>>  static int mvpp2_probe(struct platform_device *pdev)
>>  {
>> -	struct fwnode_handle *fwnode = pdev->dev.fwnode;
>> -	struct fwnode_handle *port_fwnode;
>>  	struct mvpp2 *priv;
>>  	struct resource *res;
>>  	void __iomem *base;
>> @@ -7591,7 +7589,7 @@ static int mvpp2_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	/* Map DTS-active ports. Should be done before FIFO mvpp2_init */
>> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
>> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>>  		if (!fwnode_property_read_u32(port_fwnode, "port-id", &i))
>>  			priv->port_map |= BIT(i);
>>  	}
>> @@ -7614,7 +7612,7 @@ static int mvpp2_probe(struct platform_device *pdev)
>>  		goto err_axi_clk;
>>  
>>  	/* Initialize ports */
>> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
>> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>>  		err = mvpp2_port_probe(pdev, port_fwnode, priv);
>>  		if (err < 0)
>>  			goto err_port_probe;
>> @@ -7653,10 +7651,8 @@ static int mvpp2_probe(struct platform_device *pdev)
>>  	return 0;
>>  
>>  err_port_probe:
>> -	fwnode_handle_put(port_fwnode);
>> -
>>  	i = 0;
>> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
>> +	device_for_each_child_node_scoped(&pdev->dev, port_fwnode) {
>>  		if (priv->port_list[i])
>>  			mvpp2_port_remove(priv->port_list[i]);
>>  		i++;
>> @@ -7677,13 +7673,12 @@ static int mvpp2_probe(struct platform_device *pdev)
>>  static void mvpp2_remove(struct platform_device *pdev)
>>  {
>>  	struct mvpp2 *priv = platform_get_drvdata(pdev);
>> -	struct fwnode_handle *fwnode = pdev->dev.fwnode;
>>  	int i = 0, poolnum = MVPP2_BM_POOLS_NUM;
>>  	struct fwnode_handle *port_fwnode;
>>  
>>  	mvpp2_dbgfs_cleanup(priv);
>>  
>> -	fwnode_for_each_available_child_node(fwnode, port_fwnode) {
>> +	device_for_each_child_node(&pdev->dev, port_fwnode) {
>>  		if (priv->port_list[i]) {
>>  			mutex_destroy(&priv->port_list[i]->gather_stats_lock);
>>  			mvpp2_port_remove(priv->port_list[i]);
> 
> This loop is just silly. There is no need to iterate the child nodes.
> port_fwnode is not used, and the loop boils down to:
> 
> 	for (i = 0; i < priv->port_count; i++) {
> 		mutex_destroy(&priv->port_list[i]->gather_stats_lock);
> 		mvpp2_port_remove(priv->port_list[i]);
> 	}
> 
> Not only is walking the child nodes not necessary, but checking whether
> the pointer is NULL is also unnecessary. mvpp2_port_probe() populates
> the array using:
> 
>         priv->port_list[priv->port_count++] = port;
> 
> and "port" can not be NULL here, so we're guaranteed that all port_list
> entries for 0..priv->port_count will be non-NULL, and the driver makes
> this assumption in multiple places.
> 
> In fact, I'd say that using fwnode_for_each_available_child_node() or
> device_for_each_child_node() is buggy here if the availability of the
> children change - it could leave ports not cleaned up.
> 

I will add your suggestions in a separate patch with the corresponding
Suggested-by: tag. In that case, and taking into account that the
pointer check is unnecessary, the loop after a goto err_port_probe will
turn into this:

err_port_probe:
	for (i = 0; i < priv->port_count; i++)
		mvpp2_port_remove(priv->port_list[i]);

and the loop in mvpp2_remove() will be exactly the one you suggested.

Apart from that, there is a suspicious check towards the end of the same
function:

 if (is_acpi_node(port_fwnode))
		return;

At the point it is called in the current implementation, port_fwnode
could have been cleaned. And after removing the loop, it is simply
uninitialized. Was that meant to be pdev->dev->fwnode?

Thanks and best regards,
Javier Carrasco

Re: [PATCH 6/6] net: mvpp2: use device_for_each_child_node() to access device child nodes

[Russell King (Oracle)]

On Mon, Jul 29, 2024 at 11:23:47AM +0200, Javier Carrasco wrote:
> Apart from that, there is a suspicious check towards the end of the same
> function:
> 
>  if (is_acpi_node(port_fwnode))
> 		return;
> 
> At the point it is called in the current implementation, port_fwnode
> could have been cleaned. And after removing the loop, it is simply
> uninitialized. Was that meant to be pdev->dev->fwnode?

If you're referring to the one before the clk_disable_unprepare() calls,
it's only slightly suspicious:

These clocks are setup in a:

        if (dev_of_node(&pdev->dev)) {
		...
	}

block, so they're only setup if we have device tree. So, avoiding it
for ACPI is entirely reasonable. However, we also have software nodes
as well, so the test should be:

	if (!dev_of_node(&pdev->dev))
		return;

to match what the probe function is doing.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!