Question on why ieee80211_prep_channel clears the IEEE80211_CONN_DISABLE_160MHZ flag.

Question on why ieee80211_prep_channel clears the IEEE80211_CONN_DISABLE_160MHZ flag.

[Ben Greear]

Hello,

I'm trying to have supplicant tell the STA to not allow 160Mhz.

In the method below, in my setup, *conn_flags has IEEE80211_CONN_DISABLE_160MHZ
set when entering the method, but this method clears that and some related flags.
The clear logic dates back to 2012, effectively, but I guess in 5.19 kernel era somehow my hacks worked.

So question is, should it still be clearing the flags here?  I can add more
hack-around logic, but possibly those lines should just be removed for everyone?

static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
				  struct ieee80211_link_data *link,
				  struct cfg80211_bss *cbss,
				  ieee80211_conn_flags_t *conn_flags)
{
	struct ieee80211_local *local = sdata->local;
	const struct ieee80211_ht_cap *ht_cap = NULL;
	const struct ieee80211_ht_operation *ht_oper = NULL;
	const struct ieee80211_vht_operation *vht_oper = NULL;
	const struct ieee80211_he_operation *he_oper = NULL;
	const struct ieee80211_eht_operation *eht_oper = NULL;
	const struct ieee80211_s1g_oper_ie *s1g_oper = NULL;
	struct ieee80211_supported_band *sband;
	struct cfg80211_chan_def chandef;
	bool is_6ghz = cbss->channel->band == NL80211_BAND_6GHZ;
	bool is_5ghz = cbss->channel->band == NL80211_BAND_5GHZ;
	struct ieee80211_bss *bss = (void *)cbss->priv;
	struct ieee80211_elems_parse_params parse_params = {
		.bss = cbss,
		.link_id = -1,
		.from_ap = true,
	};
	struct ieee802_11_elems *elems;
	const struct cfg80211_bss_ies *ies;
	int ret;
	u32 i;

	pr_info("prep-channel-0, CONN_DISABLE_160MHZ: %d\n",
		!!(*conn_flags & IEEE80211_CONN_DISABLE_160MHZ));

	rcu_read_lock();

	ies = rcu_dereference(cbss->ies);
	parse_params.start = ies->data;
	parse_params.len = ies->len;
	elems = ieee802_11_parse_elems_full(&parse_params);
	if (!elems) {
		rcu_read_unlock();
		return -ENOMEM;
	}

	sband = local->hw.wiphy->bands[cbss->channel->band];

	*conn_flags &= ~(IEEE80211_CONN_DISABLE_40MHZ |
			 IEEE80211_CONN_DISABLE_80P80MHZ |
			 IEEE80211_CONN_DISABLE_160MHZ);

Thanks,
Ben

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

Re: Question on why ieee80211_prep_channel clears the IEEE80211_CONN_DISABLE_160MHZ flag.

[Johannes Berg]

On Thu, 2023-03-30 at 16:55 -0700, Ben Greear wrote:
> I'm trying to have supplicant tell the STA to not allow 160Mhz.
> 
> In the method below, in my setup, *conn_flags has IEEE80211_CONN_DISABLE_160MHZ
> set when entering the method, but this method clears that and some related flags.
> The clear logic dates back to 2012, effectively, but I guess in 5.19 kernel era somehow my hacks worked.
> 
> So question is, should it still be clearing the flags here?  I can add more
> hack-around logic, but possibly those lines should just be removed for everyone?
> 
> static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
> 				  struct ieee80211_link_data *link,
> 				  struct cfg80211_bss *cbss,
> 				  ieee80211_conn_flags_t *conn_flags)
> {
> 	struct ieee80211_local *local = sdata->local;
> 	const struct ieee80211_ht_cap *ht_cap = NULL;
> 	const struct ieee80211_ht_operation *ht_oper = NULL;
> 	const struct ieee80211_vht_operation *vht_oper = NULL;
> 	const struct ieee80211_he_operation *he_oper = NULL;
> 	const struct ieee80211_eht_operation *eht_oper = NULL;
> 	const struct ieee80211_s1g_oper_ie *s1g_oper = NULL;
> 	struct ieee80211_supported_band *sband;
> 	struct cfg80211_chan_def chandef;
> 	bool is_6ghz = cbss->channel->band == NL80211_BAND_6GHZ;
> 	bool is_5ghz = cbss->channel->band == NL80211_BAND_5GHZ;
> 	struct ieee80211_bss *bss = (void *)cbss->priv;
> 	struct ieee80211_elems_parse_params parse_params = {
> 		.bss = cbss,
> 		.link_id = -1,
> 		.from_ap = true,
> 	};
> 	struct ieee802_11_elems *elems;
> 	const struct cfg80211_bss_ies *ies;
> 	int ret;
> 	u32 i;
> 
> 	pr_info("prep-channel-0, CONN_DISABLE_160MHZ: %d\n",
> 		!!(*conn_flags & IEEE80211_CONN_DISABLE_160MHZ));
> 
> 	rcu_read_lock();
> 
> 	ies = rcu_dereference(cbss->ies);
> 	parse_params.start = ies->data;
> 	parse_params.len = ies->len;
> 	elems = ieee802_11_parse_elems_full(&parse_params);
> 	if (!elems) {
> 		rcu_read_unlock();
> 		return -ENOMEM;
> 	}
> 
> 	sband = local->hw.wiphy->bands[cbss->channel->band];
> 
> 	*conn_flags &= ~(IEEE80211_CONN_DISABLE_40MHZ |
> 			 IEEE80211_CONN_DISABLE_80P80MHZ |
> 			 IEEE80211_CONN_DISABLE_160MHZ);
> 

I'm guessing - I don't really remember right now - that this is so we
can make a new decision to set these flags? We don't really clear them
in any other place, I guess?

But honestly I don't know. There's a lot of state and maybe we should
just memset() it all whenever we disconnect (get into some kind of idle
state), just like we do with the links now that we free...

johannes

Re: Question on why ieee80211_prep_channel clears the IEEE80211_CONN_DISABLE_160MHZ flag.

[Ben Greear]

On 4/24/23 09:58, Johannes Berg wrote:
> On Thu, 2023-03-30 at 16:55 -0700, Ben Greear wrote:
>> I'm trying to have supplicant tell the STA to not allow 160Mhz.
>>
>> In the method below, in my setup, *conn_flags has IEEE80211_CONN_DISABLE_160MHZ
>> set when entering the method, but this method clears that and some related flags.
>> The clear logic dates back to 2012, effectively, but I guess in 5.19 kernel era somehow my hacks worked.
>>
>> So question is, should it still be clearing the flags here?  I can add more
>> hack-around logic, but possibly those lines should just be removed for everyone?
>>
>> static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>> 				  struct ieee80211_link_data *link,
>> 				  struct cfg80211_bss *cbss,
>> 				  ieee80211_conn_flags_t *conn_flags)
>> {
>> 	struct ieee80211_local *local = sdata->local;
>> 	const struct ieee80211_ht_cap *ht_cap = NULL;
>> 	const struct ieee80211_ht_operation *ht_oper = NULL;
>> 	const struct ieee80211_vht_operation *vht_oper = NULL;
>> 	const struct ieee80211_he_operation *he_oper = NULL;
>> 	const struct ieee80211_eht_operation *eht_oper = NULL;
>> 	const struct ieee80211_s1g_oper_ie *s1g_oper = NULL;
>> 	struct ieee80211_supported_band *sband;
>> 	struct cfg80211_chan_def chandef;
>> 	bool is_6ghz = cbss->channel->band == NL80211_BAND_6GHZ;
>> 	bool is_5ghz = cbss->channel->band == NL80211_BAND_5GHZ;
>> 	struct ieee80211_bss *bss = (void *)cbss->priv;
>> 	struct ieee80211_elems_parse_params parse_params = {
>> 		.bss = cbss,
>> 		.link_id = -1,
>> 		.from_ap = true,
>> 	};
>> 	struct ieee802_11_elems *elems;
>> 	const struct cfg80211_bss_ies *ies;
>> 	int ret;
>> 	u32 i;
>>
>> 	pr_info("prep-channel-0, CONN_DISABLE_160MHZ: %d\n",
>> 		!!(*conn_flags & IEEE80211_CONN_DISABLE_160MHZ));
>>
>> 	rcu_read_lock();
>>
>> 	ies = rcu_dereference(cbss->ies);
>> 	parse_params.start = ies->data;
>> 	parse_params.len = ies->len;
>> 	elems = ieee802_11_parse_elems_full(&parse_params);
>> 	if (!elems) {
>> 		rcu_read_unlock();
>> 		return -ENOMEM;
>> 	}
>>
>> 	sband = local->hw.wiphy->bands[cbss->channel->band];
>>
>> 	*conn_flags &= ~(IEEE80211_CONN_DISABLE_40MHZ |
>> 			 IEEE80211_CONN_DISABLE_80P80MHZ |
>> 			 IEEE80211_CONN_DISABLE_160MHZ);
>>
> 
> I'm guessing - I don't really remember right now - that this is so we
> can make a new decision to set these flags? We don't really clear them
> in any other place, I guess?
> 
> But honestly I don't know. There's a lot of state and maybe we should
> just memset() it all whenever we disconnect (get into some kind of idle
> state), just like we do with the links now that we free...

We have been running this patch below for 3 or so weeks, and have not noticed any problems.

I am personally worried about making bigger changes to this sort of logic (ie, memset),
as the over all code is convoluted and hard to think through all of the changes
and use cases.

The MLO changes seem to have done a better job of splitting up
the configured vs current settings.  I think that split is key to better
architecture over all.  Conn-flags is 'configured' as I understand it, so
probably removing mac80211 code that changes it (as opposed to changing it from
user-space or other configured input) is in the right direction.


@@ -4762,9 +4762,13 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,

  	sband = local->hw.wiphy->bands[cbss->channel->band];

+	/* This makes our logic to disable 160Mhz (at least) not work.
+	 * I am not sure it is useful in any case, so commenting out for now.
+	 * --Ben
  	*conn_flags &= ~(IEEE80211_CONN_DISABLE_40MHZ |
  			 IEEE80211_CONN_DISABLE_80P80MHZ |
  			 IEEE80211_CONN_DISABLE_160MHZ);
+	*/

  	/* disable HT/VHT/HE if we don't support them */
  	if (!sband->ht_cap.ht_supported && !is_6ghz) {

I can send a patch to just remove those three lines if you think that is good approach?

Thanks,
Ben

> 
> johannes
> 

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

Re: Question on why ieee80211_prep_channel clears the IEEE80211_CONN_DISABLE_160MHZ flag.

[Johannes Berg]

On Mon, 2023-04-24 at 12:58 -0700, Ben Greear wrote:
> > > 
> > > 	*conn_flags &= ~(IEEE80211_CONN_DISABLE_40MHZ |
> > > 			 IEEE80211_CONN_DISABLE_80P80MHZ |
> > > 			 IEEE80211_CONN_DISABLE_160MHZ);
> > > 
> > 
> > I'm guessing - I don't really remember right now - that this is so we
> > can make a new decision to set these flags? We don't really clear them
> > in any other place, I guess?

Looks like this goes back to my commit 24398e39c8ee ("mac80211: set HT
channel before association"). But looking at it briefly now, I'm not
really sure why. I mean, it _looks_ like it needs to be preserved to not
flip around the channel type between auth and assoc, but ... why this
way?

> > But honestly I don't know. There's a lot of state and maybe we should
> > just memset() it all whenever we disconnect (get into some kind of idle
> > state), just like we do with the links now that we free...
> 
> We have been running this patch below for 3 or so weeks, and have not noticed any problems.

:)

I'll note that we've also not added 320 MHz here and not seen any
issues, but that may not mean all that much.

> I am personally worried about making bigger changes to this sort of logic (ie, memset),
> as the over all code is convoluted and hard to think through all of the changes
> and use cases.
> 
> The MLO changes seem to have done a better job of splitting up
> the configured vs current settings.  I think that split is key to better
> architecture over all.  Conn-flags is 'configured' as I understand it, so
> probably removing mac80211 code that changes it (as opposed to changing it from
> user-space or other configured input) is in the right direction.

Well my point here was that for the links that aren't deflink we already
now reset them to 0 because we completely free them and reallocate new
ones, and expect that to work. So if that doesn't work for the deflink
we have a problem with the others as well, most likely.

So seems to me we should also reset this data, and see what falls out.
Maybe we'll find bugs that apply to both cases, but hopefully not that
much will break?

> @@ -4762,9 +4762,13 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
> 
>   	sband = local->hw.wiphy->bands[cbss->channel->band];
> 
> +	/* This makes our logic to disable 160Mhz (at least) not work.
> +	 * I am not sure it is useful in any case, so commenting out for now.
> +	 * --Ben
>   	*conn_flags &= ~(IEEE80211_CONN_DISABLE_40MHZ |
>   			 IEEE80211_CONN_DISABLE_80P80MHZ |
>   			 IEEE80211_CONN_DISABLE_160MHZ);
> +	*/
> 
>   	/* disable HT/VHT/HE if we don't support them */
>   	if (!sband->ht_cap.ht_supported && !is_6ghz) {
> 
> I can send a patch to just remove those three lines if you think that is good approach?

I wish I knew :)

Clearly we eventually still call ieee80211_determine_chantype(), which
should result in the same calculations again, right? And while it gets
the input conn_flags, it anyway doesn't check the bandwidth bits
there...

So _seems_ safe? Maybe we were trying to be able to upgrade a connection
to 40 MHz later on assoc, originally?

johannes