Input: HIL - fix rwlock recursion bug

The following bug happens when insmoding hp_sdc_mlc.ko: HP SDC MLC: Registering the System Domain Controller's HIL MLC. BUG: rwlock recursion on CPU#0, hotplug/1814, 00854734 Backtrace: [<10267560>] _raw_write_lock+0x50/0x88 [<10104008>] _write_lock_irqsave+0x14/0x24 [<008537d4>] hp_sdc_mlc_out+0x38/0x25c [hp_sdc_mlc] [<0084ebd8>] hilse_donode+0x308/0x470 [hil_mlc] [<0084ed80>] hil_mlcs_process+0x40/0x6c [hil_mlc] [<10130f80>] tasklet_action+0x78/0xb8 [<10130cec>] __do_softirq+0x60/0xcc [<1010428c>] __lock_text_end+0x38/0x48 [<10108348>] do_cpu_irq_mask+0xf0/0x11c [<1010b068>] intr_return+0x0/0xc Signed-off-by: Helge Deller <deller@gmx.de> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Input: HIL - fix rwlock recursion bug
The following bug happens when insmoding hp_sdc_mlc.ko: HP SDC MLC: Registering the System Domain Controller's HIL MLC. BUG: rwlock recursion on CPU#0, hotplug/1814, 00854734 Backtrace: [<10267560>] _raw_write_lock+0x50/0x88 [<10104008>] _write_lock_irqsave+0x14/0x24 [<008537d4>] hp_sdc_mlc_out+0x38/0x25c [hp_sdc_mlc] [<0084ebd8>] hilse_donode+0x308/0x470 [hil_mlc] [<0084ed80>] hil_mlcs_process+0x40/0x6c [hil_mlc] [<10130f80>] tasklet_action+0x78/0xb8 [<10130cec>] __do_softirq+0x60/0xcc [<1010428c>] __lock_text_end+0x38/0x48 [<10108348>] do_cpu_irq_mask+0xf0/0x11c [<1010b068>] intr_return+0x0/0xc Signed-off-by: Helge Deller <deller@gmx.de> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Helge Deller · Dmitry Torokhov
1 parent 5a90e5bca9
Showing 4 changed files with 19 additions and 25 deletions Side-by-side Diff
drivers/input/serio/hil_mlc.c
drivers/input/serio/hp_sdc.c
drivers/input/serio/hp_sdc_mlc.c
include/linux/hp_sdc.h
@@ -716,7 +716,9 @@
 		break;
  
 	case HILSE_CTS:
+		write_lock_irqsave(&mlc->lock, flags);
 		nextidx = mlc->cts(mlc) ? node->bad : node->good;
+		write_unlock_irqrestore(&mlc->lock, flags);
 		break;
  
 	default:
@@ -100,6 +100,7 @@
 EXPORT_SYMBOL(hp_sdc_release_hil_irq);
 EXPORT_SYMBOL(hp_sdc_release_cooked_irq);
  
+EXPORT_SYMBOL(__hp_sdc_enqueue_transaction);
 EXPORT_SYMBOL(hp_sdc_enqueue_transaction);
 EXPORT_SYMBOL(hp_sdc_dequeue_transaction);
  
  
  
  
@@ -593,18 +594,15 @@
 }
  
 /******* Functions called in either user or kernel context ****/
-int hp_sdc_enqueue_transaction(hp_sdc_transaction *this)
+int __hp_sdc_enqueue_transaction(hp_sdc_transaction *this)
 {
-	unsigned long flags;
 	int i;
  
 	if (this == NULL) {
-		tasklet_schedule(&hp_sdc.task);
+		BUG();
 		return -EINVAL;
 	}
  
-	write_lock_irqsave(&hp_sdc.lock, flags);
-
 	/* Can't have same transaction on queue twice */
 	for (i = 0; i < HP_SDC_QUEUE_LEN; i++)
 		if (hp_sdc.tq[i] == this)
  
  
  
@@ -617,19 +615,27 @@
 	for (i = 0; i < HP_SDC_QUEUE_LEN; i++)
 		if (hp_sdc.tq[i] == NULL) {
 			hp_sdc.tq[i] = this;
-			write_unlock_irqrestore(&hp_sdc.lock, flags);
 			tasklet_schedule(&hp_sdc.task);
 			return 0;
 		}
  
-	write_unlock_irqrestore(&hp_sdc.lock, flags);
 	printk(KERN_WARNING PREFIX "No free slot to add transaction.\n");
 	return -EBUSY;
  
  fail:
-	write_unlock_irqrestore(&hp_sdc.lock,flags);
 	printk(KERN_WARNING PREFIX "Transaction add failed: transaction already queued?\n");
 	return -EINVAL;
+}
+
+int hp_sdc_enqueue_transaction(hp_sdc_transaction *this) {
+	unsigned long flags;
+	int ret;
+
+	write_lock_irqsave(&hp_sdc.lock, flags);
+	ret = __hp_sdc_enqueue_transaction(this);
+	write_unlock_irqrestore(&hp_sdc.lock,flags);
+
+	return ret;
 }
  
 int hp_sdc_dequeue_transaction(hp_sdc_transaction *this)
