Return-path: Received: from mail-ew0-f17.google.com ([209.85.219.17]:50060 "EHLO mail-ew0-f17.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752210AbZAGAXx (ORCPT ); Tue, 6 Jan 2009 19:23:53 -0500 Received: by ewy10 with SMTP id 10so8751997ewy.13 for ; Tue, 06 Jan 2009 16:23:51 -0800 (PST) From: David Kilroy To: linux-wireless@vger.kernel.org, linville@tuxdriver.com Cc: arvidjaar@mail.ru, David Kilroy Subject: [PATCH] orinoco: take the driver lock in the rx tasklet Date: Wed, 7 Jan 2009 00:23:55 +0000 Message-Id: <1231287835-17707-1-git-send-email-kilroyd@googlemail.com> (sfid-20090107_012359_247346_1421ABFF) Sender: linux-wireless-owner@vger.kernel.org List-ID: Fix the warning reproduced below. We add to rx_list in interrupt context and remove elements in tasklet context. While removing elements we need to prevent the interrupt modifying the list. Note that commit 31afcef385bb8bf528c6fbe05b359af9f456f02a did not preserve locking semantics on what is now orinoco_rx. This patch reinstates the locking semantics and ensures it covers rx_list as well. This leads to additional cleanup required in free_orinocodev. [89479.105038] WARNING: at 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] [] warn_slowpath+0x60/0x80 [89479.106104] [] ? native_sched_clock+0x20/0x70 [89479.106194] [] ? lock_release_holdtime+0x35/0x200 [89479.106218] [] ? __slab_alloc+0x550/0x560 [89479.106254] [] ? _spin_unlock+0x1d/0x20 [89479.106270] [] ? __slab_alloc+0x550/0x560 [89479.106302] [] ? delay_tsc+0x17/0x24 [89479.106319] [] ? __const_udelay+0x21/0x30 [89479.106376] [] ? hermes_bap_seek+0x112/0x1e0 [hermes] [89479.106396] [] ? trace_hardirqs_off+0xb/0x10 [89479.106418] [] ? __kmalloc_track_caller+0xb7/0x110 [89479.106448] [] ? dev_alloc_skb+0x1c/0x30 [89479.106465] [] ? dev_alloc_skb+0x1c/0x30 [89479.106482] [] __list_add+0x8f/0xa0 [89479.106551] [] orinoco_interrupt+0xcae/0x16c0 [orinoco] [89479.106574] [] ? tick_dev_program_event+0x33/0xb0 [89479.106594] [] ? native_sched_clock+0x20/0x70 [89479.106613] [] ? lock_release_holdtime+0x35/0x200 [89479.106662] [] ? trace_hardirqs_off+0xb/0x10 [89479.106892] [] ? usb_hcd_irq+0x97/0xa0 [usbcore] [89479.106926] [] handle_IRQ_event+0x29/0x60 [89479.106947] [] handle_level_irq+0x69/0xe0 [89479.106963] [] ? handle_level_irq+0x0/0xe0 [89479.106977] [] ? tcp_v4_rcv+0x633/0x6e0 [89479.107025] [] ? common_interrupt+0x28/0x30 [89479.107057] [] ? sk_run_filter+0x320/0x7a0 [89479.107078] [] ? list_del+0x21/0x90 [89479.107106] [] ? orinoco_rx_isr_tasklet+0x2ce/0x480 [orinoco] [89479.107131] [] ? __lock_acquire+0x160/0x1650 [89479.107151] [] ? native_sched_clock+0x20/0x70 [89479.107169] [] ? lock_release_holdtime+0x35/0x200 [89479.107200] [] ? irq_enter+0xa/0x60 [89479.107217] [] ? do_IRQ+0xd2/0x130 [89479.107518] [] ? restore_nocheck_notrace+0x0/0xe [89479.107542] [] ? __do_softirq+0x0/0x110 [89479.107561] [] ? trace_hardirqs_on_caller+0x74/0x140 [89479.107583] [] ? trace_hardirqs_on_thunk+0xc/0x10 [89479.107602] [] ? tasklet_action+0x27/0x90 [89479.107620] [] ? trace_hardirqs_on_caller+0x74/0x140 [89479.107638] [] ? tasklet_action+0x43/0x90 [89479.107655] [] ? __do_softirq+0x6f/0x110 [89479.107674] [] ? __do_softirq+0x0/0x110 [89479.107685] [] ? handle_level_irq+0x0/0xe0 [89479.107715] [] ? irq_exit+0x5d/0x80 [89479.107732] [] ? do_IRQ+0xd2/0x130 [89479.107747] [] ? sysenter_exit+0xf/0x16 [89479.107765] [] ? trace_hardirqs_on_caller+0xfd/0x140 [89479.107782] [] ? common_interrupt+0x28/0x30 [89479.107797] ---[ end trace a1fc0a52df4a729d ]--- Reported-by: Andrey Borzenkov Signed-off-by: David Kilroy --- John, this is a regression to 2.6.27. I guess it should go into 2.6.29, and possibly stable. The latter case requires at least a path munge s?wireless/orinoco?wireless/? to apply. I can roll a separate patch for stable if necessary. Dave. --- drivers/net/wireless/orinoco/orinoco.c | 28 +++++++++++++++++++++++++--- 1 files changed, 25 insertions(+), 3 deletions(-) diff --git a/drivers/net/wireless/orinoco/orinoco.c b/drivers/net/wireless/orinoco/orinoco.c index b33e13f..2b7e2e9 100644 --- a/drivers/net/wireless/orinoco/orinoco.c +++ b/drivers/net/wireless/orinoco/orinoco.c @@ -1613,6 +1613,16 @@ 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; + unsigned long flags; + + /* orinoco_rx requires the driver lock, and we also need to + * protect priv->rx_list, so just hold the lock over the + * lot. + * + * If orinoco_lock fails, we've unplugged the card. In this + * case just abort. */ + if (orinoco_lock(priv, &flags) != 0) + return; /* extract desc and skb from queue */ list_for_each_entry_safe(rx_data, temp, &priv->rx_list, list) { @@ -1625,6 +1635,8 @@ static void orinoco_rx_isr_tasklet(unsigned long data) kfree(desc); } + + orinoco_unlock(priv, &flags); } /********************************************************************/ @@ -3649,12 +3661,22 @@ struct net_device void free_orinocodev(struct net_device *dev) { struct orinoco_private *priv = netdev_priv(dev); + struct orinoco_rx_data *rx_data, *temp; - /* No need to empty priv->rx_list: if the tasklet is scheduled - * when we call tasklet_kill it will run one final time, - * emptying the list */ + /* If the tasklet is scheduled when we call tasklet_kill it + * will run one final time. However the tasklet will only + * drain priv->rx_list if the hw is still available. */ tasklet_kill(&priv->rx_tasklet); + /* Explicitly drain priv->rx_list */ + list_for_each_entry_safe(rx_data, temp, &priv->rx_list, list) { + list_del(&rx_data->list); + + dev_kfree_skb(rx_data->skb); + kfree(rx_data->desc); + kfree(rx_data); + } + unregister_pm_notifier(&priv->pm_notifier); orinoco_uncache_fw(priv); -- 1.6.0.6