| Message ID | 20230801151623.30387-1-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | leds: Fix BUG_ON check for LED_COLOR_ID_MULTI that is always false | expand |
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 [李琼斯]
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 > >
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.
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(-)
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.
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
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(-)