[PATCH v1] led: fix next coverity scan error

[Quentin Schulz]

Hi Tom,

On 2/25/25 5:13 PM, Tom Rini wrote:
> On Tue, Feb 25, 2025 at 12:29:29PM +0100, Heiko Schocher wrote:
>> Hello Quentin,
>>
>> On 25.02.25 12:13, Quentin Schulz wrote:
>>> Hi Heiko,
>>>
>>> On 2/25/25 10:49 AM, Heiko Schocher wrote:
>>>> Tom reported the following covervity scan error:
>>>>
>>>> *** CID 542488:  Control flow issues  (NO_EFFECT)
>>>> /drivers/led/led-uclass.c: 277 in led_get_function_name()
>>>> 271                     return uc_plat->label;
>>>> 272
>>>> 273             /* Now try to detect function label name */
>>>> 274             func = dev_read_string(dev, "function");
>>>> 275             cp = dev_read_u32(dev, "color", &color);
>>>> 276             // prevent coverity scan error CID 541279: (TAINTED_SCALAR)
>>>>>>>       CID 542488:  Control flow issues  (NO_EFFECT)
>>>>>>>       This less-than-zero comparison of an unsigned value is never true.
>>>> "color < 0U".
>>>> 277             if (color < LED_COLOR_ID_WHITE || color >= LED_COLOR_ID_MAX)
>>>> 278                     cp = -EINVAL;
>>>> 279
>>>>
>>>> see:
>>>> https://lists.denx.de/pipermail/u-boot/2025-February/581567.html
>>>>
>>>> Fix it.
>>>>
>>>> Signed-off-by: Heiko Schocher <hs at denx.de>
>>>>
>>>> ---
>>>> Azure build:
>>>> https://dev.azure.com/hs0298/hs/_build/results?buildId=171&view=results
>>>>
>>>>    drivers/led/led-uclass.c                       | 2 +-
>>>>    dts/upstream/include/dt-bindings/leds/common.h | 5 ++++-
>>>
>>> NACK. We don't modify files in dts/upstream as they are directly
>>> imported from devicetree-rebasing tree and would either be replaced
>>> during next update or induce a merge conflict.
>>
>> Ah, yes, indeed!
>>
>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/led/led-uclass.c b/drivers/led/led-uclass.c
>>>> index 22f61d12d38..e2edcc43f30 100644
>>>> --- a/drivers/led/led-uclass.c
>>>> +++ b/drivers/led/led-uclass.c
>>>> @@ -274,7 +274,7 @@ static const char *led_get_function_name(struct udevice *dev)
>>>>        func = dev_read_string(dev, "function");
>>>>        cp = dev_read_u32(dev, "color", &color);
>>>>        // prevent coverity scan error CID 541279: (TAINTED_SCALAR)
>>>> -    if (color < LED_COLOR_ID_WHITE || color >= LED_COLOR_ID_MAX)
>>>> +    if (color >= LED_COLOR_ID_MAX)
>>>>            cp = -EINVAL;
>>>>        if (cp == 0 || func) {
>>>> diff --git a/dts/upstream/include/dt-bindings/leds/common.h
>>>> b/dts/upstream/include/dt-bindings/leds/common.h
>>>> index 4f017bea012..9e5ac5196e2 100644
>>>> --- a/dts/upstream/include/dt-bindings/leds/common.h
>>>> +++ b/dts/upstream/include/dt-bindings/leds/common.h
>>>> @@ -21,7 +21,10 @@
>>>>    #define LEDS_BOOST_ADAPTIVE    1
>>>>    #define LEDS_BOOST_FIXED    2
>>>> -/* Standard LED colors */
>>>> +/*
>>>> + * Standard LED colors, LED_COLOR_ID_WHITE must be the
>>>> + * first one and start with value 0
>>>> + */
>>>
>>> Can you explain why white needs to be the first AND be 0?
>>>
>>> I don't know enough about the C standard or compilers in general to know
>>> what happens if the indices in led_colors aren't in order but I guess
>>> that's where your limitation comes from?
>>>
>>> e.g.
>>>
>>>     static const char * const led_colors[LED_COLOR_ID_MAX] = {
>>>             [LED_COLOR_ID_WHITE] = "white",
>>>             [LED_COLOR_ID_RED] = "red",
>>>             [LED_COLOR_ID_GREEN] = "green",
>>>             [LED_COLOR_ID_BLUE] = "blue",
>>>             [LED_COLOR_ID_AMBER] = "amber",
>>>             [LED_COLOR_ID_VIOLET] = "violet",
>>>             [LED_COLOR_ID_YELLOW] = "yellow",
>>>             [LED_COLOR_ID_IR] = "ir",
>>>             [LED_COLOR_ID_MULTI] = "multicolor",
>>>             [LED_COLOR_ID_RGB] = "rgb",
>>>             [LED_COLOR_ID_PURPLE] = "purple",
>>>             [LED_COLOR_ID_ORANGE] = "orange",
>>>             [LED_COLOR_ID_PINK] = "pink",
>>>             [LED_COLOR_ID_CYAN] = "cyan",
>>>             [LED_COLOR_ID_LIME] = "lime",
>>>     };
>>>
>>> is ordered, but what happens if you have e.g.:
>>>
>>>     static const char * const led_colors[LED_COLOR_ID_MAX] = {
>>>             [LED_COLOR_ID_RED] = "red",
>>>             [LED_COLOR_ID_WHITE] = "white",
>>>             [LED_COLOR_ID_GREEN] = "green",
>>>             [LED_COLOR_ID_BLUE] = "blue",
>>>             [LED_COLOR_ID_AMBER] = "amber",
>>>             [LED_COLOR_ID_VIOLET] = "violet",
>>>             [LED_COLOR_ID_YELLOW] = "yellow",
>>>             [LED_COLOR_ID_IR] = "ir",
>>>             [LED_COLOR_ID_MULTI] = "multicolor",
>>>             [LED_COLOR_ID_RGB] = "rgb",
>>>             [LED_COLOR_ID_PURPLE] = "purple",
>>>             [LED_COLOR_ID_ORANGE] = "orange",
>>>             [LED_COLOR_ID_PINK] = "pink",
>>>             [LED_COLOR_ID_CYAN] = "cyan",
>>>             [LED_COLOR_ID_LIME] = "lime",
>>>     };
>>>
>>> ? I think it should be just fine?
>>
>> Yep, fully fine, so I drop simply this comment.
>>
>>> Reading the code, it seems there's an assumption that led_colors has a
>>> contiguous and full list of indices from LED_COLOR_ID "enum". What
>>> happens when the dt-binding gets extended with new colors and the DT
>>> makes use of it without updating led_colors in led-uclass.c? Aren't we
>>> going to try to print a NULL string?
>>>
>>> If that's the case, I see two solutions:
>>> - ignore LED colors when they aren't in led_colors,
>>> - decouple LED_COLOR_ID_MAX from the binding with UBOOT_LED_COLOR_ID_MAX
>>> which is the currently supported max based on led_colors last index
>>> (+1). We could even do a #warn or #error so that UBOOT_LED_COLOR_ID_MAX
>>> and LED_COLOR_ID_MAX are the same value so that we're forced to keep
>>> them in sync, but that'll make Tom's life harder when merging newer
>>> releases of devicetree-rebasing tree.
>>
>> Yep... may Tom can comment here, what he prefers, thanks for the review!
> 
> We should just expand the comment in the uclass when checking to note
> that we don't need to check the lower bound, only upper bound due to
> typing.
> 

My point was rather about dt-binding updates to LED_COLOR_ID_MAX. The 
code currently assumes we always have an entry in led_colors for all 
values between 0 and LED_COLOR_ID_MAX. But that may change if there are 
new values added in the kernel and we don't update led-uclass.c (e.g. 
the new LED_COLOR_ID_MAX is bigger than the current one, so there are 
NULL entries now in the array).

I've tested by removing the AMBER entry and then I have the following in 
the CLI:

=> led list
<NULL>:heartbeat <inactive>
blue:sd         <inactive>
=> led <NULL>:heartbeat
LED '<NULL>:heartbeat': off
=> led <NULL>:heartbeat on

so it seems "safe" wrt having a NULL led_colors[color]. If we have more 
than one non-existing entry in led_colors array with the same function, 
I assume only the first one will be addressed.

Cheers,
Quentin