leds: Fix LED_OFF brightness race

While commit fa15d8c69238 ("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 32360bf6a5d4 ("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: fa15d8c69238 ("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>

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f6c46d2e5276b5eb6c6862c5e02017bf017c137d..e3d8ddcff5670e390cca121c8747e4a504fc68d4 100644 (file)
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -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;
+       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);