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 <[email protected]> Signed-off-by: Jacek Anaszewski <[email protected]> --- 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; -- 2.1.4
