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@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
