Re: [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch

[Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>]

Hello Johannes,

> > > Right, it will be updated on RX. But then if we chanswitch, we would
> > > probably (mac80211 using a pointer to the non-transmitting BSS) update
> > > only one of the nontransmitting BSSes?
> > > 
> > > Just saying that maybe we need to be careful there - or your wording
> > > might be incorrect. We might end up updating a *nontransmitting* BSS,
> > > and then its transmitting/other non-tx ones only later?
> > 
> > Hmmm... I am not sure we are on the same page here. Could you please
> > clarify your concerns here ?
> 
> I'm trying to say we might have this:
> 
> cfg80211
>  * transmitting BSS 0
>    - nontx BSS 1
>    - nontx BSS 2
>    - nontx BSS 3
> mac80211
>  * ifmgd->associated (and cfg80211's wdev->current_bss?) = nontx BSS 2

Yes, this is the use-case that I tried to address in the last revision
of the patch. Suggested approach is similar to what is done for normal
case:
- to keep this hierarchy updating channels and location in rb-tree
- remove newly added hierarchy of the same transmitting BSS on the new
  channel

Note that here we update/remove not only transmitting BSSs, but their
nontx BSS hierarchies as well.

> 
> 
> Now, things like the channel information etc. will always be identical
> between the 4 BSSes, by definition.
> 
> However, if you chanswitch and mac80211 just lets cfg80211 know about
> the current_bss, then you may end up in a situation where the channel
> information is no longer the same, which is very surprising.
> 
> 
> > The normal (non multi-BSSID) BSS usecase seem to be clear: keep old and
> > remove new (if any), since it is not easy to update ifmgd->associated.
> 
> Right.
> 
> > Now let me take another look at the usecase when STA is connected to
> > a transmitting or non-transmitting BSS of a multi-BSS AP. At the moment
> > suggested code does the following. If STA is connected to the non-transmitting
> > BSS, then we switch to its transmitting BSS, instead of working with
> > current_bss directly.
> 
> We switch? Where? Maybe I missed that.

If you take a look at the top of new cfg80211_update_assoc_bss_entry
function:

+       /* use transmitting bss */
+       if (cbss->pub.transmitted_bss)
+               cbss = container_of(cbss->pub.transmitted_bss,
+                                   struct cfg80211_internal_bss,
+                                   pub);

> > So we look for the new entry (with new channel) of the transmitting BSS.
> > If it exists, then we remove it and _all_ of its non-transmitting BSSs.
> > Finally, we update channel and location in rb-tree of the existing (old)
> > transmitting BSS as well as _all_ of its non-transmitting entries.
> 
> That would indeed address the scenario I was thinking of ...

Ok! Let me know if you have any other concerns or questions.

Actually one of the major concerns is the lack of testing for the 'multi-BSSID'
scenario. I verified the 'normal' scenario using both mac80211 (iwlwifi) and
FullMAC (qtnfmac) cards. But at the moment I don't have any mac80211 card
supporting multi-BSSID.

Regards,
Sergey