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

[Naresh Solanki <naresh.solanki@9elements.com>]

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@9elements.com>
>>>>>>
>>>>>> 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