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: <4f996369-2959-4e17-917d-f2de48d22064@gmail.com>
Date: Sat, 13 Jul 2024 23:37:46 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Jonathan Cameron <jic23@...nel.org>,
 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 12/07/2024 23:06, Andreas Kemnade wrote:
> On Mon, 8 Jul 2024 17:45:43 +0200
> Javier Carrasco <javier.carrasco.cruz@...il.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