Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: wgong=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Fri, 30 Jul 2021 18:47:50 +0800
From:   Wen Gong <wgong@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCH 9/9] mac80211: save transmit power envelope element and
 power constraint
In-Reply-To: <d9491db4ece67ac78eb39a1078b91a106770fbb0.camel@sipsolutions.net>
References: <20210517201932.8860-1-wgong@codeaurora.org>
 <20210517201932.8860-10-wgong@codeaurora.org>
 (sfid-20210517_222034_029448_A9A89D57)
 <d9491db4ece67ac78eb39a1078b91a106770fbb0.camel@sipsolutions.net>
Message-ID: <1126f8d996e895ae048092b3f8aad19b@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

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