Received-SPF: pass (google.com: domain of linux-wireless+bounces-8621-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <2bcadb06-2a4e-4603-945c-839d92ace8fa@candelatech.com>
Date: Thu, 6 Jun 2024 06:49:56 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: mac80211: scan ignores next_delay calculation after first probe
To: Sven Eckelmann <sven@narfation.org>,
 Michael-CY Lee <michael-cy.lee@mediatek.com>
Cc: linux-wireless@vger.kernel.org, johannes@sipsolutions.net
References: <2540184.6tgchFWduM@ripper>
Content-Language: en-MW
From: Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <2540184.6tgchFWduM@ripper>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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