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@xxxxxxxxxxxxxxxxx>
Signed-off-by: Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>
---
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
