mac80211: scan ignores next_delay calculation after first probe

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

Hi,

I was debugging some problems when trying to scan for BSS (and they were often 
not recorded on channel 1) and noticed some potential problems with some code 
changes by you. Not necesserily the changes itself but the parts which look a 
little bit like they were missed.

With your commit d60277ac3fc9 ("wifi: mac80211: apply duration for SW scan"), 
I can now set the duration in SW scans (thank you). But __ieee80211_start_scan 
just overwrites the calculated next delay of ieee80211_scan_state_send_probe. 
So for the first channel, the duration still seems to be wrong.

In the past, the version from Ben Greear just overwrote the value 
IEEE80211_CHANNEL_TIME (from ieee80211_scan_state_send_probe) with the value 
IEEE80211_CHANNEL_TIME in __ieee80211_start_scan. This slightly odd behavior 
was introduced in 8a690674e060 ("mac80211: Support on-channel scan option."). 
And even when it didn't made a lot of sense to me - it didn't change the 
behavior. But now it seems to be counter productive. Maybe you can check this 
again and maybe Ben Greear still remembers why this there in the first place.

The discussion is about this part (which overwrites the correct value for 
next_delay):

static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
				  struct cfg80211_scan_request *req)
{
[snip]
	if (hw_scan) {
		__set_bit(SCAN_HW_SCANNING, &local->scanning);
	} else if ((req->n_channels == 1) &&
		   (req->channels[0] == local->hw.conf.chandef.chan)) {
[snip]

		if ((req->channels[0]->flags & (IEEE80211_CHAN_NO_IR |
						IEEE80211_CHAN_RADAR)) ||
		    !req->n_ssids) {
			next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
			if (req->n_ssids)
				set_bit(SCAN_BEACON_WAIT, &local->scanning);
		} else {
			ieee80211_scan_state_send_probe(local, &next_delay);
			next_delay = IEEE80211_CHANNEL_TIME;
		}
[snip]
}


And here is the code in for ieee80211_scan_state_send_probe which always sets 
next_delay to the correct value:

static void ieee80211_scan_state_send_probe(struct ieee80211_local *local,
					    unsigned long *next_delay)
{
[snip]
	/*
	 * After sending probe requests, wait for probe responses
	 * on the channel.
	 */
	*next_delay = msecs_to_jiffies(scan_req->duration) >
		      IEEE80211_PROBE_DELAY + IEEE80211_CHANNEL_TIME ?
		      msecs_to_jiffies(scan_req->duration) - IEEE80211_PROBE_DELAY :
		      IEEE80211_CHANNEL_TIME;
	local->next_scan_state = SCAN_DECISION;
}



And maybe you have also noticed that your patch missed the calculation for the 
passive scan in __ieee80211_start_scan. It always sets it to 
IEEE80211_PASSIVE_CHANNEL_TIME. But I would have guessed that the calculation 
should also be 

next_delay = msecs_to_jiffies(scan_req->duration) > IEEE80211_PASSIVE_CHANNEL_TIME ?
	  msecs_to_jiffies(scan_req->duration) :
	  IEEE80211_PASSIVE_CHANNEL_TIME;


Another part which seem to have been missed by your patch is the 
scan_state_decision helper code in ieee80211_scan_get_channel_time. It looks 
to me like it could now under-estimate the scan time because it doesn't handle 
the duration information.


Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]