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

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

[Rong Zhang]

Since the Fixes: commit, the brightness attribute becomes write-only
when the LED is controlled fully by the hardware. A write-only attribute
is very confusing.

Moreover, the LED driver may disable hardware control upon brightness
set, but the hardware control trigger remains active, resulting in the
software and hardware being out of sync.

Fix it by disabling hardware control when the brightness attribute is
written.

This should also match the semantics of hardware control:

    When the LED is in hw control, no software blink is possible and
    doing so will effectively disable hw control.

Fixes: b819dc7d8fb2 ("leds: core: Report ENODATA for brightness of hardware controlled LED")
Signed-off-by: Rong Zhang <i@rong.moe>
---
Note: led_trigger_is_hw_controlled() currently returns true only for
private triggers, as there is no way for the LED core to know if a
public hardware control trigger is in hardware control mode.

More discussions at https://lore.kernel.org/all/a0c35498226b33eebacf7641aed126ffc8f99456.camel@rong.moe/
---
 drivers/leds/led-class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index a51b0ed53886..3e25e5fb63dc 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);
 	led_set_brightness(led_cdev, state);
 

---
base-commit: cab9e339cfbc1a4e075e53e281dfb00391e1a6bb
change-id: f8fb6869-leds-hw-control-brightness-set-b18b4f5ab09b

Thanks,
Rong

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

[sashiko-bot]

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.
--

--- Patch [1]: [PATCH] leds: core: Disable hardware control when writing brightness ---
commit 86a852e7c49262f7479b1653712185d4edd86c24
Author: Rong Zhang <i@rong.moe>

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);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-leds-hw-control-brightness-set-v1-1-1de593b09d26@rong.moe?part=1