rt2x00: fix random stalls

Is possible that we stop queue and then do not wake up it again, especially when packets are transmitted fast. That can be easily reproduced with modified tx queue entry_num to some small value e.g. 16.

If mac80211 already hold local->queue_stop_reason_lock, then we can wait on that lock in both rt2x00queue_pause_queue() and rt2x00queue_unpause_queue(). After drooping ->queue_stop_reason_lock is possible that __ieee80211_wake_queue() will be performed before __ieee80211_stop_queue(), hence we stop queue and newer wake up it again.

Another race condition is possible when between rt2x00queue_threshold() check and rt2x00queue_pause_queue() we will process all pending tx buffers on different cpu. This might happen if for example intterrupt will be triggered on cpu performing rt2x00mac_tx().

To prevent race conditions serialize pause/unpause by queue->tx_lock.

Cc [email protected]

Signed-off-by Stanislaw Gruszka <[email protected]>

---
v1 -> v2:
- use spin_lock_bh
- serialize also rt2x00queue_threshold()

 drivers/net/wireless/rt2x00/rt2x00dev.c |    6 +++++-
 drivers/net/wireless/rt2x00/rt2x00mac.c |    7 +++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

drivers/net/wireless/rt2x00/rt2x00dev.c +5 -1

index 49a51b4..61a5042 100644
@@ -430,10 +430,14 @@ void rt2x00lib_txdone(struct queue_entry *entry,
 	/*
 	 * If the data queue was below the threshold before the txdone
 	 * handler we must make sure the packet queue in the mac80211 stack
-	 * is reenabled when the txdone handler has finished.
+	 * is reenabled when the txdone handler has finished. This has to be
+	 * serialized with rt2x00mac_tx(), otherwise we can wake up queue
+	 * before it was stopped.
 	 */
+	spin_lock_bh(&entry->queue->tx_lock);
 	if (!rt2x00queue_threshold(entry->queue))
 		rt2x00queue_unpause_queue(entry->queue);
+	spin_unlock_bh(&entry->queue->tx_lock);
 }
 EXPORT_SYMBOL_GPL(rt2x00lib_txdone);
 

drivers/net/wireless/rt2x00/rt2x00mac.c +5 -0

index ede3c58..923702c 100644
@@ -152,13 +152,20 @@ void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	if (unlikely(rt2x00queue_write_tx_frame(queue, skb, false)))
 		goto exit_fail;
 
+	/*
+	 * Pausing queue has to be serialized with rt2x00lib_txdone() .
+	 */
+	spin_lock(&queue->tx_lock);
 	if (rt2x00queue_threshold(queue))
 		rt2x00queue_pause_queue(queue);
+	spin_unlock(&queue->tx_lock);

Quoted 11 lines

drivers/net/wireless/rt2x00/rt2x00mac.c +5 -0

@@ -152,13 +152,20 @@ void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	if (unlikely(rt2x00queue_write_tx_frame(queue, skb, false)))
 		goto exit_fail;
 
+	/*
+	 * Pausing queue has to be serialized with rt2x00lib_txdone() .
+	 */
+	spin_lock(&queue->tx_lock);
 	if (rt2x00queue_threshold(queue))
 		rt2x00queue_pause_queue(queue);
+	spin_unlock(&queue->tx_lock);

drivers/net/wireless/rt2x00/rt2x00mac.c void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb) · line 171

Quoted 10 lines

drivers/net/wireless/rt2x00/rt2x00mac.c +2 -0

 
 	return;
 
  exit_fail:
+	spin_lock(&queue->tx_lock);
 	rt2x00queue_pause_queue(queue);
+	spin_unlock(&queue->tx_lock);
  exit_free_skb:
 	ieee80211_free_txskb(hw, skb);
 }

I'm sorry, but I'm still not convinced that we can use spin_lock_bh in one place of the code and then spin_lock in another place of the code, using the *same* spinlock.
I always use the cheat sheet shown in:
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html

which to me shows that by definition we should be using spin_lock_bh in all cases now, the new ones and the existing cases where we lock tx_lock.

Quoted 13 lines

drivers/net/wireless/rt2x00/rt2x00mac.c void rt2x00mac_tx(struct ieee80211_hw *hw, struct sk_buff *skb) · line 171

Quoted 6 lines

drivers/net/wireless/rt2x00/rt2x00mac.c +2 -0

+	spin_lock(&queue->tx_lock);
 	rt2x00queue_pause_queue(queue);
+	spin_unlock(&queue->tx_lock);
  exit_free_skb:
 	ieee80211_free_txskb(hw, skb);
 }

I'm sorry, but I'm still not convinced that we can use spin_lock_bh in one place of the code and then spin_lock in another place of the code, using the *same* spinlock.
I always use the cheat sheet shown in:
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html

Quoted 2 lines

which to me shows that by definition we should be using spin_lock_bh in all cases now, the new ones and the existing cases where we lock tx_lock.

We have bh disabled here since ieee80211_xmit is always called with bh disabled (early on dev_queue_xmit() or in ieee80211_tx_skb_tid()). I can add comment about that.

Quoted 13 lines · Stanislaw Gruszka

Quoted 13 lines

Quoted 6 lines

+ spin_lock(&queue->tx_lock);
  rt2x00queue_pause_queue(queue);
+ spin_unlock(&queue->tx_lock);
  exit_free_skb:
  ieee80211_free_txskb(hw, skb);
 }

I'm sorry, but I'm still not convinced that we can use spin_lock_bh in one place of the code and then spin_lock in another place of the code, using the *same* spinlock.
I always use the cheat sheet shown in:
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html

Quoted 6 lines

Quoted 2 lines

which to me shows that by definition we should be using spin_lock_bh in all cases now, the new ones and the existing cases where we lock tx_lock.

We have bh disabled here since ieee80211_xmit is always called with bh disabled (early on dev_queue_xmit() or in ieee80211_tx_skb_tid()). I can add comment about that.

And in fact if you were to use spin_unlock_bh() in that kind of context it would be a bug :-)