Hi,
On 20-Feb-25 12:23 PM, Remi Pommarel wrote:
> While commit fa15d8c69238 ("leds: Fix set_brightness_delayed() race")
> successfully forces led_set_brightness() to be called with LED_OFF at
> least once when switching from blinking to LED on state so that
> hw-blinking can be disabled, another race remains. Indeed in
> led_set_brightness(LED_OFF) followed by led_set_brightness(any)
> scenario the following CPU scheduling can happen:
>
> CPU0 CPU1
> ---- ----
> set_brightness_delayed() {
> test_and_clear_bit(BRIGHTNESS_OFF)
> led_set_brightness(LED_OFF) {
> set_bit(BRIGHTNESS_OFF)
> queue_work()
> }
> led_set_brightness(any) {
> set_bit(BRIGHTNESS)
> queue_work() //already queued
> }
> test_and_clear_bit(BRIGHTNESS)
> /* LED set with brightness any */
> }
>
> /* From previous CPU1 queue_work() */
> set_brightness_delayed() {
> test_and_clear_bit(BRIGHTNESS_OFF)
> /* LED turned off */
> test_and_clear_bit(BRIGHTNESS)
> /* Clear from previous run, LED remains off */
>
> In that case the led_set_brightness(LED_OFF)/led_set_brightness(any)
> sequence will be effectively executed in reverse order and LED will
> remain off.
>
> With the introduction of commit 32360bf6a5d4 ("leds: Introduce ordered
> workqueue for LEDs events instead of system_wq") the race is easier to
> trigger as sysfs brightness configuration does not wait for
> set_brightness_delayed() work to finish (flush_work() removal).
>
> Use delayed_set_value to optionnally re-configure brightness after a
> LED_OFF. That way a LED state could be configured more that once but
> final state will always be as expected. Ensure that delayed_set_value
> modification is seen before set_bit() using smp_mb__before_atomic().
>
> Fixes: fa15d8c69238 ("leds: Fix set_brightness_delayed() race")
> Signed-off-by: Remi Pommarel <repk@xxxxxxxxxxxx>
> ---
> Changes for v1 to v2:
> - Use smp_mb__before_atomic().
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Regards,
Hans
> ---
> drivers/leds/led-core.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index f6c46d2e5276..e3d8ddcff567 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -159,8 +159,19 @@ static void set_brightness_delayed(struct work_struct *ws)
> * 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))
> + if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags)) {
> set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
> + /*
> + * The consecutives led_set_brightness(LED_OFF),
> + * led_set_brightness(LED_FULL) could have been executed out of
> + * order (LED_FULL first), if the work_flags has been set
> + * between LED_SET_BRIGHTNESS_OFF and LED_SET_BRIGHTNESS of this
> + * work. To avoid ending with the LED turned off, turn the LED
> + * on again.
> + */
> + if (led_cdev->delayed_set_value != LED_OFF)
> + set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
> + }
>
> if (test_and_clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags))
> set_brightness_delayed_set_brightness(led_cdev, led_cdev->delayed_set_value);
> @@ -331,10 +342,13 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
> * change is done immediately afterwards (before the work runs),
> * it uses a separate work_flag.
> */
> - if (value) {
> - led_cdev->delayed_set_value = value;
> + led_cdev->delayed_set_value = value;
> + /* Ensure delayed_set_value is seen before work_flags modification */
> + smp_mb__before_atomic();
> +
> + if (value)
> set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
> - } else {
> + else {
> clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
> clear_bit(LED_SET_BLINK, &led_cdev->work_flags);
> set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
