[PATCH] cmd: led: reject unknown LED state instead of silently returning success

[Naveen Kumar Chaudhary]

After the commit 72675b063b6e changed led states to an enum, I honestly
don't see any problem (others can correct me). The current
implementation is more readable than if-else ladder, specially when the
list grows for whatever reason.

However, thanks to your comment, I went through the implementation once
more and found that the below define in led.c seems leftover and buggy
to me and needs to be removed :

#define LED_TOGGLE LEDST_COUNT

Thoughts?

Regards,
Naveen

On Wed 08 Jul 06:00 PM, Quentin Schulz wrote:
> Hi Naveen,
> 
> On 7/8/26 4:32 PM, Naveen Kumar Chaudhary wrote:
> > get_led_cmd() searches state_label[] for a matching name and returns
> > -1 on no match, but its declared return type is enum led_state_t.
> > All enumerators of that enum are non-negative, so the compiler is
> > free to pick an unsigned underlying type (and does so with
> > -fshort-enums, which several U-Boot targets enable). In that case
> > -1 is reinterpreted as a large positive value and any subsequent
> > comparison against -1 silently misbehaves.
> > 
> > In do_led() the returned value is stored into an enum led_state_t
> > and dispatched through a switch that has no default arm and no case
> > for the sentinel. When the user supplies an unknown state name the
> > switch falls through, ret was already set to 0 by led_get_by_label(),
> > and the command reports success without doing anything.
> > 
> > Return LEDST_COUNT from get_led_cmd() on no match. LEDST_COUNT is
> > already a valid enumerator, so this avoids all questions about the
> > enum's underlying representation. In do_led(), when a state argument
> > was actually supplied (argc > 2) and the lookup produced LEDST_COUNT,
> > reject it with a diagnostic and CMD_RET_USAGE; the existing
> > LEDST_COUNT switch arm continues to handle the "no state argument"
> > case where it means "show current state".
> > 
> 
> That looks correct to me, but I think get_led_cmd() is overengineered and
> actually still flawed. Indeed, we iterate up to LEDST_COUNT-1 times on the
> state_label[] array, but nothing guarantees the array is that big (it is
> right now) so we could theoretically have out-of-bounds access in the
> future. It also isn't handling the case when there's no matching string at a
> given index (e.g. holes in the array or padding at the end due to the last
> element in the array not being LEDST_COUNT-1), where state_label[i] would be
> NULL and strncmp doesn't seem to be handling that case (it isn't in
> lib/string.c for example).
> 
> I think the answer is to instead get rid of this and drastically simplify.
> 
> Remove get_led_cmd() and state_label[] and do something like the following
> in do_led() (NOT TESTED):
> 
> """
> diff --git a/cmd/led.c b/cmd/led.c
> index 296c07b3b38b..d1fc9dc8cdbd 100644
> --- a/cmd/led.c
> +++ b/cmd/led.c
> @@ -9,27 +9,6 @@
>  #include <led.h>
>  #include <dm/uclass-internal.h>
> 
> -#define LED_TOGGLE LEDST_COUNT
> -
> -static const char *const state_label[] = {
> -	[LEDST_OFF]	= "off",
> -	[LEDST_ON]	= "on",
> -	[LEDST_TOGGLE]	= "toggle",
> -	[LEDST_BLINK]	= "blink",
> -};
> -
> -enum led_state_t get_led_cmd(char *var)
> -{
> -	int i;
> -
> -	for (i = 0; i < LEDST_COUNT; i++) {
> -		if (!strncmp(var, state_label[i], strlen(var)))
> -			return i;
> -	}
> -
> -	return -1;
> -}
> -
>  static int show_led_state(struct udevice *dev)
>  {
>  	int ret;
> @@ -83,12 +62,24 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
>  	if (strncmp(led_label, "list", 4) == 0)
>  		return list_leds();
> 
> -	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
> -	if (cmd == LEDST_BLINK) {
> +	if (argc == 2) {
> +		cmd = LEDST_COUNT;
> +	} else if (!strncmp(argv[2], "off", strlen(argv[2]))) {
> +		cmd = LEDST_OFF;
> +	} else if (!strncmp(argv[2], "on", strlen(argv[2]))) {
> +		cmd = LEDST_ON;
> +	} else if (!strncmp(argv[2], "toggle", strlen(argv[2]))) {
> +		cmd = LEDST_TOGGLE;
> +	} else if (!strncmp(argv[2], "blink", strlen(argv[2]))) {
>  		if (argc < 4)
>  			return CMD_RET_USAGE;
> +
> +		cmd = LEDST_BLINK;
>  		freq_ms = dectoul(argv[3], NULL);
> +	} else {
> +		return CMD_RET_USAGE;
>  	}
> +
>  	ret = led_get_by_label(led_label, &dev);
>  	if (ret) {
>  		printf("LED '%s' not found (err=%d)\n", led_label, ret);
> """
> 
> What do you think?
> 
> I wouldn't necessarily add a printf for when the user mistypes, just return
> CMD_RET_USAGE where we list what's possible. If you really want to keep it
> at least replace "state" with "operation" as it's something we do on the LED
> not its state (e.g. one can request "toggle" which definitely isn't a
> state).
> 
> Please also add
> Fixes: ffe2052d6e8a ("dm: led: Add a new 'led' command")
> 
> to your commit log also as that's the commit introducing the error.
> 
> Cheers,
> Quentin
> 
> > Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev at gmail.com>
> > ---
> >   cmd/led.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/led.c b/cmd/led.c
> > index 296c07b3b38..d547276e480 100644
> > --- a/cmd/led.c
> > +++ b/cmd/led.c
> > @@ -27,7 +27,7 @@ enum led_state_t get_led_cmd(char *var)
> >   			return i;
> >   	}
> > -	return -1;
> > +	return LEDST_COUNT;
> >   }
> >   static int show_led_state(struct udevice *dev)
> > @@ -84,6 +84,10 @@ int do_led(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >   		return list_leds();
> >   	cmd = argc > 2 ? get_led_cmd(argv[2]) : LEDST_COUNT;
> > +	if (argc > 2 && cmd == LEDST_COUNT) {
> > +		printf("Unknown LED state '%s'\n", argv[2]);
> > +		return CMD_RET_USAGE;
> > +	}
> >   	if (cmd == LEDST_BLINK) {
> >   		if (argc < 4)
> >   			return CMD_RET_USAGE;
>