[PATCH][input-led] Defer input led work to workqueue

[PATCH][input-led] Defer input led work to workqueue

[Samuel Thibault]

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 <samuel.thibault@ens-lyon.org>

--- a/drivers/input/leds.c
+++ b/drivers/input/leds.c
@@ -100,13 +100,25 @@ 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 workqueue_struct *vt_led_wq;
+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;
+	queue_work(vt_led_wq, &vt_led_work[led]);
 }
 
 /* LED state change for some keyboard, notify that keyboard.  */
@@ -244,6 +256,26 @@ void input_led_disconnect(struct input_d
 	mutex_unlock(&vt_led_registered_lock);
 }
 
+static int __init input_led_init(void)
+{
+	unsigned i;
+
+	vt_led_wq = alloc_workqueue("input_leds", WQ_UNBOUND, 0);
+	if (!vt_led_wq)
+		return -ENOMEM;
+	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)
+{
+	destroy_workqueue(vt_led_wq);
+}
+
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("User LED support for input layer");
 MODULE_AUTHOR("Samuel Thibault <samuel.thibault@ens-lyon.org>");
+module_init(input_led_init);
+module_exit(input_led_exit);

Re: [PATCH][input-led] Defer input led work to workqueue

[Johannes Berg]

On Tue, 2014-08-26 at 03:54 +0200, Samuel Thibault wrote:

> +	vt_led_wq = alloc_workqueue("input_leds", WQ_UNBOUND, 0);
> +	if (!vt_led_wq)
> +		return -ENOMEM;

Does this really need a separate workqueue rather than just using
schedule_work()? There doesn't seem to be much point in having its own
workqueue really, to me.

johannes

Re: [PATCH][input-led] Defer input led work to workqueue

[Samuel Thibault]

Johannes Berg, le Tue 26 Aug 2014 10:01:35 +0200, a écrit :
> On Tue, 2014-08-26 at 03:54 +0200, Samuel Thibault wrote:
> 
> > +	vt_led_wq = alloc_workqueue("input_leds", WQ_UNBOUND, 0);
> > +	if (!vt_led_wq)
> > +		return -ENOMEM;
> 
> Does this really need a separate workqueue rather than just using
> schedule_work()? There doesn't seem to be much point in having its own
> workqueue really, to me.

Ah, yes, probably.  I'm not up to date with this kind of thing (last
time I used those, they were called tasklets and bottom halves, you
know... ;) ), so I just followed Documentation/workqueue.txt, which does
not talk about schedule_work.

Samuel

Re: [PATCH][input-led] Defer input led work to workqueue

[Valdis.Kletnieks-PjAqaU27lzQ]

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

On Tue, 26 Aug 2014 10:01:35 +0200, Johannes Berg said:
> On Tue, 2014-08-26 at 03:54 +0200, Samuel Thibault wrote:
>
> > +	vt_led_wq = alloc_workqueue("input_leds", WQ_UNBOUND, 0);
> > +	if (!vt_led_wq)
> > +		return -ENOMEM;
>
> Does this really need a separate workqueue rather than just using
> schedule_work()? There doesn't seem to be much point in having its own
> workqueue really, to me.

As I noted in another email, the patch works. Whether it's *correct* I can't
tell. :)

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

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

[Samuel Thibault]

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 <samuel.thibault@ens-lyon.org>

---

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

--- a/drivers/input/leds.c
+++ b/drivers/input/leds.c
@@ -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 <samuel.thibault@ens-lyon.org>");
+module_init(input_led_init);
+module_exit(input_led_exit);

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

[Johannes Berg]

On Tue, 2014-08-26 at 11:17 +0200, Samuel Thibault wrote:

> -	led_trigger_event(&vt_led_triggers[led], !!brightness);
> +	vt_led_state[led] = !!brightness;
> +	schedule_work(&vt_led_work[led]);


> +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)
> +{
> +}

Come to think of it - don't you need to cancel_work_sync() in exit so
the work struct can't be queued while the module is being unloaded?

johannes

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

[Samuel Thibault]

Johannes Berg, le Tue 26 Aug 2014 15:22:07 +0200, a écrit :
> On Tue, 2014-08-26 at 11:17 +0200, Samuel Thibault wrote:
> 
> > -	led_trigger_event(&vt_led_triggers[led], !!brightness);
> > +	vt_led_state[led] = !!brightness;
> > +	schedule_work(&vt_led_work[led]);
> 
> 
> > +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)
> > +{
> > +}
> 
> Come to think of it - don't you need to cancel_work_sync() in exit so
> the work struct can't be queued while the module is being unloaded?

Well, this is never built as a module ATM, so it's not a problem.

Samuel

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

[Samuel Thibault]

Samuel Thibault, le Tue 26 Aug 2014 15:24:10 +0200, a écrit :
> Johannes Berg, le Tue 26 Aug 2014 15:22:07 +0200, a écrit :
> > On Tue, 2014-08-26 at 11:17 +0200, Samuel Thibault wrote:
> > 
> > > -	led_trigger_event(&vt_led_triggers[led], !!brightness);
> > > +	vt_led_state[led] = !!brightness;
> > > +	schedule_work(&vt_led_work[led]);
> > 
> > 
> > > +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)
> > > +{
> > > +}
> > 
> > Come to think of it - don't you need to cancel_work_sync() in exit so
> > the work struct can't be queued while the module is being unloaded?
> 
> Well, this is never built as a module ATM, so it's not a problem.

(but I have changed that for future versions of the whole patch, yes)

Samuel
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Valdis.Kletnieks-PjAqaU27lzQ]

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

On Tue, 26 Aug 2014 11:17:25 +0200, you said:

>
> Difference with v1: uses schedule_work instead of its own workqueue.
>
> --- a/drivers/input/leds.c
> +++ b/drivers/input/leds.c
> @@ -100,13 +100,24 @@ static unsigned long vt_led_registered[B

Same thing for B2 - next-20140826 complains, and no complaints with this patch
on top.

Tested-by: Valdis Kletnieks <valdis.kletnieks-PjAqaU27lzQ@public.gmane.org>

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

Re: [PATCH][input-led] Defer input led work to workqueue

[Valdis.Kletnieks]

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

On Tue, 26 Aug 2014 03:54:53 +0200, Samuel Thibault said:
> 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 <samuel.thibault@ens-lyon.org>
>
> --- a/drivers/input/leds.c
> +++ b/drivers/input/leds.c
> @@ -100,13 +100,25 @@ static unsigned long vt_led_registered[B

I admit having zero understanding of the code, but I can confirm that
next-20140825 still throws the lockdep whine I was seeing, but the same
tree with this patch on top of it boots without warning. Soo...

Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

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

Re: [PATCH][input-led] Defer input led work to workqueue

[Sabrina Dubroca]

2014-08-26, 14:49:18 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Tue, 26 Aug 2014 03:54:53 +0200, Samuel Thibault said:
> > 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 <samuel.thibault@ens-lyon.org>
> >
> > --- a/drivers/input/leds.c
> > +++ b/drivers/input/leds.c
> > @@ -100,13 +100,25 @@ static unsigned long vt_led_registered[B
> 
> I admit having zero understanding of the code, but I can confirm that
> next-20140825 still throws the lockdep whine I was seeing, but the same
> tree with this patch on top of it boots without warning. Soo...
> 
> Tested-By: Valdis Kletnieks <valdis.kletnieks@vt.edu>

Same for me.

Tested-by: Sabrina Dubroca <sd@queasysnail.net>


Thanks,

-- 
Sabrina

Re: [PATCH][input-led] Defer input led work to workqueue

[Sabrina Dubroca]

2014-08-26, 22:02:08 +0200, Sabrina Dubroca wrote:
> [...]
> 
> Tested-by: Sabrina Dubroca <sd@queasysnail.net>

Forgot to mention, this applies to both versions of the patch.


-- 
Sabrina