Re: mac80211: scan ignores next_delay calculation after first probe

["Michael-cy Lee (李峻宇)" <Michael-cy.Lee@mediatek.com>]

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