Hi Johannes,
On Tue, Oct 17, 2017 at 5:51 PM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> Hi,
>
>> While working on an issue that marvell module stops connecting to AP,
>> bisect reveals that the issue starts to happen from commit 0711d638,
>> which uses wdev->ssid_len instead of wdev->current_bss to determine
>> if driver's .disconnect() should be called.
>>
>> It happens because mwifiex_cfg80211_connect() returns -EALREADY
>> when it finds wdev->current_bss is valid:
>>
>> if (priv->wdev.current_bss) {
>> [PRINT LOG]
>> return -EALREADY;
>> }
>>
>> This would make cfg80211_connect() set wdev->ssid_len to 0, and thus
>> mwifiex_cfg80211_disconnect() won't be called by
>> cfg80211_disconnect().
>
> Hmm, none of this makes much sense to me right now.
>
> Does mwifiex treat this -EALREADY as *keeping* an old connection, or
> tearing it down entirely?
>From the call trace:
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.
> Because right now clearly cfg80211 assumes, on the one hand, that no
> connection is kept (resetting ssid_len), but on the other hand it got
> here with current_bss set - so perhaps we should reject that before in
> cfg80211, rather than in mwifiex?
Yes the inconsistency may be a problem.
> 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...
Thanks,
Jesse
