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 <h...@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
