Received-SPF: pass (google.com: domain of linux-wireless+bounces-793-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <65d21336e8f7e180352403a3e894aeaf27a22cab.camel@sipsolutions.net>
Subject: Re: [PATCH v2] wifi: mac80211: Update bssid indicator with real BSS
 numbers
From: Johannes Berg <johannes@sipsolutions.net>
To: Allen Ye <allen.ye@mediatek.com>, linux-wireless@vger.kernel.org, 
	Lorenzo Bianconi <lorenzo@kernel.org>
Cc: Felix Fietkau <nbd@nbd.name>, Lorenzo Bianconi
 <lorenzo.bianconi@redhat.com>,  Evelyn Tsai <evelyn.tsai@mediatek.com>,
 Money Wang <money.wang@mediatek.com>,  linux-mediatek@lists.infradead.org
Date: Thu, 14 Dec 2023 13:29:42 +0100
In-Reply-To: <20231208063820.25983-1-allen.ye@mediatek.com>
References: <20231208063820.25983-1-allen.ye@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.2 (3.50.2-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

Hi,

We should have Lorenzo here, he wrote the original code.

On Fri, 2023-12-08 at 14:38 +0800, Allen Ye wrote:
> The cnt member in mbssid is the count of total number of MBSSID elements
> instead of BSSID. Therefore, we fix this by reading the MaxBSSID Indicato=
r
> field directly.

I'll say I don't understand this much ...

Are you trying to have BSSIDs that are hidden from the kernel? Or not
contiguous in the MBSSID set? Not sure how the two can be not
equivalent?

> Co-developed-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
> Signed-off-by: Evelyn Tsai <evelyn.tsai@mediatek.com>
> Co-developed-by: Money Wang <money.wang@mediatek.com>
> Signed-off-by: Money Wang <money.wang@mediatek.com>
> Signed-off-by: Allen Ye <allen.ye@mediatek.com>

I have to admit that I chuckled a bit about this for a 5 line patch :-)

> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 606b1b2e4123..f90bcd59f85a 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -1164,9 +1164,11 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_da=
ta *sdata,
>  	/* copy in optional mbssid_ies */
>  	if (mbssid) {
>  		u8 *pos =3D new->tail + new->tail_len;
> +		const struct element *mbssid_elem;
> =20
>  		new->mbssid_ies =3D (void *)pos;
>  		pos +=3D struct_size(new->mbssid_ies, elem, mbssid->cnt);
> +		mbssid_elem =3D (const struct element *)pos;
>  		pos +=3D ieee80211_copy_mbssid_beacon(pos, new->mbssid_ies,
>  						    mbssid);
>  		if (rnr) {
> @@ -1175,8 +1177,7 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_dat=
a *sdata,
>  			ieee80211_copy_rnr_beacon(pos, new->rnr_ies, rnr);
>  		}
>  		/* update bssid_indicator */
> -		link_conf->bssid_indicator =3D
> -			ilog2(__roundup_pow_of_two(mbssid->cnt + 1));
> +		link_conf->bssid_indicator =3D mbssid_elem->data[0];

But this seems fishy to me, if you look into the element itself, you're
going to have to do some validation on it? And what about fragmentation?

johannes