[PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[Marek Behún]

At the time we call
    BUG_ON(props.color == LED_COLOR_ID_MULTI);
the props variable is still initialized to zero.

Call the BUG_ON only after we parse fwnode into props.

Fixes: 77dce3a22e89 ("leds: disallow /sys/class/leds/*:multi:* for now")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/led-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index b9b1295833c9..04f9ea675f2c 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -474,15 +474,15 @@ int led_compose_name(struct device *dev, struct led_init_data *init_data,
 	struct fwnode_handle *fwnode = init_data->fwnode;
 	const char *devicename = init_data->devicename;
 
-	/* We want to label LEDs that can produce full range of colors
-	 * as RGB, not multicolor */
-	BUG_ON(props.color == LED_COLOR_ID_MULTI);
-
 	if (!led_classdev_name)
 		return -EINVAL;
 
 	led_parse_fwnode_props(dev, fwnode, &props);
 
+	/* We want to label LEDs that can produce full range of colors
+	 * as RGB, not multicolor */
+	BUG_ON(props.color == LED_COLOR_ID_MULTI);
+
 	if (props.label) {
 		/*
 		 * If init_data.devicename is NULL, then it indicates that
-- 
2.41.0

Re: (subset) [PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[Lee Jones]

On Tue, 01 Aug 2023 17:16:23 +0200, Marek Behún wrote:
> At the time we call
>     BUG_ON(props.color == LED_COLOR_ID_MULTI);
> the props variable is still initialized to zero.
> 
> Call the BUG_ON only after we parse fwnode into props.
> 
> 
> [...]

Applied, thanks!

[1/1] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false
      commit: c3f853184bed04105682383c2971798c572226b5

--
Lee Jones [李琼斯]

Re: [PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[Mark Rutland]

On Tue, Aug 01, 2023 at 05:16:23PM +0200, Marek Beh'un wrote:
> At the time we call
>     BUG_ON(props.color == LED_COLOR_ID_MULTI);
> the props variable is still initialized to zero.
> 
> Call the BUG_ON only after we parse fwnode into props.
> 
> Fixes: 77dce3a22e89 ("leds: disallow /sys/class/leds/*:multi:* for now")
> Signed-off-by: Marek Beh'un <kabel@kernel.org>

I've just discovered this has broken boot on my Libre Computer
AML-A311D-CC-V0.2, which was working just fine with Debian 12's stock kernel:

  mark@stor-flodeboller:~$ uname -a
  Linux stor-flodeboller 6.1.0-12-arm64 #1 SMP Debian 6.1.52-1 (2023-09-07) aarch64 GNU/Linux

When upgrading to v6.6-rc3 for testing, the board dies at boot time due to the
BUG_ON() moved by this commit. That BUG_ON() provides no useful context for
solving the issue, and it's *distinctly* unhelpful.

I decompiled the DTB provided by the firmware, and it has:

| leds {  
|         compatible = "gpio-leds";
| 
|         led-yellow-green {
|                 color = <0x08>;
|                 function = "status";
|                 gpios = <0x16 0x44 0x01>;
|                 linux,default-trigger = "default-on";
|                 panic-indicator;
|         };
| 
|         led-blue {
|                 color = <0x03>;
|                 function = "activity";
|                 gpios = <0x16 0x48 0x01>;
|                 linux,default-trigger = "activity";
|         };
| };

... and IIUC LED_COLOR_ID_MULTI is 8, so the problem is the led-yellow-green
subnode. I presume that could use LED_COLOR_ID_RGB.

Can we please do something so that this doesn't prevent the board from booting?

Is there some reason we don't transparently convert LED_COLOR_ID_MULTI to
LED_COLOR_ID_RGB?

If that's not viable, could we skip the LED *without* crashing the kernel, so
that the board remains usable?

Thanks,
Mark.

> ---
>  drivers/leds/led-core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index b9b1295833c9..04f9ea675f2c 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -474,15 +474,15 @@ int led_compose_name(struct device *dev, struct led_init_data *init_data,
>  	struct fwnode_handle *fwnode = init_data->fwnode;
>  	const char *devicename = init_data->devicename;
>  
> -	/* We want to label LEDs that can produce full range of colors
> -	 * as RGB, not multicolor */
> -	BUG_ON(props.color == LED_COLOR_ID_MULTI);
> -
>  	if (!led_classdev_name)
>  		return -EINVAL;
>  
>  	led_parse_fwnode_props(dev, fwnode, &props);
>  
> +	/* We want to label LEDs that can produce full range of colors
> +	 * as RGB, not multicolor */
> +	BUG_ON(props.color == LED_COLOR_ID_MULTI);
> +
>  	if (props.label) {
>  		/*
>  		 * If init_data.devicename is NULL, then it indicates that
> -- 
> 2.41.0
> 
> 

Re: [PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[Mark Rutland]

On Fri, Oct 06, 2023 at 03:24:04PM +0100, Mark Rutland wrote:
> On Tue, Aug 01, 2023 at 05:16:23PM +0200, Marek Beh'un wrote:
> > At the time we call
> >     BUG_ON(props.color == LED_COLOR_ID_MULTI);
> > the props variable is still initialized to zero.
> > 
> > Call the BUG_ON only after we parse fwnode into props.
> > 
> > Fixes: 77dce3a22e89 ("leds: disallow /sys/class/leds/*:multi:* for now")
> > Signed-off-by: Marek Beh'un <kabel@kernel.org>
> 
> I've just discovered this has broken boot on my Libre Computer
> AML-A311D-CC-V0.2, which was working just fine with Debian 12's stock kernel:

Sorry the the noise; I've just spotted this is fixed by:

  https://lore.kernel.org/linux-leds/20230918140724.18634-1-kabel@kernel.org/

... and I look forward to that hitting mainline :)

Mark.

Re: [PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[Lee Jones]

On Fri, 06 Oct 2023, Mark Rutland wrote:

> On Fri, Oct 06, 2023 at 03:24:04PM +0100, Mark Rutland wrote:
> > On Tue, Aug 01, 2023 at 05:16:23PM +0200, Marek Beh'un wrote:
> > > At the time we call
> > >     BUG_ON(props.color == LED_COLOR_ID_MULTI);
> > > the props variable is still initialized to zero.
> > > 
> > > Call the BUG_ON only after we parse fwnode into props.
> > > 
> > > Fixes: 77dce3a22e89 ("leds: disallow /sys/class/leds/*:multi:* for now")
> > > Signed-off-by: Marek Beh'un <kabel@kernel.org>
> > 
> > I've just discovered this has broken boot on my Libre Computer
> > AML-A311D-CC-V0.2, which was working just fine with Debian 12's stock kernel:
> 
> Sorry the the noise; I've just spotted this is fixed by:
> 
>   https://lore.kernel.org/linux-leds/20230918140724.18634-1-kabel@kernel.org/
> 
> ... and I look forward to that hitting mainline :)

Your long agonising wait is over:

commit 9dc1664fab2246bc2c3e9bf2cf21518a857f9b5b
Author: Marek Behún <kabel@kernel.org>
Date:   Mon Sep 18 16:07:24 2023 +0200

    leds: Drop BUG_ON check for LED_COLOR_ID_MULTI
    
    Commit c3f853184bed ("leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that
    is always false") fixed a no-op BUG_ON. This turned out to cause a
    regression, since some in-tree device-tree files already use
    LED_COLOR_ID_MULTI.
    
    Drop the BUG_ON altogether.
    
    Fixes: c3f853184bed ("leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false")
    Reported-by: Da Xue <da@libre.computer>
    Closes: https://lore.kernel.org/linux-leds/ZQLelWcNjjp2xndY@duo.ucw.cz/T/
    Signed-off-by: Marek Behún <kabel@kernel.org>
    Link: https://lore.kernel.org/r/20230918140724.18634-1-kabel@kernel.org
    Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/leds/led-core.c | 4 ----
 1 file changed, 4 deletions(-)

-- 
Lee Jones [李琼斯]

Re: [PATCH] leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false

[Mark Rutland]

On Fri, Oct 06, 2023 at 03:45:18PM +0100, Lee Jones wrote:
> On Fri, 06 Oct 2023, Mark Rutland wrote:
> > On Fri, Oct 06, 2023 at 03:24:04PM +0100, Mark Rutland wrote:
> > > On Tue, Aug 01, 2023 at 05:16:23PM +0200, Marek Beh'un wrote:
> > > > At the time we call
> > > >     BUG_ON(props.color == LED_COLOR_ID_MULTI);
> > > > the props variable is still initialized to zero.
> > > > 
> > > > Call the BUG_ON only after we parse fwnode into props.
> > > > 
> > > > Fixes: 77dce3a22e89 ("leds: disallow /sys/class/leds/*:multi:* for now")
> > > > Signed-off-by: Marek Beh'un <kabel@kernel.org>
> > > 
> > > I've just discovered this has broken boot on my Libre Computer
> > > AML-A311D-CC-V0.2, which was working just fine with Debian 12's stock kernel:
> > 
> > Sorry the the noise; I've just spotted this is fixed by:
> > 
> >   https://lore.kernel.org/linux-leds/20230918140724.18634-1-kabel@kernel.org/
> > 
> > ... and I look forward to that hitting mainline :)
> 
> Your long agonising wait is over:
> 
> commit 9dc1664fab2246bc2c3e9bf2cf21518a857f9b5b
> Author: Marek Beh'un <kabel@kernel.org>
> Date:   Mon Sep 18 16:07:24 2023 +0200
> 
>     leds: Drop BUG_ON check for LED_COLOR_ID_MULTI
>     
>     Commit c3f853184bed ("leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that
>     is always false") fixed a no-op BUG_ON. This turned out to cause a
>     regression, since some in-tree device-tree files already use
>     LED_COLOR_ID_MULTI.
>     
>     Drop the BUG_ON altogether.
>     
>     Fixes: c3f853184bed ("leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false")
>     Reported-by: Da Xue <da@libre.computer>
>     Closes: https://lore.kernel.org/linux-leds/ZQLelWcNjjp2xndY@duo.ucw.cz/T/
>     Signed-off-by: Marek Beh'un <kabel@kernel.org>
>     Link: https://lore.kernel.org/r/20230918140724.18634-1-kabel@kernel.org
>     Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  drivers/leds/led-core.c | 4 ----
>  1 file changed, 4 deletions(-)

Ah, doubly sorry then! I must've messed up when checking the logs for
drivers/leds/...

Thanks, regardless!

Mark.