Re: mac80211: scan ignores next_delay calculation after first probe

[Ben Greear <greearb@candelatech.com>]

On 6/5/24 23:27, Sven Eckelmann wrote:
> 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.

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

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com