led: core: Use atomic bit-field for the blink-flags

All the LED_BLINK* flags are accessed read-modify-write from e.g.
led_set_brightness and led_blink_set_oneshot while both
set_brightness_work and the blink_timer may be running.

If these race then the modify step done by one of them may be lost,
switch the LED_BLINK* flags to a new atomic work_flags bit-field
to avoid this race.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index d946eb1667815755a981832b0644fe56b73cd781..326ee6e925a205531f97f0815451e4af87505ceb 100644 (file)
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -204,6 +204,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
                dev_warn(parent, "Led %s renamed to %s due to name collision",
                                led_cdev->name, dev_name(led_cdev->dev));
 
+       led_cdev->work_flags = 0;
 #ifdef CONFIG_LEDS_TRIGGERS
        init_rwsem(&led_cdev->trigger_lock);
 #endif

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 3bce44893021365a999724a9aff0cf17d462a95f..bd6bb4d44f054b1739ec5cb16b9c95471d6cf044 100644 (file)
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -53,12 +53,13 @@ static void led_timer_function(unsigned long data)
 
        if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
                led_set_brightness_nosleep(led_cdev, LED_OFF);
-               led_cdev->flags &= ~LED_BLINK_SW;
+               clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
                return;
        }
 
-       if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) {
-               led_cdev->flags &=  ~(LED_BLINK_ONESHOT_STOP | LED_BLINK_SW);
+       if (test_and_clear_bit(LED_BLINK_ONESHOT_STOP,
+                              &led_cdev->work_flags)) {
+               clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
                return;
        }
 
@@ -73,10 +74,9 @@ static void led_timer_function(unsigned long data)
                 * Do it only if there is no pending blink brightness
                 * change, to avoid overwriting the new value.
                 */
-               if (!(led_cdev->flags & LED_BLINK_BRIGHTNESS_CHANGE))
+               if (!test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE,
+                                       &led_cdev->work_flags))
                        led_cdev->blink_brightness = brightness;
-               else
-                       led_cdev->flags &= ~LED_BLINK_BRIGHTNESS_CHANGE;
                brightness = LED_OFF;
                delay = led_cdev->blink_delay_off;
        }
@@ -87,13 +87,15 @@ static void led_timer_function(unsigned long data)
         * the final blink state so that the led is toggled each delay_on +
         * delay_off milliseconds in worst case.
         */
-       if (led_cdev->flags & LED_BLINK_ONESHOT) {
-               if (led_cdev->flags & LED_BLINK_INVERT) {
+       if (test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags)) {
+               if (test_bit(LED_BLINK_INVERT, &led_cdev->work_flags)) {
                        if (brightness)
-                               led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
+                               set_bit(LED_BLINK_ONESHOT_STOP,
+                                       &led_cdev->work_flags);
                } else {
                        if (!brightness)
-                               led_cdev->flags |= LED_BLINK_ONESHOT_STOP;
+                               set_bit(LED_BLINK_ONESHOT_STOP,
+                                       &led_cdev->work_flags);
                }
        }
 
@@ -106,10 +108,9 @@ static void set_brightness_delayed(struct work_struct *ws)
                container_of(ws, struct led_classdev, set_brightness_work);
        int ret = 0;
 
-       if (led_cdev->flags & LED_BLINK_DISABLE) {
+       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);
-               led_cdev->flags &= ~LED_BLINK_DISABLE;
        }
 
        ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
@@ -152,7 +153,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
                return;
        }
 
-       led_cdev->flags |= LED_BLINK_SW;
+       set_bit(LED_BLINK_SW, &led_cdev->work_flags);
        mod_timer(&led_cdev->blink_timer, jiffies + 1);
 }
 
