Re: [PATCH V2 2/2] leds: rgb: leds-group-multicolor: Implement default-intensity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Device Tree Binding & Implementation Mismatch for `default-intensity`

Pre-existing issues:
- [High] 32-bit unsigned integer overflow and potential division by zero in brightness scaling calculation
--

--- Patch [2]: [PATCH V2 2/2] leds: rgb: leds-group-multicolor: Implement default-intensity ---
commit 3299ece4693e9a064de7eb3b6ca548536d4ca2ef
Author: Stefan Wahren <wahrenst@gmx.net>

leds: rgb: leds-group-multicolor: Implement default-intensity

Currently it is not possible to specify the initial color of a LED multicolor
group during boot. This commit implements the default-intensity property
similar to the leds-pwm-multicolor driver, falling back to the maximum
intensity if the property is missing.

> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
> index a707d51c6a4bd..f0e6c2a25a070 100644
> --- a/drivers/leds/rgb/leds-group-multicolor.c
> +++ b/drivers/leds/rgb/leds-group-multicolor.c
> @@ -108,8 +108,14 @@ static int leds_gmc_probe(struct platform_device *pdev)
>  
>  		subled[i].color_index = led_cdev->color;
>  
> -		/* Configure the LED intensity to its maximum */
> -		subled[i].intensity = max_brightness;
> +		ret = fwnode_property_read_u32(led_cdev->dev->fwnode, "default-intensity",
> +					       &subled[i].intensity);

[Severity: Medium]
Will this parse the default-intensity property from the correct node?

Based on the device tree binding documentation for leds-group-multicolor, the
default-intensity property is expected to be located in the parent multicolor
group node.

However, in leds_gmc_probe(), the driver attempts to read it from the
monochromatic sub-LED fwnode instead:

    ret = fwnode_property_read_u32(led_cdev->dev->fwnode, "default-intensity",
                                   &subled[i].intensity);

Does this code ignore valid configurations where the property is placed in the
parent node according to the schema?

> +
> +		/* In case default-intensity is missing, fallback to maximum */
> +		if (ret)
> +			subled[i].intensity = max_brightness;
> +		else if (subled[i].intensity > max_brightness)
> +			subled[i].intensity = max_brightness;
>  	}
>  
>  	/* Initialise the multicolor's LED class device */

[Severity: High]
This is a pre-existing issue, but is there a potential integer overflow and
division by zero when scaling brightness in leds_gmc_set()?

When leds_gmc_set() calculates the brightness for the monochromatic sub-LEDs,
it uses 32-bit unsigned arithmetic:

drivers/leds/rgb/leds-group-multicolor.c:leds_gmc_set() {
    ...
    mono_brightness = DIV_ROUND_CLOSEST(brightness * intensity * mono_max_brightness,
                                        group_max_brightness * group_max_brightness);
    ...
}

If the underlying LEDs have high max_brightness values configured (for example,
65535 for a PWM period), the numerator can wrap around, resulting in a
calculated brightness of 0.

Furthermore, if group_max_brightness happens to be exactly 65536, the
denominator overflows to exactly 0, which would lead to a division by zero
panic during brightness adjustment.

Could this arithmetic be adjusted to prevent overflows with high brightness
values?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802115727.50411-1-wahrenst@gmx.net?part=2