Re: [PATCH v2 2/2] leds: max597x: Add support for max597x

From: Naresh Solanki
Date: Mon Mar 27 2023 - 13:50:03 EST


Hi,

On 27-03-2023 10:50 pm, Christophe JAILLET wrote:
Le 27/03/2023 à 17:47, Naresh Solanki a écrit :
Hi,

On 24-03-2023 09:06 pm, Christophe JAILLET wrote:
Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
Hi,

On 24-03-2023 01:48 am, Christophe JAILLET wrote:
Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>

max597x is hot swap controller with indicator LED support.
This driver uses DT property to configure led during boot time &
also provide the LED control in sysfs.


[...]


+static int max597x_led_probe(struct platform_device *pdev)
+{
+    struct device_node *np = dev_of_node(pdev->dev.parent);
+    struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+    struct device_node *led_node;
+    struct device_node *child;
+    int ret = 0;
+
+    if (!regmap)
+        return -EPROBE_DEFER;
+
+    led_node = of_get_child_by_name(np, "leds");
+    if (!led_node)
+        return -ENODEV;
+
+    for_each_available_child_of_node(led_node, child) {
+        u32 reg;
+
+        if (of_property_read_u32(child, "reg", &reg))
+            continue;
+
+        if (reg >= MAX597X_NUM_LEDS) {
+            dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
+                MAX597X_NUM_LEDS);
+            continue;
+        }
+
+        ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
+        if (ret < 0)
+            of_node_put(child);

This of_node_put() looks odd to me.
Not sure if I get this right but if led setup fails of_node_put should be called.

My understanding is that this of_node_put() is there in case of error, to release what would otherwise never be released by for_each_available_child_of_node() if we exit early from the loop.

If the purpose is to release a reference taken in max597x_setup_led() in case of error:
    - this should be done IMHO within max597x_setup_led() directly
    - there should be a of_node_get() somewhere in max597x_setup_led()
      (after:
     if (of_property_read_string(nc, "label", &led->led.name))
         led->led.name = nc->name;
       + error handling path,  *or*
      just before the final return ret; when we know that everything is fine,
      if I understand correctly the code)

Is the reference taken elsewhere?
Did I miss something obvious?


One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311
Please do let me know if I have to do anything about it.
By reference, I was speaking of reference taken by a of_node_get() call and released by a of_node_put() call.

Anyway, I do agree with leds-sc27xx-bltc.c.
There is a of_node_put() because for_each_available_child_of_node() won't be able to do it by itself *in case of early return* ("return err;")

In all other paths (when the loop goes to the end), the reference taken by for_each_available_child_of_node() is also released, on the next iteration, by for_each_available_child_of_node().

In *your* case, if you don't break or return, there is no need to call of_node_put() explicitly. It would lead to a double put. (yours and the one that will be done by for_each_available_child_of_node()).

Have a look at for_each_available_child_of_node() and more specifically at of_get_next_available_child().

At the first call 'child' is NULL. A ref is taken [1]. Nothing is released.
For following calls, a new ref is taken on a new node [1], and the previous reference is released [2].
On the last call, the 'for' loop will not be executed because there is nothing to scan anymore. No new reference is taken, and the previous (and last) refence is finally released [2].
Yes you are right. That of_node_put would be duplicate as it is already taken care by the for loop.
Will remove that in next revision.


[1]: https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L808
[2]: https://elixir.bootlin.com/linux/v6.3-rc3/source/drivers/of/base.c#L811


"return ret;" or "break;" missing ?

Didn't add a break so that it can continue initializing remaining led if any.

Ok. Don't know the code enough to see if correct or not, but based on my comment above, I think that something is missing in max597x_setup_led() and that errors should be silently ignored here.
In my implementation, I have chosen to continue with the next LED if an error occurs, rather than aborting the 'for loop' with an error. I have seen other implementations also done in a similar way.
Do you have any further inputs or suggestions on this approach.

No, sorry, I won't be of any help on what design is the best.

CJ

Regards,
Naresh