After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723241 - [review] dcbw/wifi-awesome: misc wifi fixes
[review] dcbw/wifi-awesome: misc wifi fixes
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: Wi-Fi
unspecified
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-01-29 15:14 UTC by Dan Williams
Modified: 2016-08-29 18:02 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Dan Williams 2014-01-29 15:14:11 UTC
There's a couple of things here:

1) WiFi critical protocol notification - WiFi drivers can make some critical operations more robust by lowering data rates and ensuring that Bluetooth doesn't interfere.  This would be used for association/setup related stuff like DHCP or 802.1x.  The kernel got support for this a while back, so use it.

2) NM's periodic scans should not trigger a roaming decision in the supplicant.  This happens because whenever the supplicant gets scan results (triggered by any application) it re-evaluates the list for the highest-strength access point and then roams to it.  This is undesirable, the supplicant should only roam when it finds an AP that is better according to its internal background scan logic.

3) I found an issue with invalid BSSIDs getting added to the seen bssid cache.  Fix that and clean up some code.
Comment 1 Thomas Haller 2014-01-29 16:48:29 UTC
Pushed two !fixups.


>> wifi: don't allow broadcast scans to trigger roaming decisions

Commit message: "NM's requested broadcast scans should *NOT* trigger"


>> wifi: ensure seen-bssid cache is updated on activation success

I remember that I removed this call to update_seen_bssids_cache(), and left the comment "/* No need to update seen BSSIDs cache */". You might want to remove this comment too...

But I still doubt, that it is needed. set_current_ap() always calls it when setting a different current_ap. So, either the current_ap did not change, or update_seen_bssids_cache() was already called. I'd drop this commit.
Comment 2 Dan Winship 2014-01-30 15:07:44 UTC
> wifi: add nl80211 Critical Protocol indication support

>+AC_MSG_CHECKING([Linux kernel nl80211 Critical Protocol Start/Stop])
>+AC_COMPILE_IFELSE(

(Note for later: we should fix all these checks to use AC_EGREP_CPP instead of AC_COMPILE_IFELSE, to make things a little bit faster...)

>+	/* Indicate that a critical protocol is about to start */
>+	if (strcmp (method, NM_SETTING_IP6_CONFIG_METHOD_AUTO) == 0)
>+		wifi_utils_indicate_dhcp_running (priv->wifi_data, TRUE);

Probably should add something to the comment like "Note that despite the name, the "DHCP" critical protocol ID is intended to be used with SLAAC as well."
Comment 3 Dan Williams 2014-01-30 20:04:06 UTC
Branch updated.

wifi_utils_indicate_dhcp_running() -> wifi_utils_indicate_addressing_running() and a comment added about usage of the kernel's DHCP indication.
Comment 4 Thomas Haller 2014-01-31 17:43:41 UTC
>> wifi: don't allow broadcast scans to trigger roaming decisions

    The supplicant does that just fine by itself with optimized background
    scanning.  NM's requested broadcast scans should >>>NOT<<< trigger a roaming
    decision because they are only meant to update location information
    and UI.


ACK
Comment 5 Dan Williams 2014-01-31 20:23:02 UTC
So I was going to merge this, but I realized we only use 'bgscan' for WPA-EAP networks.  So it's a bit unclear how roaming would be affected on other types of networks, more research required.

I'll probably merge the other two patches and keep the scan one in this branch.
Comment 6 Dan Williams 2014-01-31 20:37:16 UTC
Merged the two wifi critical protocol patches.

Also realized the original reason for the explicit update_seen_bssid_cache() in activation_success_handler(), which was because I changed update_seen_bssid_cache() to only accept chagnes when the device was active.  So I think that commit was valid, and I've added it back with a comment.
Comment 7 Dan Williams 2016-08-29 18:02:42 UTC
Closing as relevant patches have been merged.