leds: avoid flush_work in atomic context

leds: avoid flush_work in atomic context

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]


It turns out that various triggers use led_blink_setup() from atomic
context, so we can't do a flush_work there. Flush is still needed for
slow LEDs, but we can move it to sysfs code where it is safe.
    
    WARNING: inconsistent lock state
    5.2.0-rc1 #1 Tainted: G        W
    --------------------------------
    inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
    swapper/1/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
    000000006e30541b
    ((work_completion)(&led_cdev->set_brightness_work)){+.?.}, at:
    +__flush_work+0x3b/0x38a
    {SOFTIRQ-ON-W} state was registered at:
      lock_acquire+0x146/0x1a1
     __flush_work+0x5b/0x38a
     flush_work+0xb/0xd
     led_blink_setup+0x1e/0xd3
     led_blink_set+0x3f/0x44
     tpt_trig_timer+0xdb/0x106
     ieee80211_mod_tpt_led_trig+0xed/0x112
    
Fixes: 0db37915d912
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Tested-by: Hugh Dickins <hughd@google.com>
    
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index aefac4d..9f8da39 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -166,11 +166,6 @@ static void led_blink_setup(struct led_classdev *led_cdev,
 		     unsigned long *delay_on,
 		     unsigned long *delay_off)
 {
-	/*
-	 * If "set brightness to 0" is pending in workqueue, we don't
-	 * want that to be reordered after blink_set()
-	 */
-	flush_work(&led_cdev->set_brightness_work);
 	if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
 	    led_cdev->blink_set &&
 	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
index ca898c1..427fc3c 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -113,6 +113,11 @@ static int timer_trig_activate(struct led_classdev *led_cdev)
 		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
 	}
 
+	/*
+	 * If "set brightness to 0" is pending in workqueue, we don't
+	 * want that to be reordered after blink_set()
+	 */
+	flush_work(&led_cdev->set_brightness_work);
 	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
 		      &led_cdev->blink_delay_off);
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: leds: avoid flush_work in atomic context

[Jacek Anaszewski]

Hi Pavel,

Thank you for the patch.

I've applied it however I'm not sure if it is an official
submission, since it doesn't look like (no [PATCH] tag
in the subject).

Beside that 'Fixes' tag is somewhat incomplete - it should be
generated using following git command:

git log -1 0db37915d912 --format='Fixes: %h ("%s")'

Fixed that and applied to the for-next branch and will push
it upstream after a bit of testing for rc3 or rc4.

Best regards,
Jacek Anaszewski

On 5/26/19 9:38 AM, Pavel Machek wrote:
> 
> It turns out that various triggers use led_blink_setup() from atomic
> context, so we can't do a flush_work there. Flush is still needed for
> slow LEDs, but we can move it to sysfs code where it is safe.
>      
>      WARNING: inconsistent lock state
>      5.2.0-rc1 #1 Tainted: G        W
>      --------------------------------
>      inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
>      swapper/1/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
>      000000006e30541b
>      ((work_completion)(&led_cdev->set_brightness_work)){+.?.}, at:
>      +__flush_work+0x3b/0x38a
>      {SOFTIRQ-ON-W} state was registered at:
>        lock_acquire+0x146/0x1a1
>       __flush_work+0x5b/0x38a
>       flush_work+0xb/0xd
>       led_blink_setup+0x1e/0xd3
>       led_blink_set+0x3f/0x44
>       tpt_trig_timer+0xdb/0x106
>       ieee80211_mod_tpt_led_trig+0xed/0x112
>      
> Fixes: 0db37915d912
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Tested-by: Hugh Dickins <hughd@google.com>
>      
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index aefac4d..9f8da39 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -166,11 +166,6 @@ static void led_blink_setup(struct led_classdev *led_cdev,
>   		     unsigned long *delay_on,
>   		     unsigned long *delay_off)
>   {
> -	/*
> -	 * If "set brightness to 0" is pending in workqueue, we don't
> -	 * want that to be reordered after blink_set()
> -	 */
> -	flush_work(&led_cdev->set_brightness_work);
>   	if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
>   	    led_cdev->blink_set &&
>   	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> diff --git a/drivers/leds/trigger/ledtrig-timer.c b/drivers/leds/trigger/ledtrig-timer.c
> index ca898c1..427fc3c 100644
> --- a/drivers/leds/trigger/ledtrig-timer.c
> +++ b/drivers/leds/trigger/ledtrig-timer.c
> @@ -113,6 +113,11 @@ static int timer_trig_activate(struct led_classdev *led_cdev)
>   		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
>   	}
>   
> +	/*
> +	 * If "set brightness to 0" is pending in workqueue, we don't
> +	 * want that to be reordered after blink_set()
> +	 */
> +	flush_work(&led_cdev->set_brightness_work);
>   	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
>   		      &led_cdev->blink_delay_off);
>   
> 

Re: leds: avoid flush_work in atomic context

[Pavel Machek]

Hi!

> Thank you for the patch.
> 
> I've applied it however I'm not sure if it is an official
> submission, since it doesn't look like (no [PATCH] tag
> in the subject).

It was official submission :-).

> Beside that 'Fixes' tag is somewhat incomplete - it should be
> generated using following git command:
> 
> git log -1 0db37915d912 --format='Fixes: %h ("%s")'
> 
> Fixed that and applied to the for-next branch and will push
> it upstream after a bit of testing for rc3 or rc4.

Ok. Note that this did not crash Hugh's machine but it may crash
someone else's, and probably crashed mine already.

So... it makes sense to push it to Linus "soon".

Thanks,
								Pavel

Re: leds: avoid flush_work in atomic context

[Hugh Dickins]

On Sun, 26 May 2019, Pavel Machek wrote:
> Hi!
> 
> > Thank you for the patch.
> > 
> > I've applied it however I'm not sure if it is an official
> > submission, since it doesn't look like (no [PATCH] tag
> > in the subject).
> 
> It was official submission :-).
> 
> > Beside that 'Fixes' tag is somewhat incomplete - it should be
> > generated using following git command:
> > 
> > git log -1 0db37915d912 --format='Fixes: %h ("%s")'
> > 
> > Fixed that and applied to the for-next branch and will push
> > it upstream after a bit of testing for rc3 or rc4.
> 
> Ok. Note that this did not crash Hugh's machine but it may crash
> someone else's, and probably crashed mine already.

Where "this" is commit 0db37915d912, I hope: right, it did not
actually crash my machine, but the splurge of warning messages
forced me to reboot quickly, and go look for the culprit.

Your fix certainly did not crash my machine either,
but I hope we don't expect your fix to crash someone else's!

> 
> So... it makes sense to push it to Linus "soon".

Agreed.

Hugh