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

Commit Message

Javier Carrasco July 6, 2024, 3:23 p.m. UTC

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 <[e-mail]>
---
 drivers/leds/leds-bd2606mvv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Jonathan Cameron July 7, 2024, 4:57 p.m. UTC | #1

On Sat, 06 Jul 2024 17:23:35 +0200
Javier Carrasco <[e-mail]> 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 <[e-mail]>
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);
>

Javier Carrasco July 8, 2024, 8:14 a.m. UTC | #2

On 07/07/2024 18:57, Jonathan Cameron wrote:
> On Sat, 06 Jul 2024 17:23:35 +0200
> Javier Carrasco <[e-mail]> 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 <[e-mail]>
> 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);
>>
>

Jonathan Cameron July 8, 2024, 8:28 a.m. UTC | #3

No

On 8 July 2024 09:14:44 BST, Javier Carrasco <[e-mail]> wrote:
>On 07/07/2024 18:57, Jonathan Cameron wrote:
>> On Sat, 06 Jul 2024 17:23:35 +0200
>> Javier Carrasco <[e-mail]> 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 <[e-mail]>
>> 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);
>>>
>> 
>

Javier Carrasco July 8, 2024, 3:45 p.m. UTC | #4

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 <[e-mail]> 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 <[e-mail]>
>> 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);
>>>
>>
>

Andreas Kemnade July 12, 2024, 9:06 p.m. UTC | #5

On Mon, 8 Jul 2024 17:45:43 +0200
Javier Carrasco <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>  
> >> 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

Javier Carrasco July 13, 2024, 9:37 p.m. UTC | #6

On 12/07/2024 23:06, Andreas Kemnade wrote:
> On Mon, 8 Jul 2024 17:45:43 +0200
> Javier Carrasco <[e-mail]> 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

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