@@ -161,7 +162,7 @@ static void led_blink_setup(struct led_classdev *led_cdev,
                     unsigned long *delay_on,
                     unsigned long *delay_off)
 {
-       if (!(led_cdev->flags & LED_BLINK_ONESHOT) &&
+       if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
            led_cdev->blink_set &&
            !led_cdev->blink_set(led_cdev, delay_on, delay_off))
                return;
@@ -188,8 +189,8 @@ void led_blink_set(struct led_classdev *led_cdev,
 {
        del_timer_sync(&led_cdev->blink_timer);
 
-       led_cdev->flags &= ~LED_BLINK_ONESHOT;
-       led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
+       clear_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags);
+       clear_bit(LED_BLINK_ONESHOT_STOP, &led_cdev->work_flags);
 
        led_blink_setup(led_cdev, delay_on, delay_off);
 }
@@ -200,17 +201,17 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
                           unsigned long *delay_off,
                           int invert)
 {
-       if ((led_cdev->flags & LED_BLINK_ONESHOT) &&
+       if (test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
             timer_pending(&led_cdev->blink_timer))
                return;
 
-       led_cdev->flags |= LED_BLINK_ONESHOT;
-       led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP;
+       set_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags);
+       clear_bit(LED_BLINK_ONESHOT_STOP, &led_cdev->work_flags);
 
        if (invert)
-               led_cdev->flags |= LED_BLINK_INVERT;
+               set_bit(LED_BLINK_INVERT, &led_cdev->work_flags);
        else
-               led_cdev->flags &= ~LED_BLINK_INVERT;
+               clear_bit(LED_BLINK_INVERT, &led_cdev->work_flags);
 
        led_blink_setup(led_cdev, delay_on, delay_off);
 }
@@ -221,7 +222,7 @@ void led_stop_software_blink(struct led_classdev *led_cdev)
        del_timer_sync(&led_cdev->blink_timer);
        led_cdev->blink_delay_on = 0;
        led_cdev->blink_delay_off = 0;
-       led_cdev->flags &= ~LED_BLINK_SW;
+       clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
 }
 EXPORT_SYMBOL_GPL(led_stop_software_blink);
 
@@ -232,17 +233,18 @@ void led_set_brightness(struct led_classdev *led_cdev,
         * If software blink is active, delay brightness setting
         * until the next timer tick.
         */
-       if (led_cdev->flags & LED_BLINK_SW) {
+       if (test_bit(LED_BLINK_SW, &led_cdev->work_flags)) {
                /*
                 * If we need to disable soft blinking delegate this to the
                 * work queue task to avoid problems in case we are called
                 * from hard irq context.
                 */
                if (brightness == LED_OFF) {
-                       led_cdev->flags |= LED_BLINK_DISABLE;
+                       set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
                        schedule_work(&led_cdev->set_brightness_work);
                } else {
-                       led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE;
+                       set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
+                               &led_cdev->work_flags);
                        led_cdev->blink_brightness = brightness;
                }
                return;

diff --git a/include/linux/leds.h b/include/linux/leds.h
index ddfcb2df3656cf0ab6aebd1fa3d624a6ec2e94e9..21c598b366f834d5044ac319bbe26c00b3d19a98 100644 (file)
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -42,16 +42,20 @@ struct led_classdev {
 #define LED_UNREGISTERING      (1 << 1)
        /* Upper 16 bits reflect control information */
 #define LED_CORE_SUSPENDRESUME (1 << 16)
-#define LED_BLINK_SW           (1 << 17)
-#define LED_BLINK_ONESHOT      (1 << 18)
-#define LED_BLINK_ONESHOT_STOP (1 << 19)
-#define LED_BLINK_INVERT       (1 << 20)
-#define LED_BLINK_BRIGHTNESS_CHANGE (1 << 21)
-#define LED_BLINK_DISABLE      (1 << 22)
-#define LED_SYSFS_DISABLE      (1 << 23)
-#define LED_DEV_CAP_FLASH      (1 << 24)
-#define LED_HW_PLUGGABLE       (1 << 25)
-#define LED_PANIC_INDICATOR    (1 << 26)
+#define LED_SYSFS_DISABLE      (1 << 17)
+#define LED_DEV_CAP_FLASH      (1 << 18)
+#define LED_HW_PLUGGABLE       (1 << 19)
+#define LED_PANIC_INDICATOR    (1 << 20)
+
+       /* set_brightness_work / blink_timer flags, atomic, private. */
+       unsigned long           work_flags;
+
+#define LED_BLINK_SW                   0
+#define LED_BLINK_ONESHOT              1
+#define LED_BLINK_ONESHOT_STOP         2
+#define LED_BLINK_INVERT               3
+#define LED_BLINK_BRIGHTNESS_CHANGE    4
+#define LED_BLINK_DISABLE              5
 
        /* Set LED brightness level
         * Must not sleep. Use brightness_set_blocking for drivers