Re: [PATCH v2] wifi: mac80211: Update bssid indicator with real BSS numbers

["Allen Ye (葉芷勳)" <Allen.Ye@mediatek.com>]

On Thu, 2023-12-14 at 13:29 +0100, Johannes Berg wrote:
>  
>  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
> Indicator
> > 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?
> 
Hi Johannes,

The number of BSS is not equal to MBSSID element count plus 1 because
there might be multiple nontransmitted BSSID profile subelements in one
MBSSID element. Also, one nontransmitted BSSID profile subelement might
be fragmented across two MBSSID elements if the length of the
subelement exceeds 255 octets.

> > 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_data *sdata,
> >  /* copy in optional mbssid_ies */
> >  if (mbssid) {
> >  u8 *pos = new->tail + new->tail_len;
> > +const struct element *mbssid_elem;
> >  
> >  new->mbssid_ies = (void *)pos;
> >  pos += struct_size(new->mbssid_ies, elem, mbssid->cnt);
> > +mbssid_elem = (const struct element *)pos;
> >  pos += ieee80211_copy_mbssid_beacon(pos, new->mbssid_ies,
> >      mbssid);
> >  if (rnr) {
> > @@ -1175,8 +1177,7 @@ ieee80211_assign_beacon(struct
> ieee80211_sub_if_data *sdata,
> >  ieee80211_copy_rnr_beacon(pos, new->rnr_ies, rnr);
> >  }
> >  /* update bssid_indicator */
> > -link_conf->bssid_indicator =
> > -ilog2(__roundup_pow_of_two(mbssid->cnt + 1));
> > +link_conf->bssid_indicator = 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

Whether the subelement is aggregated or fragmented, the MaxBSSID
Indicator field would be the same for the multiple BSSID set.
Therefore, we directly retrieve this field from the element.
For example, there are five BSSes in one multiple BSSID set, one
transmitted and four non-transmitted BSSes. We might use just two
MBSSID elements to store all the non-transmitted BSS information. Here
the MaxBSSID Indicator is 3 in both MBSSID elements. However, the
element cnt is 2 and if we use the original method to calculate the
BSSID Indicator we will get 2 which is wrong.

Thanks,
Allen