@@ -142,14 +142,11 @@
  
 static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
 {
-	unsigned long flags;
 	struct hp_sdc_mlc_priv_s *priv;
 	int rc = 2;
  
 	priv = mlc->priv;
  
-	write_lock_irqsave(&mlc->lock, flags);
-
 	/* Try to down the semaphore */
 	if (down_trylock(&mlc->isem)) {
 		struct timeval tv;
  
  
  
@@ -178,21 +175,16 @@
  wasup:
 	up(&mlc->isem);
 	rc = 0;
-	goto done;
  done:
-	write_unlock_irqrestore(&mlc->lock, flags);
 	return rc;
 }
  
 static int hp_sdc_mlc_cts(hil_mlc *mlc)
 {
 	struct hp_sdc_mlc_priv_s *priv;
-	unsigned long flags;
  
 	priv = mlc->priv;
  
-	write_lock_irqsave(&mlc->lock, flags);
-
 	/* Try to down the semaphores -- they should be up. */
 	BUG_ON(down_trylock(&mlc->isem));
 	BUG_ON(down_trylock(&mlc->osem));
  
  
  
  
@@ -221,26 +213,21 @@
 	priv->tseq[2] = 1;
 	priv->tseq[3] = 0;
 	priv->tseq[4] = 0;
-	hp_sdc_enqueue_transaction(&priv->trans);
+	__hp_sdc_enqueue_transaction(&priv->trans);
  busy:
-	write_unlock_irqrestore(&mlc->lock, flags);
 	return 1;
  done:
 	priv->trans.act.semaphore = &mlc->osem;
 	up(&mlc->csem);
-	write_unlock_irqrestore(&mlc->lock, flags);
 	return 0;
 }
  
 static void hp_sdc_mlc_out(hil_mlc *mlc)
 {
 	struct hp_sdc_mlc_priv_s *priv;
-	unsigned long flags;
  
 	priv = mlc->priv;
  
-	write_lock_irqsave(&mlc->lock, flags);
-
 	/* Try to down the semaphore -- it should be up. */
 	BUG_ON(down_trylock(&mlc->osem));
  
@@ -250,7 +237,7 @@
  do_data:
 	if (priv->emtestmode) {
 		up(&mlc->osem);
-		goto done;
+		return;
 	}
 	/* Shouldn't be sending commands when loop may be busy */
 	BUG_ON(down_trylock(&mlc->csem));
@@ -313,8 +300,6 @@
 	}
  enqueue:
 	hp_sdc_enqueue_transaction(&priv->trans);
- done:
-	write_unlock_irqrestore(&mlc->lock, flags);
 }
  
 static int __init hp_sdc_mlc_init(void)
@@ -71,6 +71,7 @@
 	  struct semaphore *semaphore;	/* Semaphore to sleep on. */
 	} act;
 } hp_sdc_transaction;
+int __hp_sdc_enqueue_transaction(hp_sdc_transaction *this);
 int hp_sdc_enqueue_transaction(hp_sdc_transaction *this);
 int hp_sdc_dequeue_transaction(hp_sdc_transaction *this);
...	...	@@ -716,7 +716,9 @@
716	716	break;
717	717
718	718	case HILSE_CTS:
	719	+ write_lock_irqsave(&mlc->lock, flags);
719	720	nextidx = mlc->cts(mlc) ? node->bad : node->good;
	721	+ write_unlock_irqrestore(&mlc->lock, flags);
720	722	break;
721	723
722	724	default:
...	...	@@ -100,6 +100,7 @@
100	100	EXPORT_SYMBOL(hp_sdc_release_hil_irq);
101	101	EXPORT_SYMBOL(hp_sdc_release_cooked_irq);
102	102
	103	+EXPORT_SYMBOL(__hp_sdc_enqueue_transaction);
103	104	EXPORT_SYMBOL(hp_sdc_enqueue_transaction);
104	105	EXPORT_SYMBOL(hp_sdc_dequeue_transaction);
105	106
106	107
107	108
108	109
...	...	@@ -593,18 +594,15 @@
593	594	}
594	595
595	596	/***** Functions called in either user or kernel context **/
596		-int hp_sdc_enqueue_transaction(hp_sdc_transaction *this)
	597	+int __hp_sdc_enqueue_transaction(hp_sdc_transaction *this)
