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>

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

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;

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