[PATCH] cfg80211: set SME state machine correctly for roam event

[PATCH] cfg80211: set SME state machine correctly for roam event

[Zhu Yi]

When we receive a successful status in CFG80211_SME_CONNECTED state,
it is a roam event. We should mark it as a success result.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 net/wireless/sme.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 8e2ef54..1aa1190 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -393,7 +393,8 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 	}
 
 	if (status == WLAN_STATUS_SUCCESS &&
-	    wdev->sme_state == CFG80211_SME_IDLE)
+	    (wdev->sme_state == CFG80211_SME_IDLE ||
+	     wdev->sme_state == CFG80211_SME_CONNECTED))
 		goto success;
 
 	if (wdev->sme_state != CFG80211_SME_CONNECTING)
-- 
1.6.0.4

Re: [PATCH] cfg80211: set SME state machine correctly for roam event

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Thu, 2009-08-13 at 17:23 +0800, Zhu Yi wrote:
> When we receive a successful status in CFG80211_SME_CONNECTED state,
> it is a roam event. We should mark it as a success result.

But there's a cfg80211_roamed() call for that? Can the driver not tell
the difference? It also sends a different event (ROAMED rather than
CONNECTED) to userspace.

johannes

> Cc: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Zhu Yi <yi.zhu@intel.com>
> ---
>  net/wireless/sme.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 8e2ef54..1aa1190 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -393,7 +393,8 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
>  	}
>  
>  	if (status == WLAN_STATUS_SUCCESS &&
> -	    wdev->sme_state == CFG80211_SME_IDLE)
> +	    (wdev->sme_state == CFG80211_SME_IDLE ||
> +	     wdev->sme_state == CFG80211_SME_CONNECTED))
>  		goto success;
>  
>  	if (wdev->sme_state != CFG80211_SME_CONNECTING)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] cfg80211: set SME state machine correctly for roam event

[Zhu Yi]

On Thu, 2009-08-13 at 17:54 +0800, Johannes Berg wrote:
> On Thu, 2009-08-13 at 17:23 +0800, Zhu Yi wrote:
> > When we receive a successful status in CFG80211_SME_CONNECTED state,
> > it is a roam event. We should mark it as a success result.
> 
> But there's a cfg80211_roamed() call for that? Can the driver not tell
> the difference? It also sends a different event (ROAMED rather than
> CONNECTED) to userspace.

The device notifies both when it begins to roam and after the new
association is made. Yes, I think I missed the cfg80211_roamed call for
the real roam event. But there is still a reassociation path that the
above situation could happen (__cfg80211_connect_result is called while
in CFG80211_SME_CONNECTED state). Or do you think we should suppress
reassoc event from driver?

Actually, the code in __cfg80211_connect_result() has already handled
the (wdev->sme_state == CFG80211_SME_CONNECTED) case. The problem is
wdev->current_bss is set to NULL but leaves wdev->sme_state still as
CFG80211_SME_CONNECTED. So I think the patch is still valid, but needs a
better description.

Thanks,
-yi

Re: [PATCH] cfg80211: set SME state machine correctly for roam event

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Fri, 2009-08-14 at 11:58 +0800, Zhu Yi wrote:

> The device notifies both when it begins to roam and after the new
> association is made. Yes, I think I missed the cfg80211_roamed call for
> the real roam event. But there is still a reassociation path that the
> above situation could happen (__cfg80211_connect_result is called while
> in CFG80211_SME_CONNECTED state). Or do you think we should suppress
> reassoc event from driver?

Would that be after a disconnect event?

I think that after a DISCONNECTED event the device should go to sleep
completely and wait for userspace instructions. Otherwise we end up
having a weird situation _again_ where userspace has no idea what the
device is doing. I suppose if the device just keeps trying you just have
to tell it to stop after a bit if it doesn't find a new connections.

Otherwise, if you're roaming, you still get a disconnect/reassoc or
something like that? Those together should form a ROAMED event.

> Actually, the code in __cfg80211_connect_result() has already handled
> the (wdev->sme_state == CFG80211_SME_CONNECTED) case. The problem is
> wdev->current_bss is set to NULL but leaves wdev->sme_state still as
> CFG80211_SME_CONNECTED. So I think the patch is still valid, but needs a
> better description.

I don't think I understand?

Why would you ever have a connect_result() while already connected?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] cfg80211: set SME state machine correctly for roam event