597	598	{
598		- unsigned long flags;
599	599	int i;
600	600
601	601	if (this == NULL) {
602		- tasklet_schedule(&hp_sdc.task);
	602	+ BUG();
603	603	return -EINVAL;
604	604	}
605	605
606		- write_lock_irqsave(&hp_sdc.lock, flags);
607		-
608	606	/* Can't have same transaction on queue twice */
609	607	for (i = 0; i < HP_SDC_QUEUE_LEN; i++)
610	608	if (hp_sdc.tq[i] == this)
611	609
612	610
613	611
...	...	@@ -617,19 +615,27 @@
617	615	for (i = 0; i < HP_SDC_QUEUE_LEN; i++)
618	616	if (hp_sdc.tq[i] == NULL) {
619	617	hp_sdc.tq[i] = this;
620		- write_unlock_irqrestore(&hp_sdc.lock, flags);
621	618	tasklet_schedule(&hp_sdc.task);
622	619	return 0;
623	620	}
624	621
625		- write_unlock_irqrestore(&hp_sdc.lock, flags);
626	622	printk(KERN_WARNING PREFIX "No free slot to add transaction.\n");
627	623	return -EBUSY;
628	624
629	625	fail:
630		- write_unlock_irqrestore(&hp_sdc.lock,flags);
631	626	printk(KERN_WARNING PREFIX "Transaction add failed: transaction already queued?\n");
632	627	return -EINVAL;
	628	+}
	629	+
	630	+int hp_sdc_enqueue_transaction(hp_sdc_transaction *this) {
	631	+ unsigned long flags;
	632	+ int ret;
	633	+
	634	+ write_lock_irqsave(&hp_sdc.lock, flags);
	635	+ ret = __hp_sdc_enqueue_transaction(this);
	636	+ write_unlock_irqrestore(&hp_sdc.lock,flags);
	637	+
	638	+ return ret;
633	639	}
634	640
635	641	int hp_sdc_dequeue_transaction(hp_sdc_transaction *this)
...	...	@@ -142,14 +142,11 @@
142	142
143	143	static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
144	144	{
145		- unsigned long flags;
146	145	struct hp_sdc_mlc_priv_s *priv;
147	146	int rc = 2;
148	147
149	148	priv = mlc->priv;
150	149
151		- write_lock_irqsave(&mlc->lock, flags);
152		-
153	150	/* Try to down the semaphore */
154	151	if (down_trylock(&mlc->isem)) {
155	152	struct timeval tv;
156	153
157	154
158	155
...	...	@@ -178,21 +175,16 @@
178	175	wasup:
179	176	up(&mlc->isem);
180	177	rc = 0;
181		- goto done;
182	178	done:
183		- write_unlock_irqrestore(&mlc->lock, flags);
184	179	return rc;
185	180	}
186	181
187	182	static int hp_sdc_mlc_cts(hil_mlc *mlc)
188	183	{
189	184	struct hp_sdc_mlc_priv_s *priv;
190		- unsigned long flags;
191	185
192	186	priv = mlc->priv;
193	187
194		- write_lock_irqsave(&mlc->lock, flags);
195		-
196	188	/* Try to down the semaphores -- they should be up. */
197	189	BUG_ON(down_trylock(&mlc->isem));
198	190	BUG_ON(down_trylock(&mlc->osem));
199	191
200	192
201	193
202	194
...	...	@@ -221,26 +213,21 @@
221	213	priv->tseq[2] = 1;
222	214	priv->tseq[3] = 0;
223	215	priv->tseq[4] = 0;
224		- hp_sdc_enqueue_transaction(&priv->trans);
	216	+ __hp_sdc_enqueue_transaction(&priv->trans);
225	217	busy:
226		- write_unlock_irqrestore(&mlc->lock, flags);
227	218	return 1;
228	219	done:
229	220	priv->trans.act.semaphore = &mlc->osem;
230	221	up(&mlc->csem);
231		- write_unlock_irqrestore(&mlc->lock, flags);
232	222	return 0;
233	223	}
234	224
235	225	static void hp_sdc_mlc_out(hil_mlc *mlc)
236	226	{
237	227	struct hp_sdc_mlc_priv_s *priv;
238		- unsigned long flags;
239	228
240	229	priv = mlc->priv;
241	230
242		- write_lock_irqsave(&mlc->lock, flags);
243		-
244	231	/* Try to down the semaphore -- it should be up. */
245	232	BUG_ON(down_trylock(&mlc->osem));
246	233
...	...	@@ -250,7 +237,7 @@
250	237	do_data:
251	238	if (priv->emtestmode) {
252	239	up(&mlc->osem);
253		- goto done;
	240	+ return;
254	241	}
255	242	/* Shouldn't be sending commands when loop may be busy */
256	243	BUG_ON(down_trylock(&mlc->csem));
...	...	@@ -313,8 +300,6 @@
313	300	}
314	301	enqueue:
315	302	hp_sdc_enqueue_transaction(&priv->trans);
316		- done:
317		- write_unlock_irqrestore(&mlc->lock, flags);
318	303	}
319	304
320	305	static int __init hp_sdc_mlc_init(void)
...	...	@@ -71,6 +71,7 @@
71	71	struct semaphore semaphore; / Semaphore to sleep on. */
72	72	} act;
73	73	} hp_sdc_transaction;
	74	+int __hp_sdc_enqueue_transaction(hp_sdc_transaction *this);
74	75	int hp_sdc_enqueue_transaction(hp_sdc_transaction *this);
75	76	int hp_sdc_dequeue_transaction(hp_sdc_transaction *this);
76	77