When a trigger wants to switch from blinking to LED on it needs to call:
led_set_brightness(LED_OFF);
led_set_brightness(LED_FULL);
To first call disables blinking and the second then turns the LED on
(the power-supply charging-blink-full-solid triggers do this).
These calls happen immediately after each other, so it is possible
that set_brightness_delayed() from the first call has not run yet
when the led_set_brightness(LED_FULL) call finishes.
If this race hits then this is causing problems for both
sw- and hw-blinking:
For sw-blinking set_brightness_delayed() clears delayed_set_value
when LED_BLINK_DISABLE is set causing the led_set_brightness(LED_FULL)
call effects to get lost when hitting the race, resulting in the LED
turning off instead of on.
For hw-blinking if the race hits delayed_set_value has been
set to LED_FULL by the time set_brightness_delayed() runs.
So led_cdev->brightness_set_blocking() is never called with
LED_OFF as argument and the hw-blinking is never disabled leaving
the LED blinking instead of on.
Fix both issues by adding LED_SET_BRIGHTNESS and LED_SET_BRIGHTNESS_OFF
work_flags making this 2 separate actions to be run by
set_brightness_delayed().
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/leds/led-core.c | 57 +++++++++++++++++++++++++++++++----------
include/linux/leds.h | 3 +++
2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 4a97cb745788..e61acc785410 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -114,21 +114,14 @@ static void led_timer_function(struct timer_list *t)
mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
}
-static void set_brightness_delayed(struct work_struct *ws)
+static void set_brightness_delayed_set_brightness(struct led_classdev *led_cdev,
+ unsigned int value)
{
- struct led_classdev *led_cdev =
- 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);
+ ret = __led_set_brightness(led_cdev, value);
if (ret == -ENOTSUPP)
- ret = __led_set_brightness_blocking(led_cdev,
- led_cdev->delayed_set_value);
+ ret = __led_set_brightness_blocking(led_cdev, value);
if (ret < 0 &&
/* LED HW might have been unplugged, therefore don't warn */
!(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -137,6 +130,30 @@ static void set_brightness_delayed(struct work_struct *ws)
"Setting an LED's brightness failed (%d)\n", ret);
}
+static void set_brightness_delayed(struct work_struct *ws)
+{
+ struct led_classdev *led_cdev =
+ container_of(ws, struct led_classdev, set_brightness_work);
+
+ if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
+ led_stop_software_blink(led_cdev);
+ set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
+ }
+
+ /*
+ * Triggers may call led_set_brightness(LED_OFF),
+ * led_set_brightness(LED_FULL) in quick succession to disable blinking
+ * and turn the LED on. Both actions may have been scheduled to run
+ * before this work item runs once. To make sure this works properly
+ * handle LED_SET_BRIGHTNESS_OFF first.
+ */
+ if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags))
+ set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
+
+ if (test_and_clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags))
+ set_brightness_delayed_set_brightness(led_cdev, led_cdev->delayed_set_value);
+}
+
static void led_set_software_blink(struct led_classdev *led_cdev,
unsigned long delay_on,
unsigned long delay_off)
@@ -271,8 +288,22 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
if (!__led_set_brightness(led_cdev, value))
return;
- /* If brightness setting can sleep, delegate it to a work queue task */
- led_cdev->delayed_set_value = value;
+ /*
+ * Brightness setting can sleep, delegate it to a work queue task.
+ * value 0 / LED_OFF is special, since it also disables hw-blinking
+ * (sw-blink disable is handled in led_set_brightness()).
+ * To avoid a hw-blink-disable getting lost when a second brightness
+ * change is done immediately afterwards (before the work runs),
+ * it uses a separate work_flag.
+ */
+ if (value) {
+ led_cdev->delayed_set_value = value;
+ set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+ } else {
+ clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+ set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
+ }
+
schedule_work(&led_cdev->set_brightness_work);
}
EXPORT_SYMBOL_GPL(led_set_brightness_nopm);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6006786cafdc..eb68aea79c6d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -116,6 +116,9 @@ struct led_classdev {
#define LED_BLINK_INVERT 3
#define LED_BLINK_BRIGHTNESS_CHANGE 4
#define LED_BLINK_DISABLE 5
+ /* Brightness off also disables hw-blinking so it is a separate action */
+#define LED_SET_BRIGHTNESS_OFF 6
+#define LED_SET_BRIGHTNESS 7
/* Set LED brightness level
* Must not sleep. Use brightness_set_blocking for drivers
--
2.39.1
