Commit 2c70953b authored Feb 20, 2025 by Remi Pommarel Committed by Lee Jones Feb 27, 2025

leds: Fix LED_OFF brightness race



While commit fa15d8c6 ("leds: Fix set_brightness_delayed() race")
successfully forces led_set_brightness() to be called with LED_OFF at
least once when switching from blinking to LED on state so that
hw-blinking can be disabled, another race remains. Indeed in
led_set_brightness(LED_OFF) followed by led_set_brightness(any)
scenario the following CPU scheduling can happen:

    CPU0                                     CPU1
    ----                                     ----
 set_brightness_delayed() {
   test_and_clear_bit(BRIGHTNESS_OFF)
                                         led_set_brightness(LED_OFF) {
                                           set_bit(BRIGHTNESS_OFF)
					   queue_work()
                                         }
                                         led_set_brightness(any) {
                                           set_bit(BRIGHTNESS)
					   queue_work() //already queued
                                         }
   test_and_clear_bit(BRIGHTNESS)
     /* LED set with brightness any */
 }

 /* From previous CPU1 queue_work() */
 set_brightness_delayed() {
   test_and_clear_bit(BRIGHTNESS_OFF)
     /* LED turned off */
   test_and_clear_bit(BRIGHTNESS)
     /* Clear from previous run, LED remains off */

In that case the led_set_brightness(LED_OFF)/led_set_brightness(any)
sequence will be effectively executed in reverse order and LED will
remain off.

With the introduction of commit 32360bf6 ("leds: Introduce ordered
workqueue for LEDs events instead of system_wq") the race is easier to
trigger as sysfs brightness configuration does not wait for
set_brightness_delayed() work to finish (flush_work() removal).

Use delayed_set_value to optionnally re-configure brightness after a
LED_OFF. That way a LED state could be configured more that once but
final state will always be as expected. Ensure that delayed_set_value
modification is seen before set_bit() using smp_mb__before_atomic().

Fixes: fa15d8c6 ("leds: Fix set_brightness_delayed() race")
Signed-off-by: Remi Pommarel <repk@triplefau.lt>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/19c81177059dab7b656c42063958011a8e4d1a66.1740050412.git.repk@triplefau.lt


Signed-off-by: Lee Jones <lee@kernel.org>

parent 012825db

drivers/leds/led-core.c

+18 −4

Original line number	Diff line number	Diff line
		@@ -159,8 +159,19 @@ static void set_brightness_delayed(struct work_struct *ws)
		* before this work item runs once. To make sure this works properly
		* handle LED_SET_BRIGHTNESS_OFF first.
		*/
		if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags))
		if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags)) {
		set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
		/*
		* The consecutives led_set_brightness(LED_OFF),
		* led_set_brightness(LED_FULL) could have been executed out of
		* order (LED_FULL first), if the work_flags has been set
		* between LED_SET_BRIGHTNESS_OFF and LED_SET_BRIGHTNESS of this
		* work. To avoid ending with the LED turned off, turn the LED
		* on again.
		*/
		if (led_cdev->delayed_set_value != LED_OFF)
		set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
		}

		if (test_and_clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags))
		set_brightness_delayed_set_brightness(led_cdev, led_cdev->delayed_set_value);
		@@ -331,10 +342,13 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
		* change is done immediately afterwards (before the work runs),
		* it uses a separate work_flag.
		*/
		if (value) {
		led_cdev->delayed_set_value = value;
		/* Ensure delayed_set_value is seen before work_flags modification */
		smp_mb__before_atomic();

		if (value)
		set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
		} else {
		else {
		clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
		clear_bit(LED_SET_BLINK, &led_cdev->work_flags);
		set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);