Re: [PATCH] mac80211: check whether scan is in progress before queueing scan_work

[Teemu Paasikivi <ext-teemu.3.paasikivi@nokia.com>]

On Tue, 2010-04-06 at 11:06 +0200, ext Johannes Berg wrote:
> On Tue, 2010-04-06 at 11:54 +0300, Teemu Paasikivi wrote:
> > As scan_work is queued from work_work it needs to be checked if scan
> > has been started during execution of work_work. Otherwise, when hw
> > scan is used, the stack gets error about hw being busy with ongoing
> > scan.
> 
> Does that mean we ask the driver to scan twice? And your particular
> driver returns busy?
> 

Yes. There seems to be a possibility, that when ieee80211_work_work is
being executed, __ieee80211_start_scan gets called and it starts a hw
scan, and also sets local->hw_scan_req etc. (because it looks like
there's some holes in use of scan_mtx). Result is that work_work queues
ieee80211_scan_work and when it is executed, as there's already
hw_scan_req set, it will try to start hw scan again. And as the driver
used returns busy, scan_work will call ieee80211_scan_completed function
which leaves the driver (hw more precisely) scanning and the stack
thinking that it is not anymore scanning.

Obviously this kind of situation doesn't happen very often in normal
use, but it can be caused quite easily by associating to access points
in a loop while running scan in another loop.


> >  This causes the stack to abort scan without notifying the driver
> > about it. This leads to a situation where the hw is scanning and the stack
> > thinks it's not. Then when the scan finishes, the stack will complain by
> > warnings.
> > 
> > Signed-off-by: Teemu Paasikivi <ext-teemu.3.paasikivi@nokia.com>
> > ---
> >  net/mac80211/work.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> > index 1e1ea30..7bd8670 100644
> > --- a/net/mac80211/work.c
> > +++ b/net/mac80211/work.c
> > @@ -919,11 +919,16 @@ static void ieee80211_work_work(struct work_struct *work)
> >  		run_again(local, jiffies + HZ/2);
> >  	}
> >  
> > -	if (list_empty(&local->work_list) && local->scan_req)
> > +	mutex_lock(&local->scan_mtx);
> > +
> > +	if (list_empty(&local->work_list) && local->scan_req &&
> > +	    !local->scanning)
> >  		ieee80211_queue_delayed_work(&local->hw,
> >  					     &local->scan_work,
> >  					     round_jiffies_relative(0));
> >  
> > +	mutex_unlock(&local->scan_mtx);
> > +
> >  	mutex_unlock(&local->work_mtx);
> 
> That really doesn't look right ... at all.
> 
> johannes
> 

Teemu