| Message ID | 1250155399-17847-1-git-send-email-yi.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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)
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(-)