Re: [PATCH 2/4] wl1271: Fix TX starvation

[Johannes Berg <johannes@sipsolutions.net>]

On Mon, 2010-10-11 at 12:56 +0300, Juuso Oikarinen wrote:

> > @@ -537,6 +533,17 @@ static void wl1271_irq_work(struct work_struct *work)
> >  			    (wl->tx_results_count & 0xff))
> >  				wl1271_tx_complete(wl);
> >  
> > +			/* Check if any tx blocks were freed */
> > +			if ((wl->tx_blocks_available > prev_tx_blocks) &&
> > +					!skb_queue_empty(&wl->tx_queue)) {
> > +				/*
> > +				 * In order to avoid starvation of the TX path,
> > +				 * call the work function directly.
> > +				 */
> > +				cancel_work_sync(&wl->tx_work);
> 
> Hmm, isn't this causing a theoretical potential for a dead-lock? The
> tx_work could be waiting in mutex-lock already when cancel_work_sync is
> called, in which case cancel_work_sync would lock forever.
> 
> IIRC the irq_work and tx_work currently run in the same queue, so this
> may work with the current driver. Smells like a hazard anyway, and
> changing the workqueues for each work could easily lead to dead locks
> here. So at minimum I'd like to see it documented in the comment why
> this cannot cause a deadlock.

It should still cause a lockdep warning which you may want to avoid ...
and unless I'm looking at the wrong driver, this is in an irqs disabled
section which is wrong as well.

johannes