mac80211: scan ignores next_delay calculation after first probe

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

External email : Please do not click links or open attachments until you have verified the sender or the content.

Quoted 3 lines · Sven Eckelmann

Hi,

I was debugging some problems when trying to scan for BSS (and they

were often

Quoted 1 line

not recorded on channel 1) and noticed some potential problems with

some code

Quoted 1 line

changes by you. Not necesserily the changes itself but the parts

which look a

Quoted 3 lines

little bit like they were missed.

With your commit d60277ac3fc9 ("wifi: mac80211: apply duration for

SW scan"),

Quoted 1 line

I can now set the duration in SW scans (thank you). But

__ieee80211_start_scan

Quoted 1 line

just overwrites the calculated next delay of

ieee80211_scan_state_send_probe.

Quoted 4 lines

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

Quoted 1 line

IEEE80211_CHANNEL_TIME in __ieee80211_start_scan. This slightly odd

behavior

Quoted 1 line

was introduced in 8a690674e060 ("mac80211: Support on-channel scan

option.").

Quoted 1 line

And even when it didn't made a lot of sense to me - it didn't

change the

Quoted 1 line

behavior. But now it seems to be counter productive. Maybe you can

check this

Quoted 1 line

again and maybe Ben Greear still remembers why this there in the

first place.

Hello Sven,

It's been a long time, I don't recall the exact details. But my goals were
to have minimal impact when we are scanning only the current working channel.
Shouldn't need to do any off-channel work, stop other traffic, or add extra delay in that case.

Thanks,
Ben

Quoted 2 lines

The discussion is about this part (which overwrites the correct

value for

Quoted 3 lines

next_delay):

static int __ieee80211_start_scan(struct ieee80211_sub_if_data

*sdata,

Quoted 23 lines

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]
}

Quoted 1 line

And here is the code in for ieee80211_scan_state_send_probe which

always sets

Quoted 3 lines

next_delay to the correct value:

static void ieee80211_scan_state_send_probe(struct ieee80211_local

*local,

Quoted 10 lines

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

:

Quoted 7 lines

      IEEE80211_CHANNEL_TIME;

local->next_scan_state = SCAN_DECISION;
}

And maybe you have also noticed that your patch missed the

calculation for the

Quoted 2 lines

passive scan in __ieee80211_start_scan. It always sets it to IEEE80211_PASSIVE_CHANNEL_TIME. But I would have guessed that the

calculation

Quoted 3 lines

should also be

next_delay = msecs_to_jiffies(scan_req->duration) >

IEEE80211_PASSIVE_CHANNEL_TIME ?

Quoted 4 lines

msecs_to_jiffies(scan_req->duration) :
IEEE80211_PASSIVE_CHANNEL_TIME;

Quoted 2 lines

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

Quoted 1 line

to me like it could now under-estimate the scan time because it

doesn't handle

Quoted 1 line

the duration information.

Quoted 4 lines

Kind regards,
Sven

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com