[PATCH v1] led: fix next coverity scan error

[Heiko Schocher]

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!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de