leds: Fix set_brightness_delayed() race

When a trigger wants to switch from blinking to LED on it needs to call:
	led_set_brightness(LED_OFF);
	led_set_brightness(LED_FULL);

To first call disables blinking and the second then turns the LED on
(the power-supply charging-blink-full-solid triggers do this).

These calls happen immediately after each other, so it is possible
that set_brightness_delayed() from the first call has not run yet
when the led_set_brightness(LED_FULL) call finishes.

If this race hits then this is causing problems for both
sw- and hw-blinking:

For sw-blinking set_brightness_delayed() clears delayed_set_value
when LED_BLINK_DISABLE is set causing the led_set_brightness(LED_FULL)
call effects to get lost when hitting the race, resulting in the LED
turning off instead of on.

For hw-blinking if the race hits delayed_set_value has been
set to LED_FULL by the time set_brightness_delayed() runs.
So led_cdev->brightness_set_blocking() is never called with
LED_OFF as argument and the hw-blinking is never disabled leaving
the LED blinking instead of on.

Fix both issues by adding LED_SET_BRIGHTNESS and LED_SET_BRIGHTNESS_OFF
work_flags making this 2 separate actions to be run by
set_brightness_delayed().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>
Link: https://lore.kernel.org/r/20230510162234.291439-3-hdegoede@redhat.com
Signed-off-by: Lee Jones <lee@kernel.org>

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 4a97cb7457888c6dd3c265985548163209f8d647..e61acc785410f71094d1133335e0c07f3618504e 100644 (file)
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -114,21 +114,14 @@ static void led_timer_function(struct timer_list *t)
        mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
 }
 
-static void set_brightness_delayed(struct work_struct *ws)
+static void set_brightness_delayed_set_brightness(struct led_classdev *led_cdev,
+                                                 unsigned int value)
 {
-       struct led_classdev *led_cdev =
-               container_of(ws, struct led_classdev, set_brightness_work);
        int ret = 0;
 
-       if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
-               led_cdev->delayed_set_value = LED_OFF;
-               led_stop_software_blink(led_cdev);
-       }
-
-       ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
+       ret = __led_set_brightness(led_cdev, value);
        if (ret == -ENOTSUPP)
-               ret = __led_set_brightness_blocking(led_cdev,
-                                       led_cdev->delayed_set_value);
+               ret = __led_set_brightness_blocking(led_cdev, value);
        if (ret < 0 &&
            /* LED HW might have been unplugged, therefore don't warn */
            !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -137,6 +130,30 @@ static void set_brightness_delayed(struct work_struct *ws)
                        "Setting an LED's brightness failed (%d)\n", ret);
 }
 
+static void set_brightness_delayed(struct work_struct *ws)
+{
+       struct led_classdev *led_cdev =
+               container_of(ws, struct led_classdev, set_brightness_work);
+
+       if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
+               led_stop_software_blink(led_cdev);
+               set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
+       }
+
+       /*
+        * Triggers may call led_set_brightness(LED_OFF),
+        * led_set_brightness(LED_FULL) in quick succession to disable blinking
+        * and turn the LED on. Both actions may have been scheduled to run
+        * 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))
+               set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
+
+       if (test_and_clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags))
+               set_brightness_delayed_set_brightness(led_cdev, led_cdev->delayed_set_value);
+}
+
 static void led_set_software_blink(struct led_classdev *led_cdev,
                                   unsigned long delay_on,
                                   unsigned long delay_off)
@@ -271,8 +288,22 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
        if (!__led_set_brightness(led_cdev, value))
                return;
 
-       /* If brightness setting can sleep, delegate it to a work queue task */
-       led_cdev->delayed_set_value = value;
+       /*
+        * Brightness setting can sleep, delegate it to a work queue task.
+        * value 0 / LED_OFF is special, since it also disables hw-blinking
+        * (sw-blink disable is handled in led_set_brightness()).
+        * To avoid a hw-blink-disable getting lost when a second brightness
+        * change is done immediately afterwards (before the work runs),
+        * it uses a separate work_flag.
+        */
+       if (value) {
+               led_cdev->delayed_set_value = value;
+               set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+       } else {
+               clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+               set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
+       }
+
        schedule_work(&led_cdev->set_brightness_work);
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_nopm);

diff --git a/include/linux/leds.h b/include/linux/leds.h
index c3dc22d184e239d70a41c0356a51c36f67dccda4..de813fe96a2031322f922e2f0b9462c2cdf4ce84 100644 (file)
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -124,6 +124,9 @@ struct led_classdev {
 #define LED_BLINK_INVERT               3
 #define LED_BLINK_BRIGHTNESS_CHANGE    4
 #define LED_BLINK_DISABLE              5
+       /* Brightness off also disables hw-blinking so it is a separate action */
+#define LED_SET_BRIGHTNESS_OFF         6
+#define LED_SET_BRIGHTNESS             7
 
        /* Set LED brightness level
         * Must not sleep. Use brightness_set_blocking for drivers