[PATCH v1] led: fix next coverity scan error

[Quentin Schulz]

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.

>   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?

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.

Cheers,
Quentin