lkml.org 
[lkml]   [2018]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] led: core: Fix race on software blink cancellation
From
Date
Any comments? I'd like to have some acks before applying
this patch.

Adding more people showing recently interest in the LED subsystem
related development.

Thanks,
Jacek Anaszewski

On 01/17/2018 10:12 PM, Jacek Anaszewski wrote:
> Commit d23a22a74fde ("leds: delay led_set_brightness if stopping soft-blink")
> made a modifications to the LED core allowing for led_set_brightness() to be
> called from hard-irq context when soft blink is being handled in soft-irq.
>
> Since that time LED core has undergone modifications related to addition of
> generic support for delegating brightness setting to a workqueue as well as
> subsequent fixes for blink setting use cases.
>
> After that the LED core code became hard to maintain and analyze, especially
> due to the imposed hard-irq context compatibility. It also turned out that
> in some cases a LED remained off after executing following sequence of commands:
>
> 1. echo timer > trigger
> 2. echo 0 > brightness
> 3. echo 100 > brightness
>
> The reason was LED_BLINK_DISABLE operation delegated to a set_brightness_work,
> triggered in 2., which was handled after 3. took effect.
>
> In order to serialize above operations and in the same time avoid
> code overcomplication the hard-irq context compatibility is being removed,
> which allows to use spin_lock_bh() for LED blink setting serialization.
>
>>From now, if in hard-irq context, users need to delegate led_set_brightness()
> to a workqueue in the place of use.
>
> Reported-by: Craig McQueen <craig.mcqueen@innerrange.com.au>
> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> ---
> Craig,
>
> It would be great if you could confirm if this fixes your use case.
>
> drivers/leds/led-core.c | 79 +++++++++++++++++++-------------
> drivers/leds/trigger/ledtrig-activity.c | 3 --
> drivers/leds/trigger/ledtrig-heartbeat.c | 3 --
> include/linux/leds.h | 5 +-
> 4 files changed, 49 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index ede4fa0..25d711b 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -22,6 +22,9 @@
> DECLARE_RWSEM(leds_list_lock);
> EXPORT_SYMBOL_GPL(leds_list_lock);
>
> +DEFINE_SPINLOCK(leds_soft_blink_lock);
> +EXPORT_SYMBOL_GPL(leds_soft_blink_lock);
> +
> LIST_HEAD(leds_list);
> EXPORT_SYMBOL_GPL(leds_list);
>
> @@ -51,26 +54,31 @@ static void led_timer_function(struct timer_list *t)
> unsigned long brightness;
> unsigned long delay;
>
> + spin_lock_bh(&leds_soft_blink_lock);
> +
> + /*
> + * Check if soft blinking wasn't disabled via led_set_brightness()
> + * in the meantime.
> + */
> + if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> + goto unlock;
> +
> if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) {
> led_set_brightness_nosleep(led_cdev, LED_OFF);
> clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> - return;
> + goto unlock;
> }
>
> if (test_and_clear_bit(LED_BLINK_ONESHOT_STOP,
> &led_cdev->work_flags)) {
> clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> - return;
> + goto unlock;
> }
>
> brightness = led_get_brightness(led_cdev);
> if (!brightness) {
> /* Time to switch the LED on. */
> - if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE,
> - &led_cdev->work_flags))
> - brightness = led_cdev->new_blink_brightness;
> - else
> - brightness = led_cdev->blink_brightness;
> + brightness = led_cdev->blink_brightness;
> delay = led_cdev->blink_delay_on;
> } else {
> /* Store the current brightness value to be able
> @@ -100,6 +108,9 @@ static void led_timer_function(struct timer_list *t)
> }
>
> mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> +
> +unlock:
> + spin_unlock_bh(&leds_soft_blink_lock);
> }
>
> static void set_brightness_delayed(struct work_struct *ws)
> @@ -108,11 +119,6 @@ static void set_brightness_delayed(struct work_struct *ws)
> 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);
> if (ret == -ENOTSUPP)
> ret = __led_set_brightness_blocking(led_cdev,
> @@ -131,6 +137,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
> {
> int current_brightness;
>
> + spin_lock_bh(&leds_soft_blink_lock);
> +
> current_brightness = led_get_brightness(led_cdev);
> if (current_brightness)
> led_cdev->blink_brightness = current_brightness;
> @@ -143,18 +151,21 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
> /* never on - just set to off */
> if (!delay_on) {
> led_set_brightness_nosleep(led_cdev, LED_OFF);
> - return;
> + goto unlock;
> }
>
> /* never off - just set to brightness */
> if (!delay_off) {
> led_set_brightness_nosleep(led_cdev,
> led_cdev->blink_brightness);
> - return;
> + goto unlock;
> }
>
> set_bit(LED_BLINK_SW, &led_cdev->work_flags);
> mod_timer(&led_cdev->blink_timer, jiffies + 1);
> +
> +unlock:
> + spin_unlock_bh(&leds_soft_blink_lock);
> }
>
>
> @@ -217,40 +228,46 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
> }
> EXPORT_SYMBOL_GPL(led_blink_set_oneshot);
>
> -void led_stop_software_blink(struct led_classdev *led_cdev)
> +void led_stop_software_blink_nolock(struct led_classdev *led_cdev)
> {
> - del_timer_sync(&led_cdev->blink_timer);
> led_cdev->blink_delay_on = 0;
> led_cdev->blink_delay_off = 0;
> clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> }
> +
> +void led_stop_software_blink(struct led_classdev *led_cdev)
> +{
> + spin_lock_bh(&leds_soft_blink_lock);
> + led_stop_software_blink_nolock(led_cdev);
> + spin_unlock_bh(&leds_soft_blink_lock);
> +}
> EXPORT_SYMBOL_GPL(led_stop_software_blink);
>
> void led_set_brightness(struct led_classdev *led_cdev,
> enum led_brightness brightness)
> {
> - /*
> - * If software blink is active, delay brightness setting
> - * until the next timer tick.
> - */
> + if (in_irq()) {
> + dev_err(led_cdev->dev,
> + "Cannot set LED brightness from hard irq context!");
> + WARN_ON(1);
> + return;
> + }
> +
> + spin_lock_bh(&leds_soft_blink_lock);
> +
> 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) {
> - set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
> - schedule_work(&led_cdev->set_brightness_work);
> + led_stop_software_blink_nolock(led_cdev);
> } else {
> - set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
> - &led_cdev->work_flags);
> - led_cdev->new_blink_brightness = brightness;
> + led_cdev->blink_brightness = brightness;
> + goto unlock;
> }
> - return;
> }
>
> led_set_brightness_nosleep(led_cdev, brightness);
> +
> +unlock:
> + spin_unlock_bh(&leds_soft_blink_lock);
> }
> EXPORT_SYMBOL_GPL(led_set_brightness);
>
> diff --git a/drivers/leds/trigger/ledtrig-activity.c b/drivers/leds/trigger/ledtrig-activity.c
> index 5081894..6f62da7 100644
> --- a/drivers/leds/trigger/ledtrig-activity.c
> +++ b/drivers/leds/trigger/ledtrig-activity.c
> @@ -48,9 +48,6 @@ static void led_activity_function(struct timer_list *t)
> int cpus;
> int i;
>
> - if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
> - led_cdev->blink_brightness = led_cdev->new_blink_brightness;
> -
> if (unlikely(panic_detected)) {
> /* full brightness in case of panic */
> led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness);
> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> index f0896de..1453352 100644
> --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> @@ -47,9 +47,6 @@ static void led_heartbeat_function(struct timer_list *t)
> return;
> }
>
> - if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
> - led_cdev->blink_brightness = led_cdev->new_blink_brightness;
> -
> /* acts like an actual heart beat -- ie thump-thump-pause... */
> switch (heartbeat_data->phase) {
> case 0:
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 5579c64..e520503 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -51,15 +51,13 @@ struct led_classdev {
> #define LED_BRIGHT_HW_CHANGED BIT(21)
> #define LED_RETAIN_AT_SHUTDOWN BIT(22)
>
> - /* set_brightness_work / blink_timer flags, atomic, private. */
> + /* 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
> @@ -97,7 +95,6 @@ struct led_classdev {
> unsigned long blink_delay_on, blink_delay_off;
> struct timer_list blink_timer;
> int blink_brightness;
> - int new_blink_brightness;
> void (*flash_resume)(struct led_classdev *led_cdev);
>
> struct work_struct set_brightness_work;
>

\
 
 \ /
  Last update: 2018-02-08 21:58    [W:0.348 / U:0.940 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site