leds: avoid flush_work in atomic context
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/0x112drivers/leds/led-core.c +0 -5
@@ -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))
drivers/leds/trigger/ledtrig-timer.c +5 -0
@@ -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);
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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
Quoted 59 lines · Pavel Machek
It turns out that various triggers use
led_blink_setup()from atomic context, so we can't do aflush_workthere. 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/0x112Fixes 0db37915d912Signed-off-by Pavel Machek <[email protected]>Tested-by Hugh Dickins <[email protected]>drivers/leds/led-core.c +0
-5@@ -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))drivers/leds/trigger/ledtrig-timer.c +5
-0@@ -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);
Hi!
Quoted 5 lines
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 :-).
Quoted 7 lines
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
Quoted 13 lines · Pavel Machek
Hi!
Quoted 5 lines
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 :-).
Quoted 3 lines
Beside that 'Fixes' tag is somewhat incomplete - it should be generated using following git command:
Quoted 7 lines
Quoted 4 lines
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!
Quoted 2 lines
So... it makes sense to push it to Linus "soon".
Agreed.
Hugh