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

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-inten=
sity`

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 d=
efault-intensity ---
commit 3299ece4693e9a064de7eb3b6ca548536d4ca2ef
Author: Stefan Wahren <[email protected]>

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

Currently it is not possible to specify the initial color of a LED multicol=
or
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 *pd=
ev)
> =20
>  		subled[i].color_index =3D led_cdev->color;
> =20
> -		/* Configure the LED intensity to its maximum */
> -		subled[i].intensity =3D max_brightness;
> +		ret =3D fwnode_property_read_u32(led_cdev->dev->fwnode, "default-inten=
sity",
> +					       &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, t=
he
default-intensity property is expected to be located in the parent multicol=
or
group node.

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

    ret =3D fwnode_property_read_u32(led_cdev->dev->fwnode, "default-intens=
ity",
                                   &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 =3D max_brightness;
> +		else if (subled[i].intensity > max_brightness)
> +			subled[i].intensity =3D max_brightness;
>  	}
> =20
>  	/* 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-LED=
s,
it uses 32-bit unsigned arithmetic:

drivers/leds/rgb/leds-group-multicolor.c:leds_gmc_set() {
    ...
    mono_brightness =3D DIV_ROUND_CLOSEST(brightness * intensity * mono_max=
_brightness,
                                        group_max_brightness * group_max_br=
ightness);
    ...
}

If the underlying LEDs have high max_brightness values configured (for exam=
ple,
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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802115727.5041=
[email protected]?part=3D2