[PATCH v1] led: fix next coverity scan error
Tom Rini
trini at konsulko.com
Tue Feb 25 17:13:34 CET 2025
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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250225/33244660/attachment.sig>
More information about the U-Boot
mailing list