netdev - Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4cf71de7-dc47-475c-bba0-a9e755f66d49@gmail.com>
Date: Mon, 8 Jul 2024 10:14:44 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Rob Herring <robh@...nel.org>, Daniel Scally <djrscally@...il.com>,
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, Jean Delvare
 <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>,
 Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
 Marcin Wojtas <marcin.s.wojtas@...il.com>,
 Russell King <linux@...linux.org.uk>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-leds@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to
 access device child nodes

On 07/07/2024 18:57, Jonathan Cameron wrote:
> On Sat, 06 Jul 2024 17:23:35 +0200
> Javier Carrasco <javier.carrasco.cruz@...il.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@...il.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);
>>
>