linux-kernel - 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: <6119CC81-5F47-4DA3-8C9C-98C7C87C9734@jic23.retrosnub.co.uk>
Date: Mon, 08 Jul 2024 09:28:18 +0100
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>,
 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

No

On 8 July 2024 09:14:44 BST, Javier Carrasco <javier.carrasco.cruz@...il.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@...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.

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