* [PATCH v2] led: core: Use atomic bit-field for the blink-flags
@ 2016-11-08 13:58 ` Jacek Anaszewski
2016-11-08 14:01 ` Jacek Anaszewski
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jacek Anaszewski @ 2016-11-08 13:58 UTC (permalink / raw)
To: linux-leds; +Cc: linux-kernel, Hans de Goede, Jacek Anaszewski
From: Hans de Goede <hdegoede@redhat.com>
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>
---
Changes since v1:
- keep set_brightness_work in linux/leds.h
drivers/leds/led-class.c | 1 +
drivers/leds/led-core.c | 52 +++++++++++++++++++++++++-----------------------
include/linux/leds.h | 24 ++++++++++++----------
3 files changed, 42 insertions(+), 35 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index c8d2d67..b12f861 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -212,6 +212,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
return -ENODEV;
}
+ 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 e2e5cc7..620257a 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -61,12 +61,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;
}
@@ -81,10 +82,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;
}
@@ -95,13 +95,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);
}
}
@@ -114,10 +116,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);
@@ -160,7 +161,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);
}
@@ -169,7 +170,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;
@@ -196,8 +197,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);
}
@@ -208,17 +209,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);
}
@@ -229,7 +230,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);
@@ -240,17 +241,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 eebcd8c..1dc69df 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -43,16 +43,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
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] led: core: Use atomic bit-field for the blink-flags
2016-11-08 13:58 ` [PATCH v2] led: core: Use atomic bit-field for the blink-flags Jacek Anaszewski
@ 2016-11-08 14:01 ` Jacek Anaszewski
2016-11-09 11:47 ` Jacek Anaszewski
2016-11-09 11:51 ` Hans de Goede
2 siblings, 0 replies; 4+ messages in thread
From: Jacek Anaszewski @ 2016-11-08 14:01 UTC (permalink / raw)
To: linux-leds; +Cc: linux-kernel, Hans de Goede
On 11/08/2016 02:58 PM, Jacek Anaszewski wrote:
> From: Hans de Goede <hdegoede@redhat.com>
>
> 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>
> ---
> Changes since v1:
> - keep set_brightness_work in linux/leds.h
Should be:
- keep set_brightness_work flags in linux/leds.h
>
> drivers/leds/led-class.c | 1 +
> drivers/leds/led-core.c | 52 +++++++++++++++++++++++++-----------------------
> include/linux/leds.h | 24 ++++++++++++----------
> 3 files changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index c8d2d67..b12f861 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
> return -ENODEV;
> }
>
> + 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 e2e5cc7..620257a 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -61,12 +61,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;
> }
>
> @@ -81,10 +82,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;
> }
> @@ -95,13 +95,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);
> }
> }
>
> @@ -114,10 +116,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);
> @@ -160,7 +161,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);
> }
>
> @@ -169,7 +170,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;
> @@ -196,8 +197,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);
> }
> @@ -208,17 +209,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);
> }
> @@ -229,7 +230,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);
>
> @@ -240,17 +241,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 eebcd8c..1dc69df 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -43,16 +43,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
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] led: core: Use atomic bit-field for the blink-flags
2016-11-08 13:58 ` [PATCH v2] led: core: Use atomic bit-field for the blink-flags Jacek Anaszewski
2016-11-08 14:01 ` Jacek Anaszewski
@ 2016-11-09 11:47 ` Jacek Anaszewski
2016-11-09 11:51 ` Hans de Goede
2 siblings, 0 replies; 4+ messages in thread
From: Jacek Anaszewski @ 2016-11-09 11:47 UTC (permalink / raw)
To: linux-leds; +Cc: linux-kernel, Hans de Goede
Since we've already agreed on keeping the flags in the leds.h,
then I'm applying this patch to the for-next branch of linux-leds.git.
Thanks,
Jacek Anaszewski
On 11/08/2016 02:58 PM, Jacek Anaszewski wrote:
> From: Hans de Goede <hdegoede@redhat.com>
>
> 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>
> ---
> Changes since v1:
> - keep set_brightness_work in linux/leds.h
>
> drivers/leds/led-class.c | 1 +
> drivers/leds/led-core.c | 52 +++++++++++++++++++++++++-----------------------
> include/linux/leds.h | 24 ++++++++++++----------
> 3 files changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index c8d2d67..b12f861 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
> return -ENODEV;
> }
>
> + 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 e2e5cc7..620257a 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -61,12 +61,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;
> }
>
> @@ -81,10 +82,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;
> }
> @@ -95,13 +95,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);
> }
> }
>
> @@ -114,10 +116,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);
> @@ -160,7 +161,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);
> }
>
> @@ -169,7 +170,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;
> @@ -196,8 +197,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);
> }
> @@ -208,17 +209,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);
> }
> @@ -229,7 +230,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);
>
> @@ -240,17 +241,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 eebcd8c..1dc69df 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -43,16 +43,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
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] led: core: Use atomic bit-field for the blink-flags
2016-11-08 13:58 ` [PATCH v2] led: core: Use atomic bit-field for the blink-flags Jacek Anaszewski
2016-11-08 14:01 ` Jacek Anaszewski
2016-11-09 11:47 ` Jacek Anaszewski
@ 2016-11-09 11:51 ` Hans de Goede
2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2016-11-09 11:51 UTC (permalink / raw)
To: Jacek Anaszewski, linux-leds; +Cc: linux-kernel
Hi,
On 08-11-16 14:58, Jacek Anaszewski wrote:
> From: Hans de Goede <hdegoede@redhat.com>
>
> 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>
> ---
> Changes since v1:
> - keep set_brightness_work in linux/leds.h
Thank you.
Patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
>
> drivers/leds/led-class.c | 1 +
> drivers/leds/led-core.c | 52 +++++++++++++++++++++++++-----------------------
> include/linux/leds.h | 24 ++++++++++++----------
> 3 files changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index c8d2d67..b12f861 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -212,6 +212,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
> return -ENODEV;
> }
>
> + 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 e2e5cc7..620257a 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -61,12 +61,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;
> }
>
> @@ -81,10 +82,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;
> }
> @@ -95,13 +95,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);
> }
> }
>
> @@ -114,10 +116,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);
> @@ -160,7 +161,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);
> }
>
> @@ -169,7 +170,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;
> @@ -196,8 +197,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);
> }
> @@ -208,17 +209,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);
> }
> @@ -229,7 +230,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);
>
> @@ -240,17 +241,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 eebcd8c..1dc69df 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -43,16 +43,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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-09 11:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20161108135903epcas5p376d72ae6d2f6cc518afe7219474d607e@epcas5p3.samsung.com>
2016-11-08 13:58 ` [PATCH v2] led: core: Use atomic bit-field for the blink-flags Jacek Anaszewski
2016-11-08 14:01 ` Jacek Anaszewski
2016-11-09 11:47 ` Jacek Anaszewski
2016-11-09 11:51 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).