[Jussi Kivilinna]

Quoting "Johannes Berg" <johannes@sipsolutions.net>:

> On Thu, 2009-08-13 at 17:23 +0800, Zhu Yi wrote:
>> When we receive a successful status in CFG80211_SME_CONNECTED state,
>> it is a roam event. We should mark it as a success result.
>
> But there's a cfg80211_roamed() call for that? Can the driver not tell
> the difference? It also sends a different event (ROAMED rather than
> CONNECTED) to userspace.
>

rndis sends 'media connect' indication when device  
associates/reassociates/"roams and associates with new AP". Specs also  
say that "must not make a media disconnect indication when  
reassociating or roaming".

-Jussi

Re: [PATCH] cfg80211: set SME state machine correctly for roam event

[Zhu Yi]

On Fri, 2009-08-14 at 22:54 +0800, Johannes Berg wrote:
> On Fri, 2009-08-14 at 11:58 +0800, Zhu Yi wrote:
> 
> > The device notifies both when it begins to roam and after the new
> > association is made. Yes, I think I missed the cfg80211_roamed call for
> > the real roam event. But there is still a reassociation path that the
> > above situation could happen (__cfg80211_connect_result is called while
> > in CFG80211_SME_CONNECTED state). Or do you think we should suppress
> > reassoc event from driver?
> 
> Would that be after a disconnect event?
> 
> I think that after a DISCONNECTED event the device should go to sleep
> completely and wait for userspace instructions. Otherwise we end up
> having a weird situation _again_ where userspace has no idea what the
> device is doing. I suppose if the device just keeps trying you just have
> to tell it to stop after a bit if it doesn't find a new connections.
> 
> Otherwise, if you're roaming, you still get a disconnect/reassoc or
> something like that? Those together should form a ROAMED event.

Yes, the iwmc3200wifi device sends special disconnect and reassoc events
for roaming. But since we know we'll be reassociated shortly, we don't
propagate this special disconnect event outside (to kernel SME and
userspace). I agree we should use cfg80211_roamed() to indicate a real
roam event. But sometimes the device reassociated to the same BSSID. Do
you think the driver should simply ignore this event or still call
cfg80211_connect_result() as well?

> > Actually, the code in __cfg80211_connect_result() has already handled
> > the (wdev->sme_state == CFG80211_SME_CONNECTED) case. The problem is
> > wdev->current_bss is set to NULL but leaves wdev->sme_state still as
> > CFG80211_SME_CONNECTED. So I think the patch is still valid, but needs a
> > better description.
> 
> I don't think I understand?
> 
> Why would you ever have a connect_result() while already connected?

Continue with the above case, in my current implementation, I call 
cfg80211_connect_result(). Then I find the SME enters a weird state
(CFG80211_SME_CONNECTED while wdev->current_bss is NULL). So I think if
we don't allow cfg80211_connect_result() to be called while
CFG80211_SME_CONNECTED, let's put a big WARN_ON at the beginning of
__cfg80211_connect_result(). The current code seems allow this case
(i.e. send some roam event), thus I'm confused.

Thanks,
-yi

Re: [PATCH] cfg80211: set SME state machine correctly for roam event

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

On Mon, 2009-08-17 at 10:35 +0800, Zhu Yi wrote:

> Yes, the iwmc3200wifi device sends special disconnect and reassoc events
> for roaming. But since we know we'll be reassociated shortly, we don't
> propagate this special disconnect event outside (to kernel SME and
> userspace). I agree we should use cfg80211_roamed() to indicate a real
> roam event. But sometimes the device reassociated to the same BSSID. Do
> you think the driver should simply ignore this event or still call
> cfg80211_connect_result() as well?

You probably can't just ignore it since it might require re-doing WPA
handshake, so I think I'd just send a "roamed to the same" event.

> Continue with the above case, in my current implementation, I call 
> cfg80211_connect_result(). Then I find the SME enters a weird state
> (CFG80211_SME_CONNECTED while wdev->current_bss is NULL). So I think if
> we don't allow cfg80211_connect_result() to be called while
> CFG80211_SME_CONNECTED, let's put a big WARN_ON at the beginning of
> __cfg80211_connect_result(). The current code seems allow this case
> (i.e. send some roam event), thus I'm confused.

Ok, I'll take a look. There probably was a reason that's somewhat
related to the cfg80211 SME, but it must be possible to change.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]