From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from cpsmtpb-ews07.kpnxchange.com ([213.75.39.10]:3335 "EHLO cpsmtpb-ews07.kpnxchange.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932805Ab2FASpt (ORCPT ); Fri, 1 Jun 2012 14:45:49 -0400 Message-ID: <4FC90DD4.8080402@gmail.com> (sfid-20120601_204553_180974_6A766333) Date: Fri, 01 Jun 2012 20:45:40 +0200 From: Gertjan van Wingerde MIME-Version: 1.0 To: Stanislaw Gruszka CC: "John W. Linville" , linux-wireless@vger.kernel.org, users@rt2x00.serialmonkey.com Subject: Re: [PATCH] rt2x00: use atomic variable for seqno References: <1338542980-16015-1-git-send-email-sgruszka@redhat.com> In-Reply-To: <1338542980-16015-1-git-send-email-sgruszka@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 06/01/12 11:29, Stanislaw Gruszka wrote: > Remove spinlock as atomic_t can be used instead. Note we use only 16 > lower bits, upper bits are changed but we impilcilty cast to u16. > > This fix possible deadlock on IBSS mode reproted by lockdep: > > ================================= > [ INFO: inconsistent lock state ] > 3.4.0-wl+ #4 Not tainted > --------------------------------- > inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage. > kworker/u:2/30374 [HC0[0]:SC0[0]:HE1:SE1] takes: > (&(&intf->seqlock)->rlock){+.?...}, at: [] rt2x00queue_create_tx_descriptor+0x380/0x490 [rt2x00lib] > {IN-SOFTIRQ-W} state was registered at: > [] __lock_acquire+0x47b/0x1050 > [] lock_acquire+0x84/0xf0 > [] _raw_spin_lock+0x33/0x40 > [] rt2x00queue_create_tx_descriptor+0x380/0x490 [rt2x00lib] > [] rt2x00queue_write_tx_frame+0x1a/0x300 [rt2x00lib] > [] rt2x00mac_tx+0x7f/0x380 [rt2x00lib] > [] __ieee80211_tx+0x1b3/0x300 [mac80211] > [] ieee80211_tx+0x105/0x130 [mac80211] > [] ieee80211_xmit+0xad/0x100 [mac80211] > [] ieee80211_subif_start_xmit+0x2d9/0x930 [mac80211] > [] dev_hard_start_xmit+0x307/0x660 > [] sch_direct_xmit+0xa1/0x1e0 > [] dev_queue_xmit+0x183/0x730 > [] neigh_resolve_output+0xfa/0x1e0 > [] ip_finish_output+0x24a/0x460 > [] ip_output+0xb7/0x100 > [] ip_local_out+0x20/0x60 > [] igmpv3_sendpack+0x4f/0x60 > [] igmp_ifc_timer_expire+0x29f/0x330 > [] run_timer_softirq+0x15c/0x2f0 > [] __do_softirq+0xae/0x1e0 > irq event stamp: 18380437 > hardirqs last enabled at (18380437): [] __slab_alloc.clone.3+0x67/0x5f0 > hardirqs last disabled at (18380436): [] __slab_alloc.clone.3+0x33/0x5f0 > softirqs last enabled at (18377616): [] __do_softirq+0x123/0x1e0 > softirqs last disabled at (18377611): [] do_softirq+0x9d/0xe0 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&(&intf->seqlock)->rlock); > > lock(&(&intf->seqlock)->rlock); > > *** DEADLOCK *** > > 4 locks held by kworker/u:2/30374: > #0: (wiphy_name(local->hw.wiphy)){++++.+}, at: [] process_one_work+0x109/0x3f0 > #1: ((&sdata->work)){+.+.+.}, at: [] process_one_work+0x109/0x3f0 > #2: (&ifibss->mtx){+.+.+.}, at: [] ieee80211_ibss_work+0x1b/0x470 [mac80211] > #3: (&intf->beacon_skb_mutex){+.+...}, at: [] rt2x00queue_update_beacon+0x24/0x50 [rt2x00lib] > > stack backtrace: > Pid: 30374, comm: kworker/u:2 Not tainted 3.4.0-wl+ #4 > Call Trace: > [] print_usage_bug+0x1f6/0x220 > [] mark_lock+0x2c2/0x300 > [] ? check_usage_forwards+0xc0/0xc0 > [] __lock_acquire+0x4bc/0x1050 > [] ? __kmalloc_track_caller+0x1c0/0x1d0 > [] ? copy_skb_header+0x26/0x90 > [] lock_acquire+0x84/0xf0 > [] ? rt2x00queue_create_tx_descriptor+0x380/0x490 [rt2x00lib] > [] _raw_spin_lock+0x33/0x40 > [] ? rt2x00queue_create_tx_descriptor+0x380/0x490 [rt2x00lib] > [] rt2x00queue_create_tx_descriptor+0x380/0x490 [rt2x00lib] > [] rt2x00queue_update_beacon_locked+0x5f/0xb0 [rt2x00lib] > [] rt2x00queue_update_beacon+0x2d/0x50 [rt2x00lib] > [] rt2x00mac_bss_info_changed+0x1ca/0x200 [rt2x00lib] > [] ? rt2x00mac_remove_interface+0x70/0x70 [rt2x00lib] > [] ieee80211_bss_info_change_notify+0xe0/0x1d0 [mac80211] > [] __ieee80211_sta_join_ibss+0x3b8/0x610 [mac80211] > [] ? mark_held_locks+0x64/0xc0 > [] ? virt_efi_query_capsule_caps+0x12/0x50 > [] ieee80211_sta_join_ibss+0xf9/0x140 [mac80211] > [] ieee80211_ibss_work+0x416/0x470 [mac80211] > [] ? trace_hardirqs_on+0xb/0x10 > [] ? skb_dequeue+0x4b/0x70 > [] ieee80211_iface_work+0x13f/0x230 [mac80211] > [] ? process_one_work+0x109/0x3f0 > [] process_one_work+0x185/0x3f0 > [] ? process_one_work+0x109/0x3f0 > [] ? ieee80211_teardown_sdata+0xa0/0xa0 [mac80211] > [] worker_thread+0x116/0x270 > [] ? manage_workers+0x1e0/0x1e0 > [] kthread+0x84/0x90 > [] ? __init_kthread_worker+0x60/0x60 > [] kernel_thread_helper+0x6/0x10 > > Cc: stable@vger.kernel.org > Signed-off-by: Stanislaw Gruszka Acked-by: Gertjan van Wingerde > --- > drivers/net/wireless/rt2x00/rt2x00.h | 3 +-- > drivers/net/wireless/rt2x00/rt2x00mac.c | 1 - > drivers/net/wireless/rt2x00/rt2x00queue.c | 13 ++++++------- > 3 files changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h > index ca36ccc..8f75402 100644 > --- a/drivers/net/wireless/rt2x00/rt2x00.h > +++ b/drivers/net/wireless/rt2x00/rt2x00.h > @@ -396,8 +396,7 @@ struct rt2x00_intf { > * for hardware which doesn't support hardware > * sequence counting. > */ > - spinlock_t seqlock; > - u16 seqno; > + atomic_t seqno; > }; > > static inline struct rt2x00_intf* vif_to_intf(struct ieee80211_vif *vif) > diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c > index ef8b984..4ff26c2 100644 > --- a/drivers/net/wireless/rt2x00/rt2x00mac.c > +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c > @@ -277,7 +277,6 @@ int rt2x00mac_add_interface(struct ieee80211_hw *hw, > else > rt2x00dev->intf_sta_count++; > > - spin_lock_init(&intf->seqlock); > mutex_init(&intf->beacon_skb_mutex); > intf->beacon = entry; > > diff --git a/drivers/net/wireless/rt2x00/rt2x00queue.c b/drivers/net/wireless/rt2x00/rt2x00queue.c > index 4c662ec..2fd8301 100644 > --- a/drivers/net/wireless/rt2x00/rt2x00queue.c > +++ b/drivers/net/wireless/rt2x00/rt2x00queue.c > @@ -207,6 +207,7 @@ static void rt2x00queue_create_tx_descriptor_seq(struct rt2x00_dev *rt2x00dev, > struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb); > struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data; > struct rt2x00_intf *intf = vif_to_intf(tx_info->control.vif); > + u16 seqno; > > if (!(tx_info->flags & IEEE80211_TX_CTL_ASSIGN_SEQ)) > return; > @@ -238,15 +239,13 @@ static void rt2x00queue_create_tx_descriptor_seq(struct rt2x00_dev *rt2x00dev, > * sequence counting per-frame, since those will override the > * sequence counter given by mac80211. > */ > - spin_lock(&intf->seqlock); > - > if (test_bit(ENTRY_TXD_FIRST_FRAGMENT, &txdesc->flags)) > - intf->seqno += 0x10; > - hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG); > - hdr->seq_ctrl |= cpu_to_le16(intf->seqno); > - > - spin_unlock(&intf->seqlock); > + seqno = atomic_add_return(0x10, &intf->seqno); > + else > + seqno = atomic_read(&intf->seqno); > > + hdr->seq_ctrl &= cpu_to_le16(IEEE80211_SCTL_FRAG); > + hdr->seq_ctrl |= cpu_to_le16(seqno); > } > > static void rt2x00queue_create_tx_descriptor_plcp(struct rt2x00_dev *rt2x00dev, -- --- Gertjan