Received-SPF: pass (google.com: domain of linux-wireless+bounces-3333-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <b74f21d22a41b2ca635e64f04c06d5db6d38d920.camel@sipsolutions.net>
Subject: Re: [v4,2/2] wifi: mac80211: refactor STA CSA parsing flows
From: Johannes Berg <johannes@sipsolutions.net>
To: Michael-CY Lee <michael-cy.lee@mediatek.com>, linux-wireless
	 <linux-wireless@vger.kernel.org>
Cc: Felix Fietkau <nbd@nbd.name>, Lorenzo Bianconi <lorenzo@kernel.org>, 
 Evelyn Tsai <evelyn.tsai@mediatek.com>, Money Wang
 <money.wang@mediatek.com>, linux-mediatek
 <linux-mediatek@lists.infradead.org>
Date: Thu, 08 Feb 2024 14:35:44 +0100
In-Reply-To: <20231222010914.6521-3-michael-cy.lee@mediatek.com>
References: <20231222010914.6521-1-michael-cy.lee@mediatek.com>
	 <20231222010914.6521-3-michael-cy.lee@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

Hi,

So ... I foolishly applied my other changes first, so I had to rebase
this - please do check.

Couple of questions/notes, if anything needs changing please send
another patch.

> +	he._oper.he_oper_params =3D cpu_to_le32(u32_encode_bits(1,
> +					IEEE80211_HE_OPERATION_6GHZ_OP_INFO));

(I changed this to le32_encode_bits, FWIW)

> +	/* if data is there validate the bandwidth & use it */
> +	if (new_chandef.chan) {
> +		if (conn_flags & IEEE80211_CONN_DISABLE_320MHZ &&
> +		    new_chandef.width =3D=3D NL80211_CHAN_WIDTH_320)
> +			ieee80211_chandef_downgrade(&new_chandef);
> =20
>  		if (conn_flags & IEEE80211_CONN_DISABLE_80P80MHZ &&
> -		    new_vht_chandef.width =3D=3D NL80211_CHAN_WIDTH_80P80)
> -			ieee80211_chandef_downgrade(&new_vht_chandef);
> +		    new_chandef.width =3D=3D NL80211_CHAN_WIDTH_80P80)
> +			ieee80211_chandef_downgrade(&new_chandef);
> +
>  		if (conn_flags & IEEE80211_CONN_DISABLE_160MHZ &&
> -		    new_vht_chandef.width =3D=3D NL80211_CHAN_WIDTH_160)
> -			ieee80211_chandef_downgrade(&new_vht_chandef);
> -	}
> +		    new_chandef.width =3D=3D NL80211_CHAN_WIDTH_160)
> +			ieee80211_chandef_downgrade(&new_chandef);
> =20


Shouldn't that have (had!) an 80 MHz handling case? Or maybe a loop a la
the one in ieee80211_config_bw():

        /*
         * Downgrade the new channel if we associated with restricted
         * bandwidth capabilities. For example, if we associated as a
         * 20 MHz STA to a 40 MHz AP (due to regulatory, capabilities
         * or config reasons) then switching to a 40 MHz channel now
         * won't do us any good -- we couldn't use it with the AP.
         */
        while (link->u.mgd.conn.bw_limit <
                        ieee80211_min_bw_limit_from_chandef(&chanreq.oper))
                ieee80211_chandef_downgrade(&chanreq.oper, NULL);


Feels like this should be the same here.

Also note how this uses ieee80211_chandef_downgrade() - we really need
to track the "chanreq.oper" vs. "chanreq.ap" in this code as well for
puncturing - can I ask you to take a brief look at that? I'll anyway
probably have to look at that soon though.

johannes