Commit fa15d8c6 authored by Hans de Goede's avatar Hans de Goede Committed by Lee Jones
Browse files

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: default avatarHans de Goede <hdegoede@redhat.com>
Reviewed-by: default avatarJacek Anaszewski <jacek.anaszewski@gmail.com>
Tested-by: default avatarYauhen Kharuzhy <jekhor@gmail.com>
Link: https://lore.kernel.org/r/20230510162234.291439-3-hdegoede@redhat.com


Signed-off-by: default avatarLee Jones <lee@kernel.org>
parent e298d8a3

drivers/leds/led-core.c

+44 −13
Original line number Diff line number Diff line
@@ -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 */

	/*

	 * 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);

include/linux/leds.h

+3 −0
Original line number Diff line number Diff line
@@ -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