Re: 2.6.28: warn_slowpath in orinoco receive path

[Andrey Borzenkov <arvidjaar@mail.ru>]

[-- Attachment #1.1: Type: text/plain, Size: 380 bytes --]

On Вторник 06 января 2009 23:02:06 Dave wrote:

> Since we have to use spinlock we should introduce an rx_lock
> specifically to protect the list so the tasklet is not held up by
> anything else the driver is doing. I'll put something together if you
> don't get round to it first.
>

You mean something like attached patch? Not sure if it worth it frankly.

[-- Attachment #1.2: orinoco-rx_list-protect --]
[-- Type: message/rfc822, Size: 5766 bytes --]

From: Andrey Borzenkov <arvidjaar@mail.ru>
Subject: [PATCH] orinoco: protect rx_list manipulation in orinoco_rx_isr_tasklet

The list is changed both in normal and interrupt context. Protect
list manipulation by additional spinlock; this hopefully fixes this warning:

[89479.105038] WARNING: at /home/bor/src/linux-git/lib/list_debug.c:30
__list_add+0x8f/0xa0()
[89479.105058] list_add corruption. prev->next should be next (dddb3568),
but was cbc28978. (prev=dddb3568).
[89479.106002] Pid: 15746, comm: X Not tainted 2.6.28-1avb #26
[89479.106020] Call Trace:
[89479.106062]  [<c011d3b0>] warn_slowpath+0x60/0x80
[89479.106104]  [<c01073d0>] ? native_sched_clock+0x20/0x70
[89479.106194]  [<c013d825>] ? lock_release_holdtime+0x35/0x200
[89479.106218]  [<c018d9f0>] ? __slab_alloc+0x550/0x560
[89479.106254]  [<c02f9c9d>] ? _spin_unlock+0x1d/0x20
[89479.106270]  [<c018d9f0>] ? __slab_alloc+0x550/0x560
[89479.106302]  [<c01ff2a7>] ? delay_tsc+0x17/0x24
[89479.106319]  [<c01ff221>] ? __const_udelay+0x21/0x30
[89479.106376]  [<dfa8b1e2>] ? hermes_bap_seek+0x112/0x1e0 [hermes]
[89479.106396]  [<c013d7eb>] ? trace_hardirqs_off+0xb/0x10
[89479.106418]  [<c018e307>] ? __kmalloc_track_caller+0xb7/0x110
[89479.106448]  [<c028eefc>] ? dev_alloc_skb+0x1c/0x30
[89479.106465]  [<c028eefc>] ? dev_alloc_skb+0x1c/0x30
[89479.106482]  [<c020e13f>] __list_add+0x8f/0xa0
[89479.106551]  [<dfd0fcae>] orinoco_interrupt+0xcae/0x16c0 [orinoco]
[89479.106574]  [<c013b0e3>] ? tick_dev_program_event+0x33/0xb0
[89479.106594]  [<c01073d0>] ? native_sched_clock+0x20/0x70
[89479.106613]  [<c013d825>] ? lock_release_holdtime+0x35/0x200
[89479.106662]  [<c013d7eb>] ? trace_hardirqs_off+0xb/0x10
[89479.106892]  [<dfe7faa7>] ? usb_hcd_irq+0x97/0xa0 [usbcore]
[89479.106926]  [<c015ba79>] handle_IRQ_event+0x29/0x60
[89479.106947]  [<c015cf89>] handle_level_irq+0x69/0xe0
[89479.106963]  [<c015cf20>] ? handle_level_irq+0x0/0xe0
[89479.106977]  <IRQ>  [<c02ca933>] ? tcp_v4_rcv+0x633/0x6e0
[89479.107025]  [<c0103f0c>] ? common_interrupt+0x28/0x30
[89479.107057]  [<c02a0000>] ? sk_run_filter+0x320/0x7a0
[89479.107078]  [<c020e041>] ? list_del+0x21/0x90
[89479.107106]  [<dfd0d24e>] ? orinoco_rx_isr_tasklet+0x2ce/0x480 [orinoco]
[89479.107131]  [<c01402e0>] ? __lock_acquire+0x160/0x1650
[89479.107151]  [<c01073d0>] ? native_sched_clock+0x20/0x70
[89479.107169]  [<c013d825>] ? lock_release_holdtime+0x35/0x200
[89479.107200]  [<c012249a>] ? irq_enter+0xa/0x60
[89479.107217]  [<c0104e52>] ? do_IRQ+0xd2/0x130
[89479.107518]  [<c010342c>] ? restore_nocheck_notrace+0x0/0xe
[89479.107542]  [<c0122830>] ? __do_softirq+0x0/0x110
[89479.107561]  [<c013f7b4>] ? trace_hardirqs_on_caller+0x74/0x140
[89479.107583]  [<c01ff678>] ? trace_hardirqs_on_thunk+0xc/0x10
[89479.107602]  [<c0122087>] ? tasklet_action+0x27/0x90
[89479.107620]  [<c013f7b4>] ? trace_hardirqs_on_caller+0x74/0x140
[89479.107638]  [<c01220a3>] ? tasklet_action+0x43/0x90
[89479.107655]  [<c012289f>] ? __do_softirq+0x6f/0x110
[89479.107674]  [<c0122830>] ? __do_softirq+0x0/0x110
[89479.107685]  <IRQ>  [<c015cf20>] ? handle_level_irq+0x0/0xe0
[89479.107715]  [<c012246d>] ? irq_exit+0x5d/0x80
[89479.107732]  [<c0104e52>] ? do_IRQ+0xd2/0x130
[89479.107747]  [<c0103337>] ? sysenter_exit+0xf/0x16
[89479.107765]  [<c013f83d>] ? trace_hardirqs_on_caller+0xfd/0x140
[89479.107782]  [<c0103f0c>] ? common_interrupt+0x28/0x30
[89479.107797] ---[ end trace a1fc0a52df4a729d ]---

Patch based on suggestion of Dave.

Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>

---

 drivers/net/wireless/orinoco.c |   13 ++++++++++++-
 drivers/net/wireless/orinoco.h |    1 +
 2 files changed, 13 insertions(+), 1 deletions(-)


diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
index 1d7e14b..27e29bb 100644
--- a/drivers/net/wireless/orinoco.c
+++ b/drivers/net/wireless/orinoco.c
@@ -72,6 +72,8 @@
  * they are doing and drop the lock again.  The orinoco_lock()
  * function handles this (it unlocks and returns -EBUSY if
  * hw_unavailable is non-zero).
+ *
+ * Additional lock priv->rx_lock protects received SKB list
  */
 
 #define DRIVER_NAME "orinoco"
@@ -1430,7 +1432,9 @@ static void __orinoco_ev_rx(struct net_device *dev, hermes_t *hw)
 	}
 	rx_data->desc = desc;
 	rx_data->skb = skb;
+	spin_lock(&priv->rx_lock);
 	list_add_tail(&rx_data->list, &priv->rx_list);
+	spin_unlock(&priv->rx_lock);
 	tasklet_schedule(&priv->rx_tasklet);
 
 	return;
@@ -1568,9 +1572,15 @@ static void orinoco_rx_isr_tasklet(unsigned long data)
 	struct orinoco_rx_data *rx_data, *temp;
 	struct hermes_rx_descriptor *desc;
 	struct sk_buff *skb;
+	struct list_head temp_rx_list;
+
+	/* Move list to temporary head to avoid races with interrupt handler */
+	spin_lock_irq(&priv->rx_lock);
+	list_replace_init(&priv->rx_list, &temp_rx_list);
+	spin_unlock_irq(&priv->rx_lock);
 
 	/* extract desc and skb from queue */
-	list_for_each_entry_safe(rx_data, temp, &priv->rx_list, list) {
+	list_for_each_entry_safe(rx_data, temp, &temp_rx_list, list) {
 		desc = rx_data->desc;
 		skb = rx_data->skb;
 		list_del(&rx_data->list);
@@ -3536,6 +3546,7 @@ struct net_device
 	INIT_WORK(&priv->join_work, orinoco_join_ap);
 	INIT_WORK(&priv->wevent_work, orinoco_send_wevents);
 
+	spin_lock_init(&priv->rx_lock);
 	INIT_LIST_HEAD(&priv->rx_list);
 	tasklet_init(&priv->rx_tasklet, orinoco_rx_isr_tasklet,
 		     (unsigned long) dev);
diff --git a/drivers/net/wireless/orinoco.h b/drivers/net/wireless/orinoco.h
index 8c29538..680c66e 100644
--- a/drivers/net/wireless/orinoco.h
+++ b/drivers/net/wireless/orinoco.h
@@ -83,6 +83,7 @@ struct orinoco_private {
 	struct tasklet_struct rx_tasklet;
 	struct list_head rx_list;
 	struct orinoco_rx_data *rx_data;
+	spinlock_t rx_lock;
 
 	/* driver state */
 	int open;

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]