[PATCH v4] leds: group-multicolor: Add support for initial value.

[PATCH v4] leds: group-multicolor: Add support for initial value.

[Martijn de Gouw]

When the driver is loaded, it turned off all LEDs in the group. This
patch changes the driver to take over existing LED states and set
the brighness and intensity in the group accordingly.

Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
---
Changes in v4:
- Use DIV_ROUND_CLOSEST_ULL to avoid potential overflow when calculating
  the intensity
- Protect reading of led_cdev->brightness by acquire the lock.
- Link to v3: https://lore.kernel.org/linux-leds/20260316201321.2789158-1-martijn.de.gouw@prodrive-technologies.com/

Changes in v3:
- Use is_on boolean instead of storing the max_brightness twice
- Link to v2: https://lore.kernel.org/linux-leds/20251124210521.2064660-1-martijn.de.gouw@prodrive-technologies.com/

Changes in v2:
- Fix multiline comments
- Improve comments
- Link to v1: https://lore.kernel.org/linux-leds/20251111204556.2803878-1-martijn.de.gouw@prodrive-technologies.com/
---
 drivers/leds/rgb/leds-group-multicolor.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
index 548c7dd63ba1e..7a77ee6776255 100644
--- a/drivers/leds/rgb/leds-group-multicolor.c
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
 	struct mc_subled *subled;
 	struct leds_multicolor *priv;
 	unsigned int max_brightness = 0;
+	bool is_on = false;
 	int i, ret, count = 0, common_flags = 0;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -96,6 +97,15 @@ static int leds_gmc_probe(struct platform_device *pdev)
 
 		max_brightness = max(max_brightness, led_cdev->max_brightness);
 
+		/*
+		 * If any LED is on, set brightness to the max brightness.
+		 * The actual brightness of the LED is set as intensity value.
+		 */
+		mutex_lock(&led_cdev->led_access);
+		if (led_cdev->brightness)
+			is_on = true;
+		mutex_unlock(&led_cdev->led_access);
+
 		count++;
 	}
 
@@ -109,14 +119,22 @@ 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;
+		/* Configure the LED intensity to its current brightness */
+		if (led_cdev->max_brightness) {
+			mutex_lock(&led_cdev->led_access);
+			subled[i].intensity = DIV_ROUND_CLOSEST_ULL((u64)led_cdev->brightness *
+								    max_brightness,
+								    led_cdev->max_brightness);
+			mutex_unlock(&led_cdev->led_access);
+		} else
+			subled[i].intensity = 0;
 	}
 
 	/* Initialise the multicolor's LED class device */
 	cdev = &priv->mc_cdev.led_cdev;
 	cdev->brightness_set_blocking = leds_gmc_set;
 	cdev->max_brightness = max_brightness;
+	cdev->brightness = is_on ? max_brightness : 0;
 	cdev->color = LED_COLOR_ID_MULTI;
 	priv->mc_cdev.num_colors = count;
 
-- 
2.39.2

Re: [PATCH v4] leds: group-multicolor: Add support for initial value.

[Lee Jones]

/* Sashiko Automation: Issues Found (5 Findings) */

Please answer or act upon these Sashiko reviews:

On Tue, 02 Jun 2026, Martijn de Gouw wrote:
> When the driver is loaded, it turned off all LEDs in the group. This
> patch changes the driver to take over existing LED states and set
> the brighness and intensity in the group accordingly.
> 
> Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
> ---
> Changes in v4:
> - Use DIV_ROUND_CLOSEST_ULL to avoid potential overflow when calculating
>   the intensity
> - Protect reading of led_cdev->brightness by acquire the lock.
> - Link to v3: https://lore.kernel.org/linux-leds/20260316201321.2789158-1-martijn.de.gouw@prodrive-technologies.com/
> 
> Changes in v3:
> - Use is_on boolean instead of storing the max_brightness twice
> - Link to v2: https://lore.kernel.org/linux-leds/20251124210521.2064660-1-martijn.de.gouw@prodrive-technologies.com/
> 
> Changes in v2:
> - Fix multiline comments
> - Improve comments
> - Link to v1: https://lore.kernel.org/linux-leds/20251111204556.2803878-1-martijn.de.gouw@prodrive-technologies.com/
> ---
>  drivers/leds/rgb/leds-group-multicolor.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
> index 548c7dd63ba1e..7a77ee6776255 100644
> --- a/drivers/leds/rgb/leds-group-multicolor.c
> +++ b/drivers/leds/rgb/leds-group-multicolor.c
> @@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
>  	struct mc_subled *subled;
>  	struct leds_multicolor *priv;
>  	unsigned int max_brightness = 0;
> +	bool is_on = false;
>  	int i, ret, count = 0, common_flags = 0;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -96,6 +97,15 @@ static int leds_gmc_probe(struct platform_device *pdev)
>  
>  		max_brightness = max(max_brightness, led_cdev->max_brightness);
>  
> +		/*
> +		 * If any LED is on, set brightness to the max brightness.
> +		 * The actual brightness of the LED is set as intensity value.
> +		 */
> +		mutex_lock(&led_cdev->led_access);
> +		if (led_cdev->brightness)
> +			is_on = true;
> +		mutex_unlock(&led_cdev->led_access);

