Re: [PATCH] leds: core: Disable hardware control when writing brightness

[Rong Zhang]

Hi all,

On Sat, 2026-07-11 at 18:52 +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] TOCTOU race in brightness_store() can cause unintended removal of a newly registered trigger.

I will integrate this into my hardware-initiated hardware control
trigger transition series [1] to make things easier, as it will anyway
move and refine led_trigger_is_hw_controlled().

[1]: https://lore.kernel.org/all/20260618-leds-trigger-hw-changed-v2-1-c28c44053cf3@xxxxxxxx/

Thanks,
Rong

> --
> 
> --- Patch [1]: [PATCH] leds: core: Disable hardware control when writing brightness ---
> commit 86a852e7c49262f7479b1653712185d4edd86c24
> Author: Rong Zhang <i@xxxxxxxx>
> 
> leds: core: Disable hardware control when writing brightness
> 
> This commit modifies the brightness sysfs attribute to disable hardware control
> when written. It addresses an issue where the brightness attribute becomes
> confusingly write-only while hardware control remains active.
> 
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index a51b0ed538864..3e25e5fb63dc8 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -72,7 +72,7 @@ static ssize_t brightness_store(struct device *dev,
> >  	if (ret)
> >  		goto unlock;
> >  
> > -	if (state == LED_OFF)
> > +	if (state == LED_OFF || led_trigger_is_hw_controlled(led_cdev))
> >  		led_trigger_remove(led_cdev);
> 
> [Severity: Medium]
> Could this introduce a race condition that removes an unrelated trigger?
> 
> led_trigger_is_hw_controlled() takes and drops the read-side of trigger_lock:
> 
> drivers/leds/led-class.c:led_trigger_is_hw_controlled() {
> 	...
> 	guard(rwsem_read)(&led_cdev->trigger_lock);
> 	return led_cdev->trigger && led_cdev->trigger->trigger_type;
> }
> 
> Since the lock is dropped before led_trigger_remove() acquires the write-side
> lock, is it possible for a concurrent thread to unregister the hardware
> trigger and attach a new default trigger in the window between the check and
> the removal? 
> 
> If that sequence occurs, led_trigger_remove() might inadvertently remove the
> newly attached trigger instead of the intended hardware trigger.
> 
> >  	led_set_brightness(led_cdev, state);
> >