summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorHans de Goede <hdegoede@redhat.com>2023-05-10 18:22:32 +0200
committerLee Jones <lee@kernel.org>2023-05-25 12:16:30 +0100
commitfa15d8c69238b352cc143cb9d8f2ca4594b94022 (patch)
tree0a42c9f755cdb1211c35242850fae5d79e78063a
parente298d8a38b2341865f9feb04591aabb109e8bb13 (diff)
leds: Fix set_brightness_delayed() race
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@redhat.com> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> Tested-by: Yauhen Kharuzhy <jekhor@gmail.com> Link: https://lore.kernel.org/r/20230510162234.291439-3-hdegoede@redhat.com Signed-off-by: Lee Jones <lee@kernel.org>
-rw-r--r--drivers/leds/led-core.c57
-rw-r--r--include/linux/leds.h3
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 c3dc22d184e2..de813fe96a20 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -124,6 +124,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