[RFC 1/1] libertas: new scan logic

[Dan Williams]

On Thu, 2007-11-29 at 23:42 +0100, Holger Schurig wrote:
> I'm now at home (without the hardware) and tomorrow at a customer,
> so I'll this will be just a VERY rant about things that come to mind.
> 
> > Maybe add some comments here about the magic numbers, ie something about
> > making the basic rates marked the way the firmware expects.
> 
> Good idea.
> 
> 
> 
> > So here you're now blocking until the scan is complete.  We don't really
> > want to do that...
> 
> Looks like you didn't run the code??? :-)
> 
> When the interface is first brought up and you (or the network scripts)
> do an initial "iwlist eth1 essid MUMBLEFUTZ", then it does a full scan,
> over all 11/13/14 channels, in one go and blocking. It doesn't use
> the worker thread than. In this case the code blocks.
> 
> But when you later run "iwlist eth1 scan", then lbs_set_scan() merely
> starts the worker thread for the scanning. Inside this thread it again
> blocks, but this doesn't matter for the user-space application. Even

Except that you're running it in the same thread as the association
thread, the main thread, everything else.  I'm not really concerned
about blocking applications, I'm concerned about blocking internal
driver stuff there.  Maybe this isn't really a problem in practice, but
it looked suspicious while reading the patch.

> with strace, you can see that the SIWSCAN returns (almost) immediately.
> iwlist than spends it's time called 3-6 times calling GIWSCAN ioctl,
> which ends up in lbs_get_scan(), and returns (2-5 times) immediately with
> -EAGAIN. Again nothing blocks, at least not the user-space.
> 
> You should have known that it doesn't block, because before I had the
> -EAGAIN added, my problem was that "iwlist eth1 scan" returned
> prematurely :-)

Again, I'm not talking about UI stuff; I'm talking about blocking on the
firmware command submission in lbs_prepare_and_send_command () where
your patch uses WAITFORRSP, while the original code uses '0' there to
indicate nonblocking command submission.  If this is intentional and you
feel that it's fine to block waiting for the command submission (and
response!!), then that's OK.  But by using WAITFORRSP there the code
will actually wait for each scan command to complete, but in most cases
we don't really care about the command completion, because the response
handling should pretty much be asynchronous.  Unless you're doing
full-blown tracking of the sub-scan commands on a queue and removing
them when the responses come back, I don't really see why the patch
needs to wait on the scan command response.

Dan

> > The scan command should just be fired off
> 
> This is what lbs_set_scan() does.
> 
> 
> 
> > I haven't looked deeply enough, but are you sure the chan_count stuff
> > will do what you want here?  If adapter->last_scanned_channel = 11...
> 
> When I run this code, the last_scanned_channel is first 0, then 4, then 8,
> then 12 (in this case it only scans 3 channels). So yes, the channel
> number and the index into the channel list are advanced correctly. And
> with "lbsdebug +host +hex" you can see that it adds different TLVs for
> the channel information to the CMD_802_11_SCAN command.
> 
> 
> 
> Then identation problems that were in the [RFC] patch don't exist
> in the submitted [PATCH].
> 
> 
> Thanks for looking throught and giving input :-)