mac80211: scan ignores next_delay calculation after first probe

mac80211: scan ignores next_delay calculation after first probe

[Sven Eckelmann]

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

Re: mac80211: scan ignores next_delay calculation after first probe

[Ben Greear]

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

Re: mac80211: scan ignores next_delay calculation after first probe

[Michael-cy Lee (李峻宇)]

Hi Sven & Ben, 

Thanks for the feedback, please see the inlines.

On Thu, 2024-06-06 at 06:49 -0700, Ben Greear wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  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
> 

I agree, since it only scans the current channel, the 'duration' might
be meaningless and therefore is not used here.

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

I see the protential problem here, I will send another patch to fix it.
Or maybe you already have one? You can send it to patchwork :)

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

Best, 
Michael