Hi Stanislaw, On 03/07/12 19:31, Stanislaw Gruszka wrote: > 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: stable@xxxxxxxxxxxxxxx > Signed-off-by: Stanislaw Gruszka <sgruszka@xxxxxxxxxx> > --- > 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(-) > > diff --git a/drivers/net/wireless/rt2x00/rt2x00dev.c b/drivers/net/wireless/rt2x00/rt2x00dev.c > index 49a51b4..61a5042 100644 > --- a/drivers/net/wireless/rt2x00/rt2x00dev.c > +++ b/drivers/net/wireless/rt2x00/rt2x00dev.c > @@ -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); > > diff --git a/drivers/net/wireless/rt2x00/rt2x00mac.c b/drivers/net/wireless/rt2x00/rt2x00mac.c > index ede3c58..923702c 100644 > --- a/drivers/net/wireless/rt2x00/rt2x00mac.c > +++ b/drivers/net/wireless/rt2x00/rt2x00mac.c > @@ -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); > > 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. --- Gertjan -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
