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

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: <2077c7e1-cf69-8648-20ac-23793f92ad14@9elements.com>
Date:   Mon, 27 Mar 2023 21:17:16 +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 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", &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.

>>> "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.

> 
> CJ
> 
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
> 
> [...]
> 
Regards,
Naresh