Re: [PATCH] wireless: Allow disabling TWT

[Ben Greear <greearb@candelatech.com>]

On 4/8/21 6:25 AM, Johannes Berg wrote:
> On Sat, 2021-03-06 at 08:20 -0800, greearb@candelatech.com wrote:
>>
>> +	/* Apply overrides as needed. */
>> +	if (ifmgd->flags & IEEE80211_STA_DISABLE_TWT) {
>> +		if (ext_capa) {
>> +			if (ext_capa && ext_capa->datalen > 10) {
>> +				ext_capa->data[9] &= ~(WLAN_EXT_CAPA10_TWT_RESPONDER_SUPPORT);
> 
> So apart from the useless code (checking "ext_capa" twice, unnecessary
> nested ifs, unnecessary parentheses), this can already be done entirely
> by userspace, since it controls the ext capa we send.

So we don't even need HE overrides in this case?

>>
>> -static bool ieee80211_twt_req_supported(const struct sta_info *sta,
>> +static bool ieee80211_twt_req_supported(struct ieee80211_sub_if_data *sdata,
>> +					const struct sta_info *sta,
>>   					const struct ieee802_11_elems *elems)
>>
> 
> This code here seems possibly wrong anyway since it doesn't take local
> capabilities into account, maybe it should, and then these changes
> wouldn't be necessary? >
>> +	/* Apply overrides as needed. */
>> +	if (ifmgd->flags & IEEE80211_STA_DISABLE_TWT) {
>> +		struct ieee80211_he_cap_elem *hec;
>> +		hec = (struct ieee80211_he_cap_elem *)(pos);
>> +		hec->mac_cap_info[0] &= ~(IEEE80211_HE_MAC_CAP0_TWT_REQ);
>> +		hec->mac_cap_info[0] &= ~(IEEE80211_HE_MAC_CAP0_TWT_RES);
>> +	}
> 
> Wait, we actually have TWT capability *twice*, once in HE and once in
> extended capabilities?! Fun.

Evidently, like maybe the spec was planning to add TWT to VHT or
something like that?

> 
> But for this shouldn't we have the more general "HE capability override"
> stuff that we have also for HT and VHT?

Maybe this can be done, but since TWT is in several different places, it
is going to be a pain to code up that way.

The way I see it, if we do go with overrides, the API is going to be pretty
tricky, basically passing a bunch of u8 around to represent different sets of IE
contents and their mask.  So I think it is cleaner to pass in specific flags such as 'disable-twt'.
Now, maybe those types of flags can be passed in as part of a nested netlink attribute
or a 'override-flags-1' u32 so that each time we add a new one it doesn't need to grow
the main netlink enum...

And, I'd still like to see a way to set station config
while it is still admin down, so that we have all of the desired local config overrides set
before we ever even start scanning...  That would be a better way to do overrides in
general in my opinion.

Thanks,
Ben

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