From: Ron Rindjunsky <[email protected]>
This patch fixes a deadlock of sta->lock use, occuring while changing
tx aggregation states, as dev_queue_xmit end up in new function
test_and_clear_sta_flags that uses that lock thus leading to deadlock
Signed-off-by: Ron Rindjunsky <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
net/mac80211/main.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index b0fddb7..5a9030c 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -662,6 +662,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
goto start_ba_err;
}
+ spin_unlock_bh(&sta->lock);
+
/* Will put all the packets in the new SW queue */
ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
spin_unlock_bh(&local->mdev->queue_lock);
@@ -688,9 +690,9 @@ start_ba_err:
kfree(sta->ampdu_mlme.tid_tx[tid]);
sta->ampdu_mlme.tid_tx[tid] = NULL;
spin_unlock_bh(&local->mdev->queue_lock);
+ spin_unlock_bh(&sta->lock);
ret = -EBUSY;
start_ba_exit:
- spin_unlock_bh(&sta->lock);
rcu_read_unlock();
return ret;
}
@@ -829,10 +831,11 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
}
state = &sta->ampdu_mlme.tid_state_tx[tid];
- spin_lock_bh(&sta->lock);
+ /* no need to use sta->lock in this state check, as
+ * ieee80211_stop_tx_ba_session will let only
+ * one stop call to pass through per sta/tid */
if ((*state & HT_AGG_STATE_REQ_STOP_BA_MSK) == 0) {
printk(KERN_DEBUG "unexpected callback to A-MPDU stop\n");
- spin_unlock_bh(&sta->lock);
rcu_read_unlock();
return;
}
@@ -855,6 +858,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
* ieee80211_wake_queue is not used here as this queue is not
* necessarily stopped */
netif_schedule(local->mdev);
+ spin_lock_bh(&sta->lock);
*state = HT_AGG_STATE_IDLE;
sta->ampdu_mlme.addba_req_num[tid] = 0;
kfree(sta->ampdu_mlme.tid_tx[tid]);
--
1.5.4.1
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
>
> This patch is wrong, it breaks error path.locking.Will resubmit
> Tomas
>
agree, the error path should be fixed, thanks.
On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
> From: Ron Rindjunsky <[email protected]>
>
> This patch fixes a deadlock of sta->lock use, occuring while changing
> tx aggregation states, as dev_queue_xmit end up in new function
> test_and_clear_sta_flags that uses that lock thus leading to deadlock
Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
(not mac80211 related though)
> Signed-off-by: Ron Rindjunsky <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
Acked-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/main.c | 10 +++++++---
> 1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index b0fddb7..5a9030c 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -662,6 +662,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
> goto start_ba_err;
> }
>
> + spin_unlock_bh(&sta->lock);
> +
> /* Will put all the packets in the new SW queue */
> ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
> spin_unlock_bh(&local->mdev->queue_lock);
> @@ -688,9 +690,9 @@ start_ba_err:
> kfree(sta->ampdu_mlme.tid_tx[tid]);
> sta->ampdu_mlme.tid_tx[tid] = NULL;
> spin_unlock_bh(&local->mdev->queue_lock);
> + spin_unlock_bh(&sta->lock);
> ret = -EBUSY;
> start_ba_exit:
> - spin_unlock_bh(&sta->lock);
> rcu_read_unlock();
> return ret;
> }
> @@ -829,10 +831,11 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
> }
> state = &sta->ampdu_mlme.tid_state_tx[tid];
>
> - spin_lock_bh(&sta->lock);
> + /* no need to use sta->lock in this state check, as
> + * ieee80211_stop_tx_ba_session will let only
> + * one stop call to pass through per sta/tid */
> if ((*state & HT_AGG_STATE_REQ_STOP_BA_MSK) == 0) {
> printk(KERN_DEBUG "unexpected callback to A-MPDU stop\n");
> - spin_unlock_bh(&sta->lock);
> rcu_read_unlock();
> return;
> }
> @@ -855,6 +858,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
> * ieee80211_wake_queue is not used here as this queue is not
> * necessarily stopped */
> netif_schedule(local->mdev);
> + spin_lock_bh(&sta->lock);
> *state = HT_AGG_STATE_IDLE;
> sta->ampdu_mlme.addba_req_num[tid] = 0;
> kfree(sta->ampdu_mlme.tid_tx[tid]);
On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
>> From: Ron Rindjunsky <[email protected]>
>>
>> This patch fixes a deadlock of sta->lock use, occuring while changing
>> tx aggregation states, as dev_queue_xmit end up in new function
>> test_and_clear_sta_flags that uses that lock thus leading to deadlock
>
> Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
> (not mac80211 related though)
>
very encouraging :) . Can you send me details I also experience some
lockups on rc2
Thanks
Tomas
On Mon, May 26, 2008 at 5:40 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-05-26 at 17:32 +0300, Tomas Winkler wrote:
>> On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
>> <[email protected]> wrote:
>> > On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
>> >> From: Ron Rindjunsky <[email protected]>
>> >>
>> >> This patch fixes a deadlock of sta->lock use, occuring while changing
>> >> tx aggregation states, as dev_queue_xmit end up in new function
>> >> test_and_clear_sta_flags that uses that lock thus leading to deadlock
>> >
>> > Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
>> > (not mac80211 related though)
>> >
>> very encouraging :) . Can you send me details I also experience some
>> lockups on rc2
>
> Well when I resume my machine it locks up right after printing "eth0:
> resuming" or something like that, *iff* eth0 was down when suspending. I
> fixed a bug like that (NAPI related) a while ago, but w/o sungem changes
> it has re-appeared, or so it seems.
>
Thanks, doesn't look like what I see.
Tomas
On Mon, 2008-05-26 at 17:32 +0300, Tomas Winkler wrote:
> On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
> <[email protected]> wrote:
> > On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
> >> From: Ron Rindjunsky <[email protected]>
> >>
> >> This patch fixes a deadlock of sta->lock use, occuring while changing
> >> tx aggregation states, as dev_queue_xmit end up in new function
> >> test_and_clear_sta_flags that uses that lock thus leading to deadlock
> >
> > Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
> > (not mac80211 related though)
> >
> very encouraging :) . Can you send me details I also experience some
> lockups on rc2
Well when I resume my machine it locks up right after printing "eth0:
resuming" or something like that, *iff* eth0 was down when suspending. I
fixed a bug like that (NAPI related) a while ago, but w/o sungem changes
it has re-appeared, or so it seems.
johannes
On Mon, May 26, 2008 at 5:19 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2008-05-26 at 17:09 +0300, Tomas Winkler wrote:
>> From: Ron Rindjunsky <[email protected]>
>>
>> This patch fixes a deadlock of sta->lock use, occuring while changing
>> tx aggregation states, as dev_queue_xmit end up in new function
>> test_and_clear_sta_flags that uses that lock thus leading to deadlock
>
> Oh, good catch, thanks. Reminds me to debug the other deadlock I saw
> (not mac80211 related though)
>
>> Signed-off-by: Ron Rindjunsky <[email protected]>
>> Signed-off-by: Tomas Winkler <[email protected]>
>
> Acked-by: Johannes Berg <[email protected]>
>
>> ---
>> net/mac80211/main.c | 10 +++++++---
>> 1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index b0fddb7..5a9030c 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -662,6 +662,8 @@ int ieee80211_start_tx_ba_session(struct ieee80211_hw *hw, u8 *ra, u16 tid)
>> goto start_ba_err;
>> }
>>
>> + spin_unlock_bh(&sta->lock);
>> +
>> /* Will put all the packets in the new SW queue */
>> ieee80211_requeue(local, ieee802_1d_to_ac[tid]);
>> spin_unlock_bh(&local->mdev->queue_lock);
>> @@ -688,9 +690,9 @@ start_ba_err:
>> kfree(sta->ampdu_mlme.tid_tx[tid]);
>> sta->ampdu_mlme.tid_tx[tid] = NULL;
>> spin_unlock_bh(&local->mdev->queue_lock);
>> + spin_unlock_bh(&sta->lock);
>> ret = -EBUSY;
>> start_ba_exit:
>> - spin_unlock_bh(&sta->lock);
>> rcu_read_unlock();
>> return ret;
>> }
>> @@ -829,10 +831,11 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
>> }
>> state = &sta->ampdu_mlme.tid_state_tx[tid];
>>
>> - spin_lock_bh(&sta->lock);
>> + /* no need to use sta->lock in this state check, as
>> + * ieee80211_stop_tx_ba_session will let only
>> + * one stop call to pass through per sta/tid */
>> if ((*state & HT_AGG_STATE_REQ_STOP_BA_MSK) == 0) {
>> printk(KERN_DEBUG "unexpected callback to A-MPDU stop\n");
>> - spin_unlock_bh(&sta->lock);
>> rcu_read_unlock();
>> return;
>> }
>> @@ -855,6 +858,7 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid)
>> * ieee80211_wake_queue is not used here as this queue is not
>> * necessarily stopped */
>> netif_schedule(local->mdev);
>> + spin_lock_bh(&sta->lock);
>> *state = HT_AGG_STATE_IDLE;
>> sta->ampdu_mlme.addba_req_num[tid] = 0;
>> kfree(sta->ampdu_mlme.tid_tx[tid]);
>
This patch is wrong, it breaks error path.locking.Will resubmit
Tomas