[Severity: High]
Does reading led_cdev->brightness directly risk getting a stale value?

If any of the monochromatic LED drivers implement the brightness_get()
callback, they might change autonomously by hardware or firmware. Should
this code call led_update_brightness() to correctly query the actual
hardware brightness before attempting to preserve state?



[Severity: Medium]
Can userspace alter a sub-LED's brightness concurrently between this loop
and the next one?

Since individual sub-LEDs are fully registered devices whose sysfs
interfaces are not disabled until the end of the probe function, their
brightness could change from > 0 to 0 after this first loop. If that
happens, is_on remains true, but the second loop would compute an
intensity of 0.


> +
>  		count++;
>  	}
>  
> @@ -109,14 +119,22 @@ 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;
> +		/* Configure the LED intensity to its current brightness */
> +		if (led_cdev->max_brightness) {
> +			mutex_lock(&led_cdev->led_access);
> +			subled[i].intensity = DIV_ROUND_CLOSEST_ULL((u64)led_cdev->brightness *
> +								    max_brightness,
> +								    led_cdev->max_brightness);
> +			mutex_unlock(&led_cdev->led_access);
> +		} else
> +			subled[i].intensity = 0;
>  	}

[Severity: High]
Does initializing the intensity based on the current brightness break
standard sysfs control and permanently cap the dynamic range?

If all sub-LEDs are off during probe, intensity becomes 0. Because
leds_gmc_set() calculates physical brightness by multiplying the requested
brightness with this intensity, the resulting hardware brightness will
always be 0 unless userspace manually writes to the multi_intensity file.

Additionally, if LEDs are dimmed during probe (e.g., 50% brightness), that
dimming factor is permanently baked into the intensity array, artificially
capping the maximum possible brightness the user can request.


>  
>  	/* Initialise the multicolor's LED class device */
>  	cdev = &priv->mc_cdev.led_cdev;
>  	cdev->brightness_set_blocking = leds_gmc_set;
>  	cdev->max_brightness = max_brightness;
> +	cdev->brightness = is_on ? max_brightness : 0;
>  	cdev->color = LED_COLOR_ID_MULTI;
>  	priv->mc_cdev.num_colors = count;

[Severity: High]
This is a pre-existing issue, but the group LED device seems to be exposed
to userspace before its initial hardware state is synchronized.

Further down in leds_gmc_probe(), devm_led_classdev_multicolor_register_ext()
is called before leds_gmc_set(). Once registered, the multi-color LED is
visible in sysfs, and userspace can immediately write to its attributes.
These sysfs writes correctly acquire the group's cdev->led_access mutex.

However, probe continues and calls leds_gmc_set() directly without holding
the mutex:

    ret = leds_gmc_set(cdev, cdev->brightness);

Can this result in concurrent executions of leds_gmc_set() corrupting the
hardware state?



[Severity: High]
This is a pre-existing issue, but does the 32-bit integer math in
leds_gmc_set() overflow for hardware with high-resolution max_brightness?

leds_gmc_set() calculates physical brightness using:

    mono_brightness = DIV_ROUND_CLOSEST(brightness * intensity * mono_max_brightness,
                                        group_max_brightness * group_max_brightness);

If the underlying monochromatic LED components use a high-resolution
max_brightness value (e.g., 4095 or 65535), their product can easily exceed
UINT_MAX. This would silently overflow the 32-bit integer limit before
division, resulting in wrapped hardware brightness values being sent to the
LED driver.

>  
> -- 
> 2.39.2
> 

-- 
Lee Jones