Re: [rt2x00-users] [PATCH 3.3 v2] rt2x00: fix random stalls

[Stanislaw Gruszka <sgruszka@redhat.com>]

Hi Gertjan

On Thu, Mar 08, 2012 at 10:40:43PM +0100, Gertjan van Wingerde wrote:
> > @@ -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.

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.

Additionally I ran patch with CONFIG_LOCKDEP which is capable to detect
locking context errors and no warning was printed.

Thanks
Stanislaw