On Wed, 2019-07-03 at 12:28 +0000, Sergey Matyukevich wrote:
> When associated BSS completes channel switch procedure, its channel
> record needs to be updated. The existing mac80211 solution was
> extended to cfg80211 in commit 5dc8cdce1d72 ("mac80211/cfg80211:
> update bss channel on channel switch")
>
> However that solution still appears to be incomplete as it may lead
> to duplicated scan entries for associated BSS after channel switch.
> The root cause of the problem is as follows. Each BSS entry is
> included into the following data structures:
> - bss list rdev->bss_list
> - bss search tree rdev->bss_tree
> Updating BSS channel record without rebuilding bss_tree may break
> tree search since cmp_bss considers all of the following: channel,
> bssid, ssid. When BSS channel is updated, but its location in bss_tree
> is not updated, then subsequent search operations may fail to locate
> this BSS. As a result, for scan performed after associated BSS channel
> switch, cfg80211_bss_update may add the second entry for the same BSS
> to both bss_list and bss_tree, rather then update the existing one.
>
> To summarize, if BSS channel needs to be updated, then bss_tree should
> be rebuilt in order to put updated BSS entry into a proper location.
>
> This commit suggests the following straightforward solution:
> - if new entry has been already created for BSS after channel switch,
> then use its IEs to update known BSS entry and then remove new
> entry completely
> - use rb_erase/rb_insert_bss reinstall updated BSS in bss_tree
>
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@xxxxxxxxxxxxx>
>
> ---
>
> v1 -> v2
> - use IEs of new BSS entry to update known BSS entry
> for this purpose extract BSS update code from cfg80211_bss_update
> into a separate function cfg80211_update_known_bss
>
> Tested on both iwlwifi and qtnfmac.
Looks good to me, a few nitpicks below.
> From my perspective, primary reason for RFC tag is nontrans_list bss handling.
> I am not sure whether nontrans_bss should be removed for new entry or not.
> The approach varies between cfg80211 API functions. For instance,
> cfg80211_unlink_bss removes them, while cfg80211_bss_expire does not.
> I would appreciate any comments in this regard.
Hmm. Good question. Ideally, if we detect CSA on any of them, we'd move
*all* of the nontrans BSSes and their transmitting BSS? Because really
that's what must happen, since a non-transmitting BSS cannot change
channel without its transmitting BSS, and vice versa.
Btw, I think expire would also remove them, since they all should always
have the same timestamp? Actually, they don't, if we jiffies changed
while we updated ... I guess we should fix that.
https://p.sipsolutions.net/09da5943735d7983.txt
> if (wdev->iftype == NL80211_IFTYPE_STATION &&
> - !WARN_ON(!wdev->current_bss))
> - wdev->current_bss->pub.channel = chandef->chan;
> + !WARN_ON(!wdev->current_bss)) {
> + cfg80211_update_assoc_bss_entry(wdev, chandef->chan);
> + }
don't really need the braces, I suppose you previously had more code
here and refactored it? :)
> +static bool
> +cfg80211_update_known_bss(struct cfg80211_registered_device *rdev,
> + struct cfg80211_internal_bss *known,
> + struct cfg80211_internal_bss *new,
> + bool signal_valid)
Maybe split out this refactoring to a separate patch, or at least add a
small statement to the commit log saying that it's just broken out of
the function.
> +void cfg80211_update_assoc_bss_entry(struct wireless_dev *wdev,
> + struct ieee80211_channel *chan)
> +{
>
[...]
> + cbss->pub.channel = chan;
> + cbss->ts = jiffies;
Not entirely sure we should set the timestamp here, you're going to have
an interesting situation where if you have a "new" entry found in the
loop below, you would actually have a *lower* timestamp?
Then again, we surely know that we still have valid data now, so I guess
that's fine. Doesn't matter that much anyway.
So yeah, looks fine to me.
johannes
