Re: Commit 0711d638 breaks mwifiex

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2017-10-17 at 18:18 +0800, Jesse Sung wrote:

> > Does mwifiex treat this -EALREADY as *keeping* an old connection,
> > or tearing it down entirely?
> 
> From the call trace:

Well, the call trace can't really answer that :-)
Does mwifiex firmware stay connected?


> 139.451318: nl80211_get_valid_chan <-nl80211_connect
> 139.451321: cfg80211_connect <-nl80211_connect
> 139.451322: cfg80211_oper_and_ht_capa <-cfg80211_connect
> 139.451323: mwifiex_cfg80211_connect <-cfg80211_connect
> 139.451337: nl80211_post_doit <-genl_family_rcv_msg
> 139.451423: nl80211_pre_doit <-genl_family_rcv_msg
> 139.451425: nl80211_disconnect <-genl_family_rcv_msg
> 139.451426: cfg80211_disconnect <-nl80211_disconnect
> 139.451430: mwifiex_cfg80211_disconnect <-cfg80211_disconnect
> 
> mwifiex_cfg80211_disconnect() would be called after
> mwifiex_cfg80211_connect(), though I'm not sure if it's triggered by
> the error returned.

I think so - it's probably wpa_supplicant trying to get back to a well-
known state (of being disconnected).

> > I think your fix is invalid because we then reset ssid_len and
> > still
> > keep an old connection (current_bss) which will lead to strange
> > nl80211
> > behaviour when getting interface data etc.
> 
> Since this is how it works before commit 0711d638 (use current_bss
> instead of ssid_len), so I'm guessing this would still work. But I
> agree that this may not be a proper fix...

It would probably work, but we get data inconsistencies, and at the
very least you get no SSID data back when you query the current state.

I don't see anything in nl80211 or so that would say we should accept a
connect() while already connected, so how about this?

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index b347e63d7aaa..fe0037ad1f5e 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -1042,6 +1042,9 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev,
 
 	ASSERT_WDEV_LOCK(wdev);
 
+	if (wdev->current_bss)
+		return -EALREADY;
+
 	if (WARN_ON(wdev->connect_keys)) {
 		kzfree(wdev->connect_keys);
 		wdev->connect_keys = NULL;

Not really quite sure about it yet, but that should address the issue?

johannes