[PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[Tomas Winkler]

From: Guy Cohen <guy.cohen@intel.com>

Putting netif_carrier_on before configuring the driver/device with the
new association state may cause a race (tx frames may be sent before
configuration is done)

Signed-off-by: Guy Cohen <guy.cohen@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 net/mac80211/mlme.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 37ea04f..cb414de 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -564,15 +564,14 @@ static void ieee80211_set_associated(struct net_device *dev,
 			sdata->bss_conf.ht_bss_conf = &conf->ht_bss_conf;
 		}
 
-		netif_carrier_on(dev);
 		ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
 		memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
 		memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
 		ieee80211_sta_send_associnfo(dev, ifsta);
 	} else {
+		netif_carrier_off(dev);
 		ieee80211_sta_tear_down_BA_sessions(dev, ifsta->bssid);
 		ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
-		netif_carrier_off(dev);
 		changed |= ieee80211_reset_erp_info(dev);
 
 		sdata->bss_conf.assoc_ht = 0;
@@ -586,6 +585,10 @@ static void ieee80211_set_associated(struct net_device *dev,
 
 	sdata->bss_conf.assoc = assoc;
 	ieee80211_bss_info_change_notify(sdata, changed);
+
+	if (assoc)
+		netif_carrier_on(dev);
+
 	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
 	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
 }
-- 
1.5.4.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[Johannes Berg]

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


> -		netif_carrier_on(dev);
>  		ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
>  		memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
>  		memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
>  		ieee80211_sta_send_associnfo(dev, ifsta);
>  	} else {
> +		netif_carrier_off(dev);
>  		ieee80211_sta_tear_down_BA_sessions(dev, ifsta->bssid);
>  		ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
> -		netif_carrier_off(dev);

Maybe it should then be symmetric so that callers of the function have
to do it in both cases? Other than that, looks good.

johannes

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

Re: [PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[Tomas Winkler]

On Thu, Jul 3, 2008 at 8:07 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
>> -             netif_carrier_on(dev);
>>               ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
>>               memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
>>               memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
>>               ieee80211_sta_send_associnfo(dev, ifsta);
>>       } else {
>> +             netif_carrier_off(dev);
>>               ieee80211_sta_tear_down_BA_sessions(dev, ifsta->bssid);
>>               ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
>> -             netif_carrier_off(dev);
>
> Maybe it should then be symmetric so that callers of the function have
> to do it in both cases? Other than that, looks good.

The sequence now is
  configure association in the driver  -> carrier_on ..... carrier_off
-> configure disassociation in the driver

So this is already symmetric or I don't understand what  you mean.

Thanks
Tomas

Re: [PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[Johannes Berg]

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

On Thu, 2008-07-03 at 20:26 +0300, Tomas Winkler wrote:
> On Thu, Jul 3, 2008 at 8:07 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> >> -             netif_carrier_on(dev);
> >>               ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
> >>               memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
> >>               memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
> >>               ieee80211_sta_send_associnfo(dev, ifsta);
> >>       } else {
> >> +             netif_carrier_off(dev);
> >>               ieee80211_sta_tear_down_BA_sessions(dev, ifsta->bssid);
> >>               ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
> >> -             netif_carrier_off(dev);
> >
> > Maybe it should then be symmetric so that callers of the function have
> > to do it in both cases? Other than that, looks good.
> 
> The sequence now is
>   configure association in the driver  -> carrier_on ..... carrier_off
> -> configure disassociation in the driver
> 
> So this is already symmetric or I don't understand what  you mean.

Yes, the call sequence is symmetric, but I was thinking whether it would
be better API-wise to have the caller of ieee80211_set_associated() do
both carrier_on and carrier_off, instead of requiring that it calls
carrier_on but calling carrier_off for it.

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

Re: [PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[Tomas Winkler]

T24gVGh1LCBKdWwgMywgMjAwOCBhdCA4OjMzIFBNLCBKb2hhbm5lcyBCZXJnIDxqb2hhbm5lc0Bz
aXBzb2x1dGlvbnMubmV0PiB3cm90ZToKPiBPbiBUaHUsIDIwMDgtMDctMDMgYXQgMjA6MjYgKzAz
MDAsIFRvbWFzIFdpbmtsZXIgd3JvdGU6Cj4+IE9uIFRodSwgSnVsIDMsIDIwMDggYXQgODowNyBQ
TSwgSm9oYW5uZXMgQmVyZyA8am9oYW5uZXNAc2lwc29sdXRpb25zLm5ldD4gd3JvdGU6Cj4+ID4K
Pj4gPj4gLSAgICAgICAgICAgICBuZXRpZl9jYXJyaWVyX29uKGRldik7Cj4+ID4+ICAgICAgICAg
ICAgICAgaWZzdGEtPmZsYWdzIHw9IElFRUU4MDIxMV9TVEFfUFJFVl9CU1NJRF9TRVQ7Cj4+ID4+
ICAgICAgICAgICAgICAgbWVtY3B5KGlmc3RhLT5wcmV2X2Jzc2lkLCBzZGF0YS0+dS5zdGEuYnNz
aWQsIEVUSF9BTEVOKTsKPj4gPj4gICAgICAgICAgICAgICBtZW1jcHkod3JxdS5hcF9hZGRyLnNh
X2RhdGEsIHNkYXRhLT51LnN0YS5ic3NpZCwgRVRIX0FMRU4pOwo+PiA+PiAgICAgICAgICAgICAg
IGllZWU4MDIxMV9zdGFfc2VuZF9hc3NvY2luZm8oZGV2LCBpZnN0YSk7Cj4+ID4+ICAgICAgIH0g
ZWxzZSB7Cj4+ID4+ICsgICAgICAgICAgICAgbmV0aWZfY2Fycmllcl9vZmYoZGV2KTsKPj4gPj4g
ICAgICAgICAgICAgICBpZWVlODAyMTFfc3RhX3RlYXJfZG93bl9CQV9zZXNzaW9ucyhkZXYsIGlm
c3RhLT5ic3NpZCk7Cj4+ID4+ICAgICAgICAgICAgICAgaWZzdGEtPmZsYWdzICY9IH5JRUVFODAy
MTFfU1RBX0FTU09DSUFURUQ7Cj4+ID4+IC0gICAgICAgICAgICAgbmV0aWZfY2Fycmllcl9vZmYo
ZGV2KTsKPj4gPgo+PiA+IE1heWJlIGl0IHNob3VsZCB0aGVuIGJlIHN5bW1ldHJpYyBzbyB0aGF0
IGNhbGxlcnMgb2YgdGhlIGZ1bmN0aW9uIGhhdmUKPj4gPiB0byBkbyBpdCBpbiBib3RoIGNhc2Vz
PyBPdGhlciB0aGFuIHRoYXQsIGxvb2tzIGdvb2QuCj4+Cj4+IFRoZSBzZXF1ZW5jZSBub3cgaXMK
Pj4gICBjb25maWd1cmUgYXNzb2NpYXRpb24gaW4gdGhlIGRyaXZlciAgLT4gY2Fycmllcl9vbiAu
Li4uLiBjYXJyaWVyX29mZgo+PiAtPiBjb25maWd1cmUgZGlzYXNzb2NpYXRpb24gaW4gdGhlIGRy
aXZlcgo+Pgo+PiBTbyB0aGlzIGlzIGFscmVhZHkgc3ltbWV0cmljIG9yIEkgZG9uJ3QgdW5kZXJz
dGFuZCB3aGF0ICB5b3UgbWVhbi4KPgo+IFllcywgdGhlIGNhbGwgc2VxdWVuY2UgaXMgc3ltbWV0
cmljLCBidXQgSSB3YXMgdGhpbmtpbmcgd2hldGhlciBpdCB3b3VsZAo+IGJlIGJldHRlciBBUEkt
d2lzZSB0byBoYXZlIHRoZSBjYWxsZXIgb2Yg77u/aWVlZTgwMjExX3NldF9hc3NvY2lhdGVkKCkg
ZG8KPiBib3RoIGNhcnJpZXJfb24gYW5kIGNhcnJpZXJfb2ZmLCBpbnN0ZWFkIG9mIHJlcXVpcmlu
ZyB0aGF0IGl0IGNhbGxzCj4gY2Fycmllcl9vbiBidXQgY2FsbGluZyBjYXJyaWVyX29mZiBmb3Ig
aXQuCgpJIHRoaW5rIGl0J3MgYWN0dWFsbHkgb2theSB0byBiZSAgaW5zaWRlIGFuZCBJIHdvdWxk
IHJhdGhlciBwcmVmZXIgdG8Kc3BsaXQgdGhpcyBmdW5jdGlvbjsgdGhlcmUgaXMgYWxyZWFkeSBv
bmUgZnVuY3Rpb24gY2FsbGVkCmllZWU4MDIxMV9zZXRfZGlzYXNzb2NpYXRlZC4gQnV0IHNpbmNl
IHdlIGhhdmUgdG8gZml4IGRpc3NhY29zaWF0aW9uCmFud2F5IGFuZCB3ZSBuZWVkIHRvIGludmVz
dGlnYXRlIGl0IG1vcmUgSSB3b3VsZCBmaXJzdCB1c2UgdGhpcyBsaXR0bGUKZml4LgpUaGFua3MK
VG9tYXMK

Re: [PATCH 1/1] mac80211: move netif_carrier_on to after ieee80211_bss_info_change_notify

[Johannes Berg]

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

On Thu, 2008-07-03 at 20:45 +0300, Tomas Winkler wrote:
> On Thu, Jul 3, 2008 at 8:33 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Thu, 2008-07-03 at 20:26 +0300, Tomas Winkler wrote:
> >> On Thu, Jul 3, 2008 at 8:07 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> >> >
> >> >> -             netif_carrier_on(dev);
> >> >>               ifsta->flags |= IEEE80211_STA_PREV_BSSID_SET;
> >> >>               memcpy(ifsta->prev_bssid, sdata->u.sta.bssid, ETH_ALEN);
> >> >>               memcpy(wrqu.ap_addr.sa_data, sdata->u.sta.bssid, ETH_ALEN);
> >> >>               ieee80211_sta_send_associnfo(dev, ifsta);
> >> >>       } else {
> >> >> +             netif_carrier_off(dev);
> >> >>               ieee80211_sta_tear_down_BA_sessions(dev, ifsta->bssid);
> >> >>               ifsta->flags &= ~IEEE80211_STA_ASSOCIATED;
> >> >> -             netif_carrier_off(dev);
> >> >
> >> > Maybe it should then be symmetric so that callers of the function have
> >> > to do it in both cases? Other than that, looks good.
> >>
> >> The sequence now is
> >>   configure association in the driver  -> carrier_on ..... carrier_off
> >> -> configure disassociation in the driver
> >>
> >> So this is already symmetric or I don't understand what  you mean.
> >
> > Yes, the call sequence is symmetric, but I was thinking whether it would
> > be better API-wise to have the caller of ieee80211_set_associated() do
> > both carrier_on and carrier_off, instead of requiring that it calls
> > carrier_on but calling carrier_off for it.
> 
> I think it's actually okay to be  inside and I would rather prefer to
> split this function; there is already one function called
> ieee80211_set_disassociated. But since we have to fix dissacosiation
> anway and we need to investigate it more I would first use this little
> fix.

Sure ok, thanks for looking into this stuff.

johannes

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