Re: [PATCH 9/9] mac80211: save transmit power envelope element and power constraint

[Wen Gong <wgong@codeaurora.org>]

Hi johannes,

Could you see my answer below?
please feel free to point out the mistakes :)

On 2021-07-30 18:47, Wen Gong wrote:
> On 2021-07-23 17:38, Johannes Berg wrote:
>> On Mon, 2021-05-17 at 16:19 -0400, Wen Gong wrote:
>>> 
>>> +		if (is_6ghz) {
>>> +			struct ieee802_11_elems elems;
>>> +			struct ieee80211_bss_conf *bss_conf;
>>> +			u8 i, n;
>>> +
>>> +			ieee802_11_parse_elems(ies->data, ies->len, false, &elems,
>>> +					       NULL, NULL);
>>> +			bss_conf = &sdata->vif.bss_conf;
>>> +			bss_conf->pwr_reduction = 0;
>>> +			if (elems.pwr_constr_elem)
>>> +				bss_conf->pwr_reduction = *elems.pwr_constr_elem;
>>> +
>>> +			memset(bss_conf->tx_pwr_env, 0, sizeof(bss_conf->tx_pwr_env));
>>> +			bss_conf->tx_pwr_env_num = elems.tx_pwr_env_num;
>>> +			n = min_t(u8, elems.tx_pwr_env_num,
>>> +				  ARRAY_SIZE(elems.tx_pwr_env));
>> 
>> If anything, that min_t would make sense only if you were actually 
>> using
>> ARRAY_SIZE(bss_conf->tx_pwr_env), but like this it's quite pointless,
>> just checking again if the element parsing was internally consistent?
>> 
>> I'd probably remove it and throw in a
>> 
>> 	BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) !=
>>                      ARRAY_SIZE(elems.tx_pwr_env));
>> 
>> instead.
>> 
>>> +			for (i = 0; i < n; i++)
>>> +				memcpy(&bss_conf->tx_pwr_env[i], elems.tx_pwr_env[i],
>>> +				       elems.tx_pwr_env_len[i]);
>> 
>> You also never validated that the element wasn't too long!
>> 
> will change it.
>> 
>> If you connect to 6 Ghz with this, and then again to another AP that
>> doesn't, you'll have it stuck at the old values. You need to reset at
>> some point (during disconnect).
>> 
> will change to reset it in ieee80211_prep_channel outside is_6ghz{}.
> Then it will be reset for each connection.
>> And then two more questions:
>> 
>> 1) Could this information change? Should we track it in beacons?
>> 
> 
> The information is from AP side, it should be not changed untill the AP 
> restart.
> If someone want to change configure of AP, the AP should restart and
> then take effect by my understand.
> Is it have some case for this information change?
> 
> 
>> 2) Should we at least check it again from the protected beacon or such
>> after association, so we don't blindly trust the probe response or
>> beacon (received during scan, not validated) at least when BIGTK is in
>> use?
> 
> May we add support for BIGTK in future with another patch?
> The info(pwr_reduction and tx_pwr_env) is used by lower driver such as 
> ath11k.
> If the info changed after association, then how to notify lower driver?
> Do it like below in ieee80211_rx_mgmt_beacon()?
> And use BSS_CHANGED_TXPOWER or a new enum in ieee80211_bss_change?
> 
> ieee80211_rx_mgmt_beacon{
> 	changed |= ieee80211_handle_pwr_constr(sdata, chan, mgmt,
> 					       elems.country_elem,
> 					       elems.country_elem_len,
> 					       elems.pwr_constr_elem,
> 					       elems.cisco_dtpc_elem);
> 
> 	ieee80211_bss_info_change_notify(sdata, changed);
> }
> 
>> 
>> johannes

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k