[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <05278bcb-7324-99b4-5442-ac44b2cdeed5@9elements.com>
Date: Mon, 27 Mar 2023 23:19:49 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
Lee Jones <lee@...nel.org>, Pavel Machek <pavel@....cz>
Cc: Patrick Rudolph <patrick.rudolph@...ements.com>,
linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: max597x: Add support for max597x
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@...ements.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", ®))
>>>>>> + 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
Powered by blists - more mailing lists