Skip to contentSkip to conversation

3.17-rc1: leds blink workqueue causes sleeping BUGs

DISCUSSION Samuel Thibault 2 participants 2 messages Sep 22, 2014 1 message not in the archive

Subsystems (2)
Input drivers LED
lore
Share & tools
lore

Samuel Thibault

author
#1 lore raw
In reply to a message not in the archive
Quoted 3 lines · Kari Suvanto, le Mon 22 Sep 2014 10:30:32 +0300, a écrit :
Quoted 1 line

Ew. I'll have a look.

Any update on this one?

I have already sent it, yes, on Tue, 26 Aug 2014 11:17:25 +0200, here it is again.

Samuel

Subject: [PATCHv2][input-led] Defer input led work to workqueue

When the kbd changes its led state (e.g. caps lock), this triggers (led_trigger_event) the kbd-capsl trigger, which is by default used by the vt::capsl LED, which triggers (led_trigger_event) the vt-capsl trigger. These two nested led_trigger_event calls take a trig->leddev_list_lock lock and thus lockdep complains.

Actually the user can make the vt::capsl LED use its own vt-capsl trigger and thus build a loop. This produces an immediate oops.

This changeset defers the second led_trigger_event call into a workqueue, which avoids the nested locking altogether. This does not prevent the user from shooting himself in the foot by creating a vt::capsl <-> vt-capsl loop, but the only consequence is the workqueue threads eating some CPU until the user breaks the loop, which is not too bad.

Signed-off-by Samuel Thibault <[email protected]>

---

Difference with v1: uses schedule_work instead of its own workqueue.

drivers/input/leds.c +28 -1
@@ -100,13 +100,24 @@ static unsigned long vt_led_registered[B
 /* Number of input devices having each LED */
 static int vt_led_references[LED_CNT];
 
+static int vt_led_state[LED_CNT];
+static struct work_struct vt_led_work[LED_CNT];
+
+static void vt_led_cb(struct work_struct *work)
+{
+	int led = work - vt_led_work;
+
+	led_trigger_event(&vt_led_triggers[led], vt_led_state[led]);
+}
+
 /* VT LED state change, tell the VT trigger.  */
 static void vt_led_set(struct led_classdev *cdev,
 			  enum led_brightness brightness)
 {
 	int led = cdev - vt_leds;
 
-	led_trigger_event(&vt_led_triggers[led], !!brightness);
+	vt_led_state[led] = !!brightness;
+	schedule_work(&vt_led_work[led]);
 }
 
 /* LED state change for some keyboard, notify that keyboard.  */

@@ -244,6 +255,22 @@ void input_led_disconnect(struct input_d
 	mutex_unlock(&vt_led_registered_lock);
 }
 
+static int __init input_led_init(void)
+{
+	unsigned i;
+
+	for (i = 0; i < LED_CNT; i++)
+		INIT_WORK(&vt_led_work[i], vt_led_cb);
+
+	return 0;
+}
+
+static void __exit input_led_exit(void)
+{
+}
+
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("User LED support for input layer");
 MODULE_AUTHOR("Samuel Thibault <[email protected]>");
+module_init(input_led_init);
+module_exit(input_led_exit);
#2 lore raw

Ok I missed that and it did not show up with my searches. I also tested your patch on top of next-20140922 and it seems to work.

-Kari