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

[Naresh Solanki]

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