mbox series

[v2,00/22] add support for S1G association

Message ID 20200831205600.21058-1-thomas@adapt-ip.com
Headers show
Series add support for S1G association | expand

Message

Thomas Pedersen Aug. 31, 2020, 8:55 p.m. UTC
This is the initial S1G patchset which adds support for:

- defining the S1G 900MHz bands in a custom regulatory database
- setting and receiving S1G beacons (sending short beacons will be
  supported in a future patch)
- configuring S1G capabilities in Association Request (setting
  capabilities along with NL80211_CMD_SET_STATION will be added later).
- scanning on S1G bands
- handling S1G Association Response format
- correctly encoding Listen Interval for S1G
- associating in mac80211
- testing S1G in mac80211_hwsim

Rate control is still TBD, this patchset simply lops off the rate
control hooks for S1G so eg. missing sband->bitrates and S1G Basic Rate
set can't do too much damage.

Note the mac80211_hwsim S1G support introduces a regression in a few
hostap hwsim tests. This is because when processing the reported bands,
hostap assumes freq < 4000 is 11b, and the actual 11b/g band is
overwritten by the S1G band info. Though it does count as a userspace
regression, I'm not sure there is much to do about it besides apply a
small patch to hostapd which treats freq < 2000 as an unknown band.

After the hostap workaround
(https://lists.infradead.org/pipermail/hostap/2020-August/038748.html),
these patches continue to pass the hwsim tests as well as HEAD.


Thomas Pedersen (22):
  ieee80211: redefine S1G bits with GENMASK
  nl80211: advertise supported channel width in S1G
  cfg80211: regulatory: handle S1G channels
  nl80211: correctly validate S1G beacon head
  nl80211: support setting S1G channels
  {cfg,mac}80211: get correct default channel width for S1G
  mac80211: s1g: choose scanning width based on frequency
  nl80211: support S1G capabilities
  mac80211: support S1G STA capabilities
  cfg80211: convert S1G beacon to scan results
  cfg80211: parse S1G Operation element for BSS channel
  mac80211: convert S1G beacon to scan results
  cfg80211: handle Association Response from S1G STA
  mac80211: encode listen interval for S1G
  mac80211: don't calculate duration for S1G
  mac80211: handle S1G low rates
  mac80211: avoid rate init for S1G band
  mac80211: receive and process S1G beacons
  mac80211: support S1G association
  nl80211: include frequency offset in survey info
  mac80211_hwsim: indicate support for S1G
  mac80211_hwsim: fix TSF timestamp write to S1G beacon

 drivers/net/wireless/mac80211_hwsim.c |  92 +++++++++--
 include/linux/ieee80211.h             | 223 +++++++++++++++++---------
 include/net/cfg80211.h                |  28 ++++
 include/net/mac80211.h                |   3 +
 include/uapi/linux/nl80211.h          |  26 +++
 net/mac80211/cfg.c                    |   2 +
 net/mac80211/chan.c                   |   9 +-
 net/mac80211/ibss.c                   |   3 +-
 net/mac80211/ieee80211_i.h            |  20 +++
 net/mac80211/iface.c                  |   5 +
 net/mac80211/mlme.c                   | 184 +++++++++++++++++----
 net/mac80211/rate.c                   |  39 ++++-
 net/mac80211/rx.c                     |  87 +++++-----
 net/mac80211/scan.c                   |  37 ++++-
 net/mac80211/tx.c                     |   4 +
 net/mac80211/util.c                   | 200 +++++++++++++++++++++++
 net/wireless/chan.c                   | 140 ++++++++++------
 net/wireless/mlme.c                   |  20 +++
 net/wireless/nl80211.c                |  52 +++++-
 net/wireless/reg.c                    |  70 ++++++--
 net/wireless/scan.c                   |  80 +++++++--
 net/wireless/util.c                   |  32 ++++
 22 files changed, 1090 insertions(+), 266 deletions(-)

Comments

Thomas Pedersen Sept. 5, 2020, 6:38 p.m. UTC | #1
On 2020-08-31 13:55, Thomas Pedersen wrote:
> S1G channels have a minimum bandwidth of 1Mhz, and there
> is a 1:1 mapping of allowed bandwidth to channel number.
> 
> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com>
> 
> ---
> 
> v2:
>  - drop the freq_reg_info() interface changes and move the
>    check for S1G band inside. Fixes a driver compile
>    error.
>  - fix iterating past bws[] in __freq_reg_info() by
>    setting initial element to 0.
>    Reported-by: kernel test robot <lkp@intel.com>
> ---
>  net/wireless/reg.c | 70 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 0ab7808fcec8..be6f54b70ad3 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -1617,9 +1617,11 @@ __freq_reg_info(struct wiphy *wiphy, u32
> center_freq, u32 min_bw)
>  {
>  	const struct ieee80211_regdomain *regd = reg_get_regdomain(wiphy);
>  	const struct ieee80211_reg_rule *reg_rule = NULL;
> +	const u32 bws[] = {0, 1, 2, 4, 5, 8, 10, 16, 20};
> +	int i = sizeof(bws) / sizeof(u32) - 1;

This could be 'int i = ARRAY_SIZE(bws) - 1'.

>  	u32 bw;
> 
> -	for (bw = MHZ_TO_KHZ(20); bw >= min_bw; bw = bw / 2) {
> +	for (bw = MHZ_TO_KHZ(bws[i]); bw >= min_bw; bw = 
> MHZ_TO_KHZ(bws[i--])) {
>  		reg_rule = freq_reg_info_regd(center_freq, regd, bw);
>  		if (!IS_ERR(reg_rule))
>  			return reg_rule;
> @@ -1631,7 +1633,9 @@ __freq_reg_info(struct wiphy *wiphy, u32
> center_freq, u32 min_bw)
>  const struct ieee80211_reg_rule *freq_reg_info(struct wiphy *wiphy,
>  					       u32 center_freq)
>  {
> -	return __freq_reg_info(wiphy, center_freq, MHZ_TO_KHZ(20));
> +	u32 min_bw = center_freq < MHZ_TO_KHZ(1000) ? 1 : 20;
> +
> +	return __freq_reg_info(wiphy, center_freq, MHZ_TO_KHZ(min_bw));
>  }
>  EXPORT_SYMBOL(freq_reg_info);
> 
> @@ -1659,6 +1663,7 @@ static uint32_t reg_rule_to_chan_bw_flags(const
> struct ieee80211_regdomain *regd
>  {
>  	const struct ieee80211_freq_range *freq_range = NULL;
>  	u32 max_bandwidth_khz, center_freq_khz, bw_flags = 0;
> +	bool is_s1g = chan->band == NL80211_BAND_S1GHZ;
> 
>  	freq_range = &reg_rule->freq_range;
> 
> @@ -1678,16 +1683,57 @@ static uint32_t
> reg_rule_to_chan_bw_flags(const struct ieee80211_regdomain *regd
>  					MHZ_TO_KHZ(20)))
>  		bw_flags |= IEEE80211_CHAN_NO_20MHZ;
> 
> -	if (max_bandwidth_khz < MHZ_TO_KHZ(10))
> -		bw_flags |= IEEE80211_CHAN_NO_10MHZ;
> -	if (max_bandwidth_khz < MHZ_TO_KHZ(20))
> -		bw_flags |= IEEE80211_CHAN_NO_20MHZ;
> -	if (max_bandwidth_khz < MHZ_TO_KHZ(40))
> -		bw_flags |= IEEE80211_CHAN_NO_HT40;
> -	if (max_bandwidth_khz < MHZ_TO_KHZ(80))
> -		bw_flags |= IEEE80211_CHAN_NO_80MHZ;
> -	if (max_bandwidth_khz < MHZ_TO_KHZ(160))
> -		bw_flags |= IEEE80211_CHAN_NO_160MHZ;
> +	if (is_s1g) {
> +		/* S1G is strict about non overlapping channels. We can
> +		 * calculate which bandwidth is allowed per channel by finding
> +		 * the largest bandwidth which cleanly divides the freq_range.
> +		 */
> +		int edge_offset;
> +		int ch_bw = max_bandwidth_khz;
> +
> +		while (ch_bw) {
> +			edge_offset = (center_freq_khz - ch_bw / 2) -
> +				      freq_range->start_freq_khz;
> +			if (edge_offset % ch_bw == 0) {
> +				switch (KHZ_TO_MHZ(ch_bw)) {
> +				case 1:
> +					bw_flags |= IEEE80211_CHAN_1MHZ;
> +					break;
> +				case 2:
> +					bw_flags |= IEEE80211_CHAN_2MHZ;
> +					break;
> +				case 4:
> +					bw_flags |= IEEE80211_CHAN_4MHZ;
> +					break;
> +				case 8:
> +					bw_flags |= IEEE80211_CHAN_8MHZ;
> +					break;
> +				case 16:
> +					bw_flags |= IEEE80211_CHAN_16MHZ;
> +					break;
> +				default:
> +					/* If we got here, no bandwidths fit on
> +					 * this frequency, ie. band edge.
> +					 */
> +					bw_flags |= IEEE80211_CHAN_DISABLED;
> +					break;
> +				}
> +				break;
> +			}
> +			ch_bw /= 2;
> +		}
> +	} else {
> +		if (max_bandwidth_khz < MHZ_TO_KHZ(10))
> +			bw_flags |= IEEE80211_CHAN_NO_10MHZ;
> +		if (max_bandwidth_khz < MHZ_TO_KHZ(20))
> +			bw_flags |= IEEE80211_CHAN_NO_20MHZ;
> +		if (max_bandwidth_khz < MHZ_TO_KHZ(40))
> +			bw_flags |= IEEE80211_CHAN_NO_HT40;
> +		if (max_bandwidth_khz < MHZ_TO_KHZ(80))
> +			bw_flags |= IEEE80211_CHAN_NO_80MHZ;
> +		if (max_bandwidth_khz < MHZ_TO_KHZ(160))
> +			bw_flags |= IEEE80211_CHAN_NO_160MHZ;
> +	}
>  	return bw_flags;
>  }
Thomas Pedersen Sept. 8, 2020, 6:30 p.m. UTC | #2
On 2020-09-06 09:04, Johannes Berg wrote:
> On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
>> 
>> Note the mac80211_hwsim S1G support introduces a regression in a few
>> hostap hwsim tests. This is because when processing the reported 
>> bands,
>> hostap assumes freq < 4000 is 11b, and the actual 11b/g band is
>> overwritten by the S1G band info. Though it does count as a userspace
>> regression, I'm not sure there is much to do about it besides apply a
>> small patch to hostapd which treats freq < 2000 as an unknown band.
>> 
>> After the hostap workaround
>> (https://lists.infradead.org/pipermail/hostap/2020-August/038748.html),
>> these patches continue to pass the hwsim tests as well as HEAD.
> 
> 
> That sounds like we could "hack around" it by sending the S1G data
> first, and then the 2.4 GHz, so the latter overwrites it on broken
> versions?

Yes that could work.

> Not sure it's worth it though, I'd say it depends a bit on what real
> hardware plans are?
> 
> I mean, if it's only hwsim for now ... who cares? And if it's going to
> be special hardware that only does S1G, then also meh, you need newer
> versions to support it, big deal.

AFAIK there are no multi-band S1G chips. The initial focus (from WFA) 
seems
to be industrial IOT.

> But if OTOH a commonly used chipset like e.g. ath9k or ath10k will get
> S1G support, then that'd be more relevant?
Johannes Berg Sept. 18, 2020, 10:40 a.m. UTC | #3
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
> An S1G BSS can beacon at either 1 or 2 MHz and the channel

> width is unique to a given frequency. Ignore scan channel

> width for now and use the allowed channel width.

> 

> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com>

> ---

>  net/mac80211/scan.c | 17 +++++++++++++++++

>  1 file changed, 17 insertions(+)

> 

> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c

> index 5ac2785cdc7b..5002791fe165 100644

> --- a/net/mac80211/scan.c

> +++ b/net/mac80211/scan.c

> @@ -905,6 +905,17 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local,

>  	local->scan_chandef.center_freq1 = chan->center_freq;

>  	local->scan_chandef.freq1_offset = chan->freq_offset;

>  	local->scan_chandef.center_freq2 = 0;

> +

> +	/* For scanning on the S1G band, ignore scan_width (which is constant

> +	 * across all channels) for now since channel width is specific to each

> +	 * channel. Detect the required channel width here and likely revisit

> +	 * later. Maybe scan_width could be used to build the channel scan list?

> +	 */

> +	if (chan->band == NL80211_BAND_S1GHZ) {

> +		local->scan_chandef.width = ieee80211_s1g_channel_width(chan);

> +		goto  set_channel;

> +	}


nit: double space after 'goto'

but really I came to say that this probably changes then, if you don't
convince me about the stuff in the previous patch review? :)

So I'm leaving this patch also for now - have applied 1-5 so far.

johannes
Johannes Berg Sept. 18, 2020, 10:45 a.m. UTC | #4
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
> Extract the BSS primary channel from the S1G Operation

> element.


Out of curiosity, do you even need to?

I mean ... you know what channel you received it on, surely?

> @@ -1318,15 +1318,26 @@ cfg80211_get_bss_channel(struct wiphy *wiphy, const u8 *ie, size_t ielen,

>  	tmp = cfg80211_find_ie(WLAN_EID_DS_PARAMS, ie, ielen);

>  	if (tmp && tmp[1] == 1) {

>  		channel_number = tmp[2];

> -	} else {

> -		tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen);

> -		if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) {

> -			struct ieee80211_ht_operation *htop = (void *)(tmp + 2);

> +		goto found_channel;

> +	}

>  

> -			channel_number = htop->primary_chan;

> -		}

> +	tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen);

> +	if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) {

> +		struct ieee80211_ht_operation *htop = (void *)(tmp + 2);

> +

> +		channel_number = htop->primary_chan;

> +		goto found_channel;

> +	}

> +

> +	tmp = cfg80211_find_ie(WLAN_EID_S1G_OPERATION, ie, ielen);

> +	if (tmp && tmp[1] >= sizeof(struct ieee80211_s1g_oper_ie)) {

> +		struct ieee80211_s1g_oper_ie *s1gop = (void *)(tmp + 2);

> +

> +		channel_number = s1gop->primary_ch;

> +		goto found_channel;

>  	}


I *am* a bit worried about this though - do you really want to try to
parse DS elements on S1G, or S1G elements on other bands? Seems like
there ought to be a band check here?

johannes
Johannes Berg Sept. 18, 2020, 10:50 a.m. UTC | #5
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
> 
> +	/* Detect whether this was an S1G Association Response and adjust IE
> +	 * location accordingly.
> +	 */
> +	rcu_read_lock();
> +	ies = rcu_dereference(bss->ies);
> +	if (WARN_ON(!ies)) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +	s1g = cfg80211_find_ie(WLAN_EID_S1G_CAPABILITIES, ies->data, ies->len);
> +	if (s1g) {
> +		cr.resp_ie = (u8 *)&mgmt->u.s1g_assoc_resp.variable;
> +		cr.resp_ie_len =
> +			len - offsetof(struct ieee80211_mgmt,
> +				       u.s1g_assoc_resp.variable);
> +	}
> +	rcu_read_unlock();

That ... is rather strange?

Why not check bss->channel->band?

johannes
Johannes Berg Sept. 18, 2020, 10:52 a.m. UTC | #6
On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
> For now just skip the duration calculation for frames
> transmitted on the S1G band and avoid a warning.
> 
> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com>
> ---
>  net/mac80211/tx.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index d2136007e2eb..bef19616c5f0 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -82,6 +82,10 @@ static __le16 ieee80211_duration(struct ieee80211_tx_data *tx,
>  
>  	erp = txrate->flags & IEEE80211_RATE_ERP_G;
>  
> +	/* TODO */
> +	if (sband->band == NL80211_BAND_S1GHZ)
> +		return 0;


I'm not even sure I'd leave a TODO there ... just say

	/* device is expected to do this */

or so?

johannes
Johannes Berg Sept. 18, 2020, 10:55 a.m. UTC | #7
Oops, sorry. I just realized I've been replying to v2, while *looking*
at v3 in patchwork ...

I'll continue on v3, but this seems mostly the same.

johannes
Thomas Pedersen Sept. 21, 2020, 5:06 a.m. UTC | #8
On 2020-09-18 03:40, Johannes Berg wrote:
> On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:

>> An S1G BSS can beacon at either 1 or 2 MHz and the channel

>> width is unique to a given frequency. Ignore scan channel

>> width for now and use the allowed channel width.

>> 

>> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com>

>> ---

>>  net/mac80211/scan.c | 17 +++++++++++++++++

>>  1 file changed, 17 insertions(+)

>> 

>> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c

>> index 5ac2785cdc7b..5002791fe165 100644

>> --- a/net/mac80211/scan.c

>> +++ b/net/mac80211/scan.c

>> @@ -905,6 +905,17 @@ static void 

>> ieee80211_scan_state_set_channel(struct ieee80211_local *local,

>>  	local->scan_chandef.center_freq1 = chan->center_freq;

>>  	local->scan_chandef.freq1_offset = chan->freq_offset;

>>  	local->scan_chandef.center_freq2 = 0;

>> +

>> +	/* For scanning on the S1G band, ignore scan_width (which is 

>> constant

>> +	 * across all channels) for now since channel width is specific to 

>> each

>> +	 * channel. Detect the required channel width here and likely 

>> revisit

>> +	 * later. Maybe scan_width could be used to build the channel scan 

>> list?

>> +	 */

>> +	if (chan->band == NL80211_BAND_S1GHZ) {

>> +		local->scan_chandef.width = ieee80211_s1g_channel_width(chan);

>> +		goto  set_channel;

>> +	}

> 

> nit: double space after 'goto'

> 

> but really I came to say that this probably changes then, if you don't

> convince me about the stuff in the previous patch review? :)

> 

> So I'm leaving this patch also for now - have applied 1-5 so far.


Thanks. I'm not really sure what else would make sense here? 
scan_req->scan_width is constant across all channels in 
scan_req->channels so for S1G we can either filter the scan_req channels 
list based on scan_width (kind of strange and unexpected), or deduce the 
correct chanenl width for each channel in the list and ignore scan_width 
(mostly correct). It seems like scan_width is currently only used for 
scanning at 5 or 10MHz anyway?

-- 
thomas
Thomas Pedersen Sept. 21, 2020, 5:12 a.m. UTC | #9
On 2020-09-18 03:45, Johannes Berg wrote:
> On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
>> Extract the BSS primary channel from the S1G Operation
>> element.
> 
> Out of curiosity, do you even need to?
> 
> I mean ... you know what channel you received it on, surely?

Consider the case where the BSS is operating @ 2Mhz, but primary is one 
of
the 1Mhz channels. The hardware (or driver) may not be able to tell you
exactly which primary channel (upper or lower) the packet came in on.

>> @@ -1318,15 +1318,26 @@ cfg80211_get_bss_channel(struct wiphy *wiphy, 
>> const u8 *ie, size_t ielen,
>>  	tmp = cfg80211_find_ie(WLAN_EID_DS_PARAMS, ie, ielen);
>>  	if (tmp && tmp[1] == 1) {
>>  		channel_number = tmp[2];
>> -	} else {
>> -		tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen);
>> -		if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) {
>> -			struct ieee80211_ht_operation *htop = (void *)(tmp + 2);
>> +		goto found_channel;
>> +	}
>> 
>> -			channel_number = htop->primary_chan;
>> -		}
>> +	tmp = cfg80211_find_ie(WLAN_EID_HT_OPERATION, ie, ielen);
>> +	if (tmp && tmp[1] >= sizeof(struct ieee80211_ht_operation)) {
>> +		struct ieee80211_ht_operation *htop = (void *)(tmp + 2);
>> +
>> +		channel_number = htop->primary_chan;
>> +		goto found_channel;
>> +	}
>> +
>> +	tmp = cfg80211_find_ie(WLAN_EID_S1G_OPERATION, ie, ielen);
>> +	if (tmp && tmp[1] >= sizeof(struct ieee80211_s1g_oper_ie)) {
>> +		struct ieee80211_s1g_oper_ie *s1gop = (void *)(tmp + 2);
>> +
>> +		channel_number = s1gop->primary_ch;
>> +		goto found_channel;
>>  	}
> 
> I *am* a bit worried about this though - do you really want to try to
> parse DS elements on S1G, or S1G elements on other bands? Seems like
> there ought to be a band check here?

OK. I'll rework this to handle garbage input a little better.
Thomas Pedersen Sept. 21, 2020, 5:52 a.m. UTC | #10
On 2020-09-18 03:50, Johannes Berg wrote:
> On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
>> 
>> +	/* Detect whether this was an S1G Association Response and adjust IE
>> +	 * location accordingly.
>> +	 */
>> +	rcu_read_lock();
>> +	ies = rcu_dereference(bss->ies);
>> +	if (WARN_ON(!ies)) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +	s1g = cfg80211_find_ie(WLAN_EID_S1G_CAPABILITIES, ies->data, 
>> ies->len);
>> +	if (s1g) {
>> +		cr.resp_ie = (u8 *)&mgmt->u.s1g_assoc_resp.variable;
>> +		cr.resp_ie_len =
>> +			len - offsetof(struct ieee80211_mgmt,
>> +				       u.s1g_assoc_resp.variable);
>> +	}
>> +	rcu_read_unlock();
> 
> That ... is rather strange?
> 
> Why not check bss->channel->band?

Thanks! That saves a lot of work :)
Thomas Pedersen Sept. 21, 2020, 6:03 a.m. UTC | #11
On 2020-09-18 03:52, Johannes Berg wrote:
> On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
>> For now just skip the duration calculation for frames
>> transmitted on the S1G band and avoid a warning.
>> 
>> Signed-off-by: Thomas Pedersen <thomas@adapt-ip.com>
>> ---
>>  net/mac80211/tx.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index d2136007e2eb..bef19616c5f0 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -82,6 +82,10 @@ static __le16 ieee80211_duration(struct 
>> ieee80211_tx_data *tx,
>> 
>>  	erp = txrate->flags & IEEE80211_RATE_ERP_G;
>> 
>> +	/* TODO */
>> +	if (sband->band == NL80211_BAND_S1GHZ)
>> +		return 0;
> 
> 
> I'm not even sure I'd leave a TODO there ... just say
> 
> 	/* device is expected to do this */

Eventually it would be nice if mac80211 could calculate duration for 
S1G? Do you know why it doesn't for HT/VHT?
Johannes Berg Sept. 21, 2020, 6:51 a.m. UTC | #12
On Sun, 2020-09-20 at 23:03 -0700, Thomas Pedersen wrote:
> 

> > I'm not even sure I'd leave a TODO there ... just say

> > 

> > 	/* device is expected to do this */

> 

> Eventually it would be nice if mac80211 could calculate duration for 

> S1G? Do you know why it doesn't for HT/VHT?


I'm not sure it matters. It almost doesn't even matter for legacy
(CCK/OFDM) because devices practically always do it anyway?

And for HT/VHT it's just quite a bit more complicated, so we never
bothered, since it wasn't necessary.

It couldn't even really be done since there are A-MPDUs and other things
happening at a lower layer.

johannes
Johannes Berg Sept. 21, 2020, 6:54 a.m. UTC | #13
On Sun, 2020-09-20 at 22:12 -0700, Thomas Pedersen wrote:
> On 2020-09-18 03:45, Johannes Berg wrote:
> > On Mon, 2020-08-31 at 13:55 -0700, Thomas Pedersen wrote:
> > > Extract the BSS primary channel from the S1G Operation
> > > element.
> > 
> > Out of curiosity, do you even need to?
> > 
> > I mean ... you know what channel you received it on, surely?
> 
> Consider the case where the BSS is operating @ 2Mhz, but primary is one 
> of
> the 1Mhz channels. The hardware (or driver) may not be able to tell you
> exactly which primary channel (upper or lower) the packet came in on.

Ah, OK, makes sense. Somehow based on a comment somewhere else I thought
you were saying that the channels are basically all unique in their
(center frequency, bandwidth) tuple, and was assuming you'd actually
have to scan them that way.

johannes
Johannes Berg Sept. 21, 2020, 6:58 a.m. UTC | #14
On Sun, 2020-09-20 at 22:06 -0700, Thomas Pedersen wrote:
> 

> > > +	/* For scanning on the S1G band, ignore scan_width (which is 

> > > constant

> > > +	 * across all channels) for now since channel width is specific to 

> > > each

> > > +	 * channel. Detect the required channel width here and likely 

> > > revisit

> > > +	 * later. Maybe scan_width could be used to build the channel scan 

> > > list?

> > > +	 */

> > > +	if (chan->band == NL80211_BAND_S1GHZ) {

> > > +		local->scan_chandef.width = ieee80211_s1g_channel_width(chan);

> > > +		goto  set_channel;

> > > +	}

> > 

> > nit: double space after 'goto'

> > 

> > but really I came to say that this probably changes then, if you don't

> > convince me about the stuff in the previous patch review? :)

> > 

> > So I'm leaving this patch also for now - have applied 1-5 so far.

> 

> Thanks. I'm not really sure what else would make sense here? 

> scan_req->scan_width is constant across all channels in 

> scan_req->channels so for S1G we can either filter the scan_req channels 

> list based on scan_width (kind of strange and unexpected), or deduce the 

> correct chanenl width for each channel in the list and ignore scan_width 

> (mostly correct). It seems like scan_width is currently only used for 

> scanning at 5 or 10MHz anyway?


Yeah, that's true, it's sort of undefined if you're not in 5 or 10, and
then we currently assume it's 20, but obviously for S1G we should assume
then it's 1 MHz or something.

FWIW, here's probably where I thought you have a unique (freq, bw) tuple
and thus shouldn't really need to have the parsing in the other patch?
But I never looked at the spec so far ...


Anyway, wrt. the code here, I think perhaps you should just simply
remove the reference to scan_width? I'm not sure what you'd really do
with it, since it's a 5/10 MHz thing?

TBH though, offhand I don't even know how the 5/10 MHz scanning is
supposed to work?

johannes