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;
Message-ID: <dfe27f657b4615369cf751d394f5cba75f5fdcc1.camel@sipsolutions.net>
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element
 and power constraint
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Wen Gong <wgong@codeaurora.org>, ath11k@lists.infradead.org
Cc:     linux-wireless@vger.kernel.org
Date:   Thu, 26 Aug 2021 10:29:11 +0200
In-Reply-To: <20210820122041.12157-9-wgong@codeaurora.org>
References: <20210820122041.12157-1-wgong@codeaurora.org>
         <20210820122041.12157-9-wgong@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 2021-08-20 at 08:20 -0400, Wen Gong wrote:
> 
>  	if (!(ifmgd->flags & IEEE80211_STA_DISABLE_HE)) {
>  		const struct cfg80211_bss_ies *ies;
> +		struct ieee80211_bss_conf *bss_conf;

am I missing where you set this?

> +		if (is_6ghz) {
> +			struct ieee802_11_elems elems;

This is pretty big, not sure we want it on the stack (causes warnings
for me in build). Also, if we're doing this anyway, then we can change
the code above (perhaps as a separate patch) to not do
cfg80211_find_ext_ie() but rather take it out of the parsed.

> +			u8 i, j = 0;
> +
> +			ieee802_11_parse_elems(ies->data, ies->len, false, &elems,

(line too long)

> +					       NULL, NULL);
> +
> +			if (elems.pwr_constr_elem)
> +				bss_conf->pwr_reduction = *elems.pwr_constr_elem;

before using it?

> +
> +			BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) !=
> +				     ARRAY_SIZE(elems.tx_pwr_env));
> +
> +			for (i = 0; i < elems.tx_pwr_env_num; i++) {
> +				if (elems.tx_pwr_env_len[i] >
> +				    sizeof(bss_conf->tx_pwr_env[j]))
> +					continue;

I did that in the parsing itself.

> +
> +				bss_conf->tx_pwr_env_num++;
> +				memcpy(&bss_conf->tx_pwr_env[j], elems.tx_pwr_env[i],
> +				       elems.tx_pwr_env_len[i]);

You're never resetting any of this for the next connection (if it's not
6 GHz for example, or doesn't have any data) - should probably memset
all the new members to 0 before the "if (is_6ghz)" or so?

johannes