Input: fix locking in memoryless force-feedback devices

Now that input core acquires dev->event_lock spinlock and disables interrupts when propagating input events, using spin_lock_bh() in ff-memless driver is not allowed. Actually, the timer_lock itself is not needed anymore, we should simply use dev->event_lock as well. Also do a small cleanup in force-feedback core. Reported-by: kerneloops.org Reported-by: http://www.kerneloops.org/searchweek.php?search=ml_ff_set_gain Reported-by: Arjan van de Ven <arjan@linux.intel.com> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Input: fix locking in memoryless force-feedback devices
Now that input core acquires dev->event_lock spinlock and disables interrupts when propagating input events, using spin_lock_bh() in ff-memless driver is not allowed. Actually, the timer_lock itself is not needed anymore, we should simply use dev->event_lock as well. Also do a small cleanup in force-feedback core. Reported-by: kerneloops.org Reported-by: http://www.kerneloops.org/searchweek.php?search=ml_ff_set_gain Reported-by: Arjan van de Ven <arjan@linux.intel.com> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Dmitry Torokhov
1 parent 558a5e296a
Showing 3 changed files with 26 additions and 24 deletions Side-by-side Diff
drivers/input/ff-core.c
drivers/input/ff-memless.c
include/linux/input.h
@@ -337,16 +337,16 @@
 	dev->ff = ff;
 	dev->flush = flush_effects;
 	dev->event = input_ff_event;
-	set_bit(EV_FF, dev->evbit);
+	__set_bit(EV_FF, dev->evbit);
  
 	/* Copy "true" bits into ff device bitmap */
 	for (i = 0; i <= FF_MAX; i++)
 		if (test_bit(i, dev->ffbit))
-			set_bit(i, ff->ffbit);
+			__set_bit(i, ff->ffbit);
  
 	/* we can emulate RUMBLE with periodic effects */
 	if (test_bit(FF_PERIODIC, ff->ffbit))
-		set_bit(FF_RUMBLE, dev->ffbit);
+		__set_bit(FF_RUMBLE, dev->ffbit);
  
 	return 0;
 }
@@ -362,12 +362,14 @@
  */
 void input_ff_destroy(struct input_dev *dev)
 {
-	clear_bit(EV_FF, dev->evbit);
-	if (dev->ff) {
-		if (dev->ff->destroy)
-			dev->ff->destroy(dev->ff);
-		kfree(dev->ff->private);
-		kfree(dev->ff);
+	struct ff_device *ff = dev->ff;
+
+	__clear_bit(EV_FF, dev->evbit);
+	if (ff) {
+		if (ff->destroy)
+			ff->destroy(ff);
+		kfree(ff->private);
+		kfree(ff);
 		dev->ff = NULL;
 	}
 }
@@ -61,7 +61,6 @@
 	struct ml_effect_state states[FF_MEMLESS_EFFECTS];
 	int gain;
 	struct timer_list timer;
-	spinlock_t timer_lock;
 	struct input_dev *dev;
  
 	int (*play_effect)(struct input_dev *dev, void *data,
  
  
  
  
  
  
  
  
@@ -368,39 +367,39 @@
 {
 	struct input_dev *dev = (struct input_dev *)timer_data;
 	struct ml_device *ml = dev->ff->private;
+	unsigned long flags;
  
 	debug("timer: updating effects");
  
-	spin_lock(&ml->timer_lock);
+	spin_lock_irqsave(&dev->event_lock, flags);
 	ml_play_effects(ml);
-	spin_unlock(&ml->timer_lock);
+	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
  
+/*
+ * Sets requested gain for FF effects. Called with dev->event_lock held.
+ */
 static void ml_ff_set_gain(struct input_dev *dev, u16 gain)
 {
 	struct ml_device *ml = dev->ff->private;
 	int i;
  
-	spin_lock_bh(&ml->timer_lock);
-
 	ml->gain = gain;
  
 	for (i = 0; i < FF_MEMLESS_EFFECTS; i++)
 		__clear_bit(FF_EFFECT_PLAYING, &ml->states[i].flags);
  
 	ml_play_effects(ml);
-
-	spin_unlock_bh(&ml->timer_lock);
 }
  
+/*
+ * Start/stop specified FF effect. Called with dev->event_lock held.
+ */
 static int ml_ff_playback(struct input_dev *dev, int effect_id, int value)
 {
 	struct ml_device *ml = dev->ff->private;
 	struct ml_effect_state *state = &ml->states[effect_id];
-	unsigned long flags;
  
-	spin_lock_irqsave(&ml->timer_lock, flags);
-
 	if (value > 0) {
 		debug("initiated play");
  
@@ -425,8 +424,6 @@
 		ml_play_effects(ml);
 	}
  
-	spin_unlock_irqrestore(&ml->timer_lock, flags);
-
 	return 0;
 }
  
@@ -436,7 +433,7 @@
 	struct ml_device *ml = dev->ff->private;
 	struct ml_effect_state *state = &ml->states[effect->id];
  
-	spin_lock_bh(&ml->timer_lock);
+	spin_lock_irq(&dev->event_lock);
  
 	if (test_bit(FF_EFFECT_STARTED, &state->flags)) {
 		__clear_bit(FF_EFFECT_PLAYING, &state->flags);
@@ -448,7 +445,7 @@
 		ml_schedule_timer(ml);
 	}
  
-	spin_unlock_bh(&ml->timer_lock);
+	spin_unlock_irq(&dev->event_lock);
  
 	return 0;
 }
@@ -482,7 +479,6 @@
 	ml->private = data;
 	ml->play_effect = play_effect;
 	ml->gain = 0xffff;
-	spin_lock_init(&ml->timer_lock);
 	setup_timer(&ml->timer, ml_effect_timer, (unsigned long)dev);
  
 	set_bit(FF_GAIN, dev->ffbit);
@@ -1377,6 +1377,10 @@
  * methods; erase() is optional. set_gain() and set_autocenter() need
  * only be implemented if driver sets up FF_GAIN and FF_AUTOCENTER
  * bits.
+ *
+ * Note that playback(), set_gain() and set_autocenter() are called with
+ * dev->event_lock spinlock held and interrupts off and thus may not
+ * sleep.
  */
 struct ff_device {
 	int (*upload)(struct input_dev *dev, struct ff_effect *effect,
...	...	@@ -337,16 +337,16 @@
337	337	dev->ff = ff;
338	338	dev->flush = flush_effects;
339	339	dev->event = input_ff_event;
340		- set_bit(EV_FF, dev->evbit);
	340	+ __set_bit(EV_FF, dev->evbit);
341	341
342	342	/* Copy "true" bits into ff device bitmap */
343	343	for (i = 0; i <= FF_MAX; i++)
344	344	if (test_bit(i, dev->ffbit))
345		- set_bit(i, ff->ffbit);
	345	+ __set_bit(i, ff->ffbit);
346	346
347	347	/* we can emulate RUMBLE with periodic effects */
348	348	if (test_bit(FF_PERIODIC, ff->ffbit))
349		- set_bit(FF_RUMBLE, dev->ffbit);
	349	+ __set_bit(FF_RUMBLE, dev->ffbit);
350	350
351	351	return 0;
352	352	}
...	...	@@ -362,12 +362,14 @@
362	362	*/
363	363	void input_ff_destroy(struct input_dev *dev)
364	364	{
365		- clear_bit(EV_FF, dev->evbit);
366		- if (dev->ff) {
367		- if (dev->ff->destroy)
368		- dev->ff->destroy(dev->ff);
369		- kfree(dev->ff->private);
370		- kfree(dev->ff);
	365	+ struct ff_device *ff = dev->ff;
	366	+
	367	+ __clear_bit(EV_FF, dev->evbit);
	368	+ if (ff) {
	369	+ if (ff->destroy)
	370	+ ff->destroy(ff);
	371	+ kfree(ff->private);
	372	+ kfree(ff);
371	373	dev->ff = NULL;
372	374	}
373	375	}
...	...	@@ -61,7 +61,6 @@
61	61	struct ml_effect_state states[FF_MEMLESS_EFFECTS];
62	62	int gain;
63	63	struct timer_list timer;
64		- spinlock_t timer_lock;
65	64	struct input_dev *dev;
66	65
67	66	int (play_effect)(struct input_dev dev, void *data,
68	67
69	68
70	69
71	70
72	71
73	72
74	73
75	74
...	...	@@ -368,39 +367,39 @@
368	367	{
369	368	struct input_dev dev = (struct input_dev )timer_data;
370	369	struct ml_device *ml = dev->ff->private;
	370	+ unsigned long flags;
371	371
372	372	debug("timer: updating effects");
373	373
374		- spin_lock(&ml->timer_lock);
	374	+ spin_lock_irqsave(&dev->event_lock, flags);
375	375	ml_play_effects(ml);
376		- spin_unlock(&ml->timer_lock);
	376	+ spin_unlock_irqrestore(&dev->event_lock, flags);
377	377	}
378	378
	379	+/*
	380	+ * Sets requested gain for FF effects. Called with dev->event_lock held.
	381	+ */
379	382	static void ml_ff_set_gain(struct input_dev *dev, u16 gain)
380	383	{
381	384	struct ml_device *ml = dev->ff->private;
382	385	int i;
383	386
384		- spin_lock_bh(&ml->timer_lock);
385		-
386	387	ml->gain = gain;
387	388
388	389	for (i = 0; i < FF_MEMLESS_EFFECTS; i++)
389	390	__clear_bit(FF_EFFECT_PLAYING, &ml->states[i].flags);
390	391
391	392	ml_play_effects(ml);
392		-
393		- spin_unlock_bh(&ml->timer_lock);
394	393	}
395	394
	395	+/*
	396	+ * Start/stop specified FF effect. Called with dev->event_lock held.
	397	+ */
396	398	static int ml_ff_playback(struct input_dev *dev, int effect_id, int value)
397	399	{
398	400	struct ml_device *ml = dev->ff->private;
399	401	struct ml_effect_state *state = &ml->states[effect_id];
400		- unsigned long flags;
401	402
402		- spin_lock_irqsave(&ml->timer_lock, flags);
403		-
404	403	if (value > 0) {
405	404	debug("initiated play");
406	405
...	...	@@ -425,8 +424,6 @@
425	424	ml_play_effects(ml);
426	425	}
427	426
428		- spin_unlock_irqrestore(&ml->timer_lock, flags);
429		-
430	427	return 0;
431	428	}
432	429
...	...	@@ -436,7 +433,7 @@
436	433	struct ml_device *ml = dev->ff->private;
437	434	struct ml_effect_state *state = &ml->states[effect->id];
438	435
439		- spin_lock_bh(&ml->timer_lock);
	436	+ spin_lock_irq(&dev->event_lock);
440	437
441	438	if (test_bit(FF_EFFECT_STARTED, &state->flags)) {
442	439	__clear_bit(FF_EFFECT_PLAYING, &state->flags);
...	...	@@ -448,7 +445,7 @@
448	445	ml_schedule_timer(ml);
449	446	}
450	447
451		- spin_unlock_bh(&ml->timer_lock);
	448	+ spin_unlock_irq(&dev->event_lock);
452	449
453	450	return 0;
454	451	}
...	...	@@ -482,7 +479,6 @@
482	479	ml->private = data;
483	480	ml->play_effect = play_effect;
484	481	ml->gain = 0xffff;
485		- spin_lock_init(&ml->timer_lock);
486	482	setup_timer(&ml->timer, ml_effect_timer, (unsigned long)dev);
487	483
488	484	set_bit(FF_GAIN, dev->ffbit);
...	...	@@ -1377,6 +1377,10 @@
1377	1377	* methods; erase() is optional. set_gain() and set_autocenter() need
1378	1378	* only be implemented if driver sets up FF_GAIN and FF_AUTOCENTER
1379	1379	* bits.
	1380	+ *
	1381	+ * Note that playback(), set_gain() and set_autocenter() are called with
	1382	+ * dev->event_lock spinlock held and interrupts off and thus may not
	1383	+ * sleep.
1380	1384	*/
1381	1385	struct ff_device {
1382	1386	int (upload)(struct input_dev dev, struct ff_effect *effect,