| Message ID | 1383569955-13236-1-git-send-email-luciano.coelho@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hey Luca, > A beacon should never have a Channel Switch Announcement information > element with a count of 0, because a count of 1 means switch just > before the next beacon. So, if a count of 0 was valid in a beacon, it > would have been transmitted in the next channel already, which is > useless. A CSA count equal to zero is only meaningful in action > frames or probe_responses. > > Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() > functions accordingly. > > Cc: Simon Wunderlich <sw@simonwunderlich.de> > Signed-off-by: Luciano Coelho <luciano.coelho@intel.com> > --- > > Hi Simon (et al), > > I identified this issue while playing around with CSA. I noticed that > we are sending a CSA beaon with count == 0, which should not happen. > The last beacon visible in the current channel (ie. before the switch) > contains a CSA IE with count == 1. > > I wanted to check with you if my proposed change would have any > side-effects, especially with the ath9k driver, which is the only user > of this code in the mainline at the moment. > > The potential danger here is if you don't check > ieee80211_csa_is_complete() before you send the first CSA beacon out. > With the previous code, there would always be a beacon with CSA (count > == 0), but now, if the count starts with 1, there won't be any. If > you don't check, my patch will probably introduce a WARN when the > ath9k driver tries to get the beacon without checking for CSA > completion.. > > Any other comments or a sanity check would also be appreciated. Thanks for checking back - I've read the part in the spec again [1], and appearently you are right. With your proposed change, shouldn't we also change the behaviour if the userspace requests a channel switch with count = 0? I guess we should immediately change the channel then without waiting for beacons? I don't see the point in changing the beacons if we don't send them, after all. Currently, ath9k calls the csa_finish() after checking whether beacon sending was complete ... so this would have to be changed. Cheers, Simon [1] IEEE 802.11-2012, 8.4.2.21 For nonmesh STAs, the Channel Switch Count field either is set to the number of TBTTs until the STA sending the Channel Switch Announcement element switches to the new channel or is set to 0. A value of 1 indicates that the switch occurs immediately before the next TBTT. A value of 0 indicates that the switch occurs at any time after the frame containing the element is transmitted. -- 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, 2013-11-04 at 14:36 +0100, Simon Wunderlich wrote: > Hey Luca, Hey Simon, Thanks for your quick reply. :) > > A beacon should never have a Channel Switch Announcement information > > element with a count of 0, because a count of 1 means switch just > > before the next beacon. So, if a count of 0 was valid in a beacon, it > > would have been transmitted in the next channel already, which is > > useless. A CSA count equal to zero is only meaningful in action > > frames or probe_responses. > > > > Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() > > functions accordingly. > > > > Cc: Simon Wunderlich <sw@simonwunderlich.de> > > Signed-off-by: Luciano Coelho <luciano.coelho@intel.com> > > --- > > > > Hi Simon (et al), > > > > I identified this issue while playing around with CSA. I noticed that > > we are sending a CSA beaon with count == 0, which should not happen. > > The last beacon visible in the current channel (ie. before the switch) > > contains a CSA IE with count == 1. > > > > I wanted to check with you if my proposed change would have any > > side-effects, especially with the ath9k driver, which is the only user > > of this code in the mainline at the moment. > > > > The potential danger here is if you don't check > > ieee80211_csa_is_complete() before you send the first CSA beacon out. > > With the previous code, there would always be a beacon with CSA (count > > == 0), but now, if the count starts with 1, there won't be any. If > > you don't check, my patch will probably introduce a WARN when the > > ath9k driver tries to get the beacon without checking for CSA > > completion.. > > > > Any other comments or a sanity check would also be appreciated. > > Thanks for checking back - I've read the part in the spec again [1], and > appearently you are right. > > With your proposed change, shouldn't we also change the behaviour if the > userspace requests a channel switch with count = 0? I guess we should > immediately change the channel then without waiting for beacons? I don't see > the point in changing the beacons if we don't send them, after all. You're right, changing the beacons doesn't make sense in this case. I'll change that and send v2. Another thing is that we are missing the action frames. The idea with the count == 0 is that the STA's should start listening on the other channel immediately after receiving the action frame (because the switch will happen at any time). If we don't send the action frame, passing context == 0 from the userspace doesn't much make sense, because the clients won't know we're switching. Well, maybe it makes a bit of sense, if there are no clients connected, but nevertheless. count == 0 should probably be avoided, because the specs are not that clear about it. In case transmission on our channel needs to be stopped immediately (eg. with DSS), we can always use the channel switch mode (ie. no TX until the switch happens). I'll probably start looking into the action frame implementation soon. > Currently, > ath9k calls the csa_finish() after checking whether beacon sending was complete > ... so this would have to be changed. Yeah, that's what I suspected from my very quick look at the ath9k code. I'll see if I can change this easily. I'll probably need some help with testing, because I don't have an ath9k device. :\ -- Cheers, Luca.
Hey Luca, > > > > Thanks for checking back - I've read the part in the spec again [1], and > > appearently you are right. > > > > With your proposed change, shouldn't we also change the behaviour if the > > userspace requests a channel switch with count = 0? I guess we should > > immediately change the channel then without waiting for beacons? I don't > > see the point in changing the beacons if we don't send them, after all. > > You're right, changing the beacons doesn't make sense in this case. > I'll change that and send v2. > > Another thing is that we are missing the action frames. The idea with > the count == 0 is that the STA's should start listening on the other > channel immediately after receiving the action frame (because the switch > will happen at any time). > > If we don't send the action frame, passing context == 0 from the > userspace doesn't much make sense, because the clients won't know we're > switching. Well, maybe it makes a bit of sense, if there are no clients > connected, but nevertheless. Yeah, switching without actionframe and count == 0 is pretty useless. > > count == 0 should probably be avoided, because the specs are not that > clear about it. In case transmission on our channel needs to be stopped > immediately (eg. with DSS), we can always use the channel switch mode > (ie. no TX until the switch happens). Hmm, we probably need to review the IBSS part too - we adopt the channel switch from others here, and adopting with count = 0 works now but would create a warning with your change ... > > I'll probably start looking into the action frame implementation soon. I've recently added CSA action frame support for the IBSS mode, because the distributed beaconing may lead to slow adoption of the CSA IEs in the IBSS. Check ieee80211_send_action_csa() for that. I guess it would be useful to send at least the action frames out if we want to change "fast" (count=0). For AP mode, I guess the right place to implement the action frames would be hostap? This would at least allow great flexibility with CSA, ECSA and all the new channel switch IEs coming now. The beacons are also generated in userspace, after all. BTW, I've just checked and the WiFi Alliance requires at least 5 beacons with CSA-IEs to pass the 802.11h test. :) > > > Currently, > > ath9k calls the csa_finish() after checking whether beacon sending was > > complete ... so this would have to be changed. > > Yeah, that's what I suspected from my very quick look at the ath9k code. > I'll see if I can change this easily. I'll probably need some help with > testing, because I don't have an ath9k device. :\ No problem, I can test here, just tell me what to test. :) Cheers, Simon -- 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
T24gTW9uLCAyMDEzLTExLTA0IGF0IDE1OjMxICswMTAwLCBTaW1vbiBXdW5kZXJsaWNoIHdyb3Rl Og0KPiA+ID4gVGhhbmtzIGZvciBjaGVja2luZyBiYWNrIC0gSSd2ZSByZWFkIHRoZSBwYXJ0IGlu IHRoZSBzcGVjIGFnYWluIFsxXSwgYW5kDQo+ID4gPiBhcHBlYXJlbnRseSB5b3UgYXJlIHJpZ2h0 Lg0KPiA+ID4gDQo+ID4gPiBXaXRoIHlvdXIgcHJvcG9zZWQgY2hhbmdlLCBzaG91bGRuJ3Qgd2Ug YWxzbyBjaGFuZ2UgdGhlIGJlaGF2aW91ciBpZiB0aGUNCj4gPiA+IHVzZXJzcGFjZSByZXF1ZXN0 cyBhIGNoYW5uZWwgc3dpdGNoIHdpdGggY291bnQgPSAwPyBJIGd1ZXNzIHdlIHNob3VsZA0KPiA+ ID4gaW1tZWRpYXRlbHkgY2hhbmdlIHRoZSBjaGFubmVsIHRoZW4gd2l0aG91dCB3YWl0aW5nIGZv ciBiZWFjb25zPyBJIGRvbid0DQo+ID4gPiBzZWUgdGhlIHBvaW50IGluIGNoYW5naW5nIHRoZSBi ZWFjb25zIGlmIHdlIGRvbid0IHNlbmQgdGhlbSwgYWZ0ZXIgYWxsLg0KPiA+IA0KPiA+IFlvdSdy ZSByaWdodCwgY2hhbmdpbmcgdGhlIGJlYWNvbnMgZG9lc24ndCBtYWtlIHNlbnNlIGluIHRoaXMg Y2FzZS4NCj4gPiBJJ2xsIGNoYW5nZSB0aGF0IGFuZCBzZW5kIHYyLg0KPiA+IA0KPiA+IEFub3Ro ZXIgdGhpbmcgaXMgdGhhdCB3ZSBhcmUgbWlzc2luZyB0aGUgYWN0aW9uIGZyYW1lcy4gIFRoZSBp ZGVhIHdpdGgNCj4gPiB0aGUgY291bnQgPT0gMCBpcyB0aGF0IHRoZSBTVEEncyBzaG91bGQgc3Rh cnQgbGlzdGVuaW5nIG9uIHRoZSBvdGhlcg0KPiA+IGNoYW5uZWwgaW1tZWRpYXRlbHkgYWZ0ZXIg cmVjZWl2aW5nIHRoZSBhY3Rpb24gZnJhbWUgKGJlY2F1c2UgdGhlIHN3aXRjaA0KPiA+IHdpbGwg aGFwcGVuIGF0IGFueSB0aW1lKS4NCj4gPiANCj4gPiBJZiB3ZSBkb24ndCBzZW5kIHRoZSBhY3Rp b24gZnJhbWUsIHBhc3NpbmcgY29udGV4dCA9PSAwIGZyb20gdGhlDQo+ID4gdXNlcnNwYWNlIGRv ZXNuJ3QgbXVjaCBtYWtlIHNlbnNlLCBiZWNhdXNlIHRoZSBjbGllbnRzIHdvbid0IGtub3cgd2Un cmUNCj4gPiBzd2l0Y2hpbmcuICBXZWxsLCBtYXliZSBpdCBtYWtlcyBhIGJpdCBvZiBzZW5zZSwg aWYgdGhlcmUgYXJlIG5vIGNsaWVudHMNCj4gPiBjb25uZWN0ZWQsIGJ1dCBuZXZlcnRoZWxlc3Mu DQo+IA0KPiBZZWFoLCBzd2l0Y2hpbmcgd2l0aG91dCBhY3Rpb25mcmFtZSBhbmQgY291bnQgPT0g MCBpcyBwcmV0dHkgdXNlbGVzcy4NCg0KQWN0dWFsbHksIGlmIHRoZSB1c2Vyc3BhY2UgcmVxdWVz dHMgY291bnQgPT0gMSwgd2Ugd29uJ3QgaGF2ZSBhbnkNCmJlYWNvbnMgZWl0aGVyLCBiZWNhdXNl IDEgbWVhbnMgImp1c3QgYmVmb3JlIHRoZSBuZXh0IFRCVFQiLiAgU28gZm9yDQpjb3VudCA9PSAx IChjb21pbmcgZnJvbSB0aGUgdXNlcnNwYWNlKSB3ZSBzaG91bGRuJ3QgY29uZmlndXJlIHRoZQ0K YmVhY29uLCBzaW5jZSB3ZSB3b24ndCBzZW5kIGl0LiAgV2UgbmVlZCB0aGUgYWN0aW9uIGZyYW1l IGZvciB0aGlzIGNhc2UNCnRvby4NCg0KDQo+ID4gY291bnQgPT0gMCBzaG91bGQgcHJvYmFibHkg YmUgYXZvaWRlZCwgYmVjYXVzZSB0aGUgc3BlY3MgYXJlIG5vdCB0aGF0DQo+ID4gY2xlYXIgYWJv dXQgaXQuICBJbiBjYXNlIHRyYW5zbWlzc2lvbiBvbiBvdXIgY2hhbm5lbCBuZWVkcyB0byBiZSBz dG9wcGVkDQo+ID4gaW1tZWRpYXRlbHkgKGVnLiB3aXRoIERTUyksIHdlIGNhbiBhbHdheXMgdXNl IHRoZSBjaGFubmVsIHN3aXRjaCBtb2RlDQo+ID4gKGllLiBubyBUWCB1bnRpbCB0aGUgc3dpdGNo IGhhcHBlbnMpLg0KPiANCj4gSG1tLCB3ZSBwcm9iYWJseSBuZWVkIHRvIHJldmlldyB0aGUgSUJT UyBwYXJ0IHRvbyAtIHdlIGFkb3B0IHRoZSBjaGFubmVsIA0KPiBzd2l0Y2ggZnJvbSBvdGhlcnMg aGVyZSwgYW5kIGFkb3B0aW5nIHdpdGggY291bnQgPSAwIHdvcmtzIG5vdyBidXQgd291bGQgDQo+ IGNyZWF0ZSBhIHdhcm5pbmcgd2l0aCB5b3VyIGNoYW5nZSAuLi4NCg0KT2theSwgSSdsbCBjaGVj ayBJQlNTIHRvby4NCg0KDQo+ID4gSSdsbCBwcm9iYWJseSBzdGFydCBsb29raW5nIGludG8gdGhl IGFjdGlvbiBmcmFtZSBpbXBsZW1lbnRhdGlvbiBzb29uLg0KPiANCj4gSSd2ZSByZWNlbnRseSBh ZGRlZCBDU0EgYWN0aW9uIGZyYW1lIHN1cHBvcnQgZm9yIHRoZSBJQlNTIG1vZGUsIGJlY2F1c2Ug dGhlIA0KPiBkaXN0cmlidXRlZCBiZWFjb25pbmcgbWF5IGxlYWQgdG8gc2xvdyBhZG9wdGlvbiBv ZiB0aGUgQ1NBIElFcyBpbiB0aGUgSUJTUy4gDQo+IENoZWNrIGllZWU4MDIxMV9zZW5kX2FjdGlv bl9jc2EoKSBmb3IgdGhhdC4NCg0KQ29vbCwgSSBoYWQgbWlzc2VkIHRoYXQuICBJIGFkbWl0IEkg ZGlkbid0IGxvb2sgaW50byBJQlNTIG11Y2guICBUaGF0DQp3YXMgZ29pbmcgdG8gYmUgbXkgbmV4 dCBzdGVwLiAoSXNuJ3QgdGhhdCBhbHdheXMgYSBnb29kIGV4Y3VzZSA7KQ0KDQoNCj4gIEkgZ3Vl c3MgaXQgd291bGQgYmUgdXNlZnVsIHRvIHNlbmQgDQo+IGF0IGxlYXN0IHRoZSBhY3Rpb24gZnJh bWVzIG91dCBpZiB3ZSB3YW50IHRvIGNoYW5nZSAiZmFzdCIgKGNvdW50PTApLg0KDQpPciBjb3Vu dCA9PSAxLiA6KQ0KDQoNCj4gRm9yIEFQIG1vZGUsIEkgZ3Vlc3MgdGhlIHJpZ2h0IHBsYWNlIHRv IGltcGxlbWVudCB0aGUgYWN0aW9uIGZyYW1lcyB3b3VsZCBiZSANCj4gaG9zdGFwPyBUaGlzIHdv dWxkIGF0IGxlYXN0IGFsbG93IGdyZWF0IGZsZXhpYmlsaXR5IHdpdGggQ1NBLCBFQ1NBIGFuZCBh bGwgdGhlIA0KPiBuZXcgY2hhbm5lbCBzd2l0Y2ggSUVzIGNvbWluZyBub3cuIFRoZSBiZWFjb25z IGFyZSBhbHNvIGdlbmVyYXRlZCBpbiANCj4gdXNlcnNwYWNlLCBhZnRlciBhbGwuDQoNCldoYXQg bmV3IGNoYW5uZWwgY2hhbm5lbCBzd2l0Y2ggSUVzPw0KDQpZb3UncmUgcmlnaHQgdGhhdCBpdCBt aWdodCBtYWtlIHNlbnNlIHRvIGltcGxlbWVudCB0aGUgYWN0aW9uIGZyYW1lcyBpbg0KaG9zdGFw LiAgQnV0IE9UT0gsIHRoZSBhY3Rpb24gZnJhbWUgaXMgcXVpdGUgc2ltcGxlIGFuZCBtYWM4MDIx MSBzaG91bGQNCmhhdmUgYWxsIHRoZSBpbmZvcm1hdGlvbiBuZWVkZWQgdG8gc2VuZCBpdCBvdXQu DQoNCg0KPiBCVFcsIEkndmUganVzdCBjaGVja2VkIGFuZCB0aGUgV2lGaSBBbGxpYW5jZSByZXF1 aXJlcyBhdCBsZWFzdCA1IGJlYWNvbnMgd2l0aCANCj4gQ1NBLUlFcyB0byBwYXNzIHRoZSA4MDIu MTFoIHRlc3QuIDopDQoNCkRvIHlvdSBtZWFuIHRoYXQgdGhlIHRlc3RzIG9ubHkgY2hlY2sgd2hl biBjb3VudCBzdGFydHMgYXMgPiA1Pw0KDQoNCj4gPiA+IEN1cnJlbnRseSwNCj4gPiA+IGF0aDlr IGNhbGxzIHRoZSBjc2FfZmluaXNoKCkgYWZ0ZXIgY2hlY2tpbmcgd2hldGhlciBiZWFjb24gc2Vu ZGluZyB3YXMNCj4gPiA+IGNvbXBsZXRlIC4uLiBzbyB0aGlzIHdvdWxkIGhhdmUgdG8gYmUgY2hh bmdlZC4NCj4gPiANCj4gPiBZZWFoLCB0aGF0J3Mgd2hhdCBJIHN1c3BlY3RlZCBmcm9tIG15IHZl cnkgcXVpY2sgbG9vayBhdCB0aGUgYXRoOWsgY29kZS4NCj4gPiBJJ2xsIHNlZSBpZiBJIGNhbiBj aGFuZ2UgdGhpcyBlYXNpbHkuICBJJ2xsIHByb2JhYmx5IG5lZWQgc29tZSBoZWxwIHdpdGgNCj4g PiB0ZXN0aW5nLCBiZWNhdXNlIEkgZG9uJ3QgaGF2ZSBhbiBhdGg5ayBkZXZpY2UuIDpcDQo+IA0K PiBObyBwcm9ibGVtLCBJIGNhbiB0ZXN0IGhlcmUsIGp1c3QgdGVsbCBtZSB3aGF0IHRvIHRlc3Qu IDopDQoNCkNvb2whIEJhc2ljYWxseSBJIGp1c3Qgd2FudCB5b3UgdG8gbWFrZSBzdXJlIHdoYXQg SSBkbyBzdGlsbCB3b3JrcyBhbmQgSQ0KZG9uJ3QgYnJlYWsgYW55dGhpbmcgb2Ygd2hhdCB5b3Ug aGF2ZSBiZWVuIGRvaW5nLiA7KQ0KDQotLQ0KQ2hlZXJzLA0KTHVjYS4NCg0K -- 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 Tue, 2013-11-05 at 10:27 +0200, Luciano Coelho wrote: > On Mon, 2013-11-04 at 15:31 +0100, Simon Wunderlich wrote: > > > > Thanks for checking back - I've read the part in the spec again [1], and > > > > appearently you are right. > > > > > > > > With your proposed change, shouldn't we also change the behaviour if the > > > > userspace requests a channel switch with count = 0? I guess we should > > > > immediately change the channel then without waiting for beacons? I don't > > > > see the point in changing the beacons if we don't send them, after all. > > > > > > You're right, changing the beacons doesn't make sense in this case. > > > I'll change that and send v2. > > > > > > Another thing is that we are missing the action frames. The idea with > > > the count == 0 is that the STA's should start listening on the other > > > channel immediately after receiving the action frame (because the switch > > > will happen at any time). > > > > > > If we don't send the action frame, passing context == 0 from the > > > userspace doesn't much make sense, because the clients won't know we're > > > switching. Well, maybe it makes a bit of sense, if there are no clients > > > connected, but nevertheless. > > > > Yeah, switching without actionframe and count == 0 is pretty useless. > > Actually, if the userspace requests count == 1, we won't have any > beacons either, because 1 means "just before the next TBTT". So for > count == 1 (coming from the userspace) we shouldn't configure the > beacon, since we won't send it. We need the action frame for this case > too. Hmmm... Now there's something that is a bit unclear to me in the specs: "For nonmesh STAs, the Channel Switch Count field either is set to the number of TBTTs until the STA sending the Channel Switch Announcement element switches to the new channel or is set to 0." There's nothing specifying exactly when, relative to the beacon, the switch actually happens. Just before? Just after? Is the beacon that matches that TBTT transmitted in the current or in the next channel? For a value of 1, it's pretty clear: "A value of 1 indicates that the switch occurs immediately before the next TBTT." I don't think we're doing that now. At least in the ath9k case, you switch the channel immediately after the beacon with count == 1, by calling ieee80211_csa_finish(). The correct would be just before the next beacon is sent. A value of 0 is also unclear, because it doesn't refer to any TBTTs at all. So if you read it literally, the following would mean that it could at *any* time in the future: "A value of 0 indicates that the switch occurs at any time after the frame containing the element is transmitted." Am I missing something? What do you think? -- Cheers, Luca.
> On Mon, 2013-11-04 at 15:31 +0100, Simon Wunderlich wrote: > > > > Thanks for checking back - I've read the part in the spec again [1], > > > > and appearently you are right. > > > > > > > > With your proposed change, shouldn't we also change the behaviour if > > > > the userspace requests a channel switch with count = 0? I guess we > > > > should immediately change the channel then without waiting for > > > > beacons? I don't see the point in changing the beacons if we don't > > > > send them, after all. > > > > > > You're right, changing the beacons doesn't make sense in this case. > > > I'll change that and send v2. > > > > > > Another thing is that we are missing the action frames. The idea with > > > the count == 0 is that the STA's should start listening on the other > > > channel immediately after receiving the action frame (because the > > > switch will happen at any time). > > > > > > If we don't send the action frame, passing context == 0 from the > > > userspace doesn't much make sense, because the clients won't know we're > > > switching. Well, maybe it makes a bit of sense, if there are no > > > clients connected, but nevertheless. > > > > Yeah, switching without actionframe and count == 0 is pretty useless. > > Actually, if the userspace requests count == 1, we won't have any > beacons either, because 1 means "just before the next TBTT". So for > count == 1 (coming from the userspace) we shouldn't configure the > beacon, since we won't send it. We need the action frame for this case > too. > Hmm, right, we will decrement the counter before sending it out ... > > For AP mode, I guess the right place to implement the action frames would > > be hostap? This would at least allow great flexibility with CSA, ECSA > > and all the new channel switch IEs coming now. The beacons are also > > generated in userspace, after all. > > What new channel channel switch IEs? Right now we also have extendended channel switch announcements (ECSA), and secondary channel offset, and there are more to come with 802.11ac I think. I have not studied it yet (and I don't have access to 802.11 drafts), but have a look at that: https://mentor.ieee.org/802.11/dcn/13/11-13-0105-00-00ac-lb190-proposed- resolution-on-cid-7367-and-7368.docx&ei=T_N4UuHWGcmt4ATQj4DwBg&usg=AFQjCNE5E- bpqRGQM7-QwG0L4TiU3OOLig&bvm=bv.55980276,d.bGE&cad=rja (not only the .doc format is ugly) > > You're right that it might make sense to implement the action frames in > hostap. But OTOH, the action frame is quite simple and mac80211 should > have all the information needed to send it out. > > > BTW, I've just checked and the WiFi Alliance requires at least 5 beacons > > with CSA-IEs to pass the 802.11h test. :) > > Do you mean that the tests only check when count starts as > 5? > It checks if the last beacon hast a CSA IE in the beacon, and also if there are 4 beacons before that including a CSA IE. It does not check for the count though, but that's implicitly given ... Cheers, Simon -- 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 Tue, 2013-11-05 at 10:27 +0200, Luciano Coelho wrote: > > On Mon, 2013-11-04 at 15:31 +0100, Simon Wunderlich wrote: > > > > > Thanks for checking back - I've read the part in the spec again > > > > > [1], and appearently you are right. > > > > > > > > > > With your proposed change, shouldn't we also change the behaviour > > > > > if the userspace requests a channel switch with count = 0? I guess > > > > > we should immediately change the channel then without waiting for > > > > > beacons? I don't see the point in changing the beacons if we don't > > > > > send them, after all. > > > > > > > > You're right, changing the beacons doesn't make sense in this case. > > > > I'll change that and send v2. > > > > > > > > Another thing is that we are missing the action frames. The idea > > > > with the count == 0 is that the STA's should start listening on the > > > > other channel immediately after receiving the action frame (because > > > > the switch will happen at any time). > > > > > > > > If we don't send the action frame, passing context == 0 from the > > > > userspace doesn't much make sense, because the clients won't know > > > > we're switching. Well, maybe it makes a bit of sense, if there are > > > > no clients connected, but nevertheless. > > > > > > Yeah, switching without actionframe and count == 0 is pretty useless. > > > > Actually, if the userspace requests count == 1, we won't have any > > beacons either, because 1 means "just before the next TBTT". So for > > count == 1 (coming from the userspace) we shouldn't configure the > > beacon, since we won't send it. We need the action frame for this case > > too. > > Hmmm... Now there's something that is a bit unclear to me in the specs: > > "For nonmesh STAs, the Channel Switch Count field either is set to the > number of TBTTs until the STA sending the Channel Switch Announcement > element switches to the new channel or is set to 0." > > There's nothing specifying exactly when, relative to the beacon, the > switch actually happens. Just before? Just after? Is the beacon that > matches that TBTT transmitted in the current or in the next channel? > > For a value of 1, it's pretty clear: > > "A value of 1 indicates that the switch occurs immediately before the > next TBTT." If it says for 1 "just before the next TBTT", then this would mean the next beacon is on the next channel. I'd interpret then that for count=0, the channel switch happens as soon as possible, and may happen any time before the next TBTT. > > I don't think we're doing that now. At least in the ath9k case, you > switch the channel immediately after the beacon with count == 1, by > calling ieee80211_csa_finish(). The correct would be just before the > next beacon is sent. Hm, I think you are right, although I'm not sure how easy that would be implementation-wise. BTW, for that case I think we also have to fix the client side, which is currently switching immediately for count = 1 and not waiting for the next TBTT. (see end of ieee80211_sta_process_chanswitch(), the rest appears correct though). > > A value of 0 is also unclear, because it doesn't refer to any TBTTs at > all. So if you read it literally, the following would mean that it > could at *any* time in the future: > > "A value of 0 indicates that the switch occurs at any time after the > frame containing the element is transmitted." > > Am I missing something? What do you think? As said above, I'd interpret it as the change should happen as soon as possible. If count = 1 means the change will happen "immediately before the next TBTT", I would guess 0 would mean even before that, or at least not after the "before the next TBTT". I agree that the spec is not perfectly clear about that, but I currently don't see any other reasonable interpretation here ... Cheers, Simon -- 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
T24gVHVlLCAyMDEzLTExLTA1IGF0IDE0OjM2ICswMTAwLCBTaW1vbiBXdW5kZXJsaWNoIHdyb3Rl Og0KPiA+IE9uIE1vbiwgMjAxMy0xMS0wNCBhdCAxNTozMSArMDEwMCwgU2ltb24gV3VuZGVybGlj aCB3cm90ZToNCj4gPiA+ID4gPiBUaGFua3MgZm9yIGNoZWNraW5nIGJhY2sgLSBJJ3ZlIHJlYWQg dGhlIHBhcnQgaW4gdGhlIHNwZWMgYWdhaW4gWzFdLA0KPiA+ID4gPiA+IGFuZCBhcHBlYXJlbnRs eSB5b3UgYXJlIHJpZ2h0Lg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IFdpdGggeW91ciBwcm9wb3Nl ZCBjaGFuZ2UsIHNob3VsZG4ndCB3ZSBhbHNvIGNoYW5nZSB0aGUgYmVoYXZpb3VyIGlmDQo+ID4g PiA+ID4gdGhlIHVzZXJzcGFjZSByZXF1ZXN0cyBhIGNoYW5uZWwgc3dpdGNoIHdpdGggY291bnQg PSAwPyBJIGd1ZXNzIHdlDQo+ID4gPiA+ID4gc2hvdWxkIGltbWVkaWF0ZWx5IGNoYW5nZSB0aGUg Y2hhbm5lbCB0aGVuIHdpdGhvdXQgd2FpdGluZyBmb3INCj4gPiA+ID4gPiBiZWFjb25zPyBJIGRv bid0IHNlZSB0aGUgcG9pbnQgaW4gY2hhbmdpbmcgdGhlIGJlYWNvbnMgaWYgd2UgZG9uJ3QNCj4g PiA+ID4gPiBzZW5kIHRoZW0sIGFmdGVyIGFsbC4NCj4gPiA+ID4gDQo+ID4gPiA+IFlvdSdyZSBy aWdodCwgY2hhbmdpbmcgdGhlIGJlYWNvbnMgZG9lc24ndCBtYWtlIHNlbnNlIGluIHRoaXMgY2Fz ZS4NCj4gPiA+ID4gSSdsbCBjaGFuZ2UgdGhhdCBhbmQgc2VuZCB2Mi4NCj4gPiA+ID4gDQo+ID4g PiA+IEFub3RoZXIgdGhpbmcgaXMgdGhhdCB3ZSBhcmUgbWlzc2luZyB0aGUgYWN0aW9uIGZyYW1l cy4gIFRoZSBpZGVhIHdpdGgNCj4gPiA+ID4gdGhlIGNvdW50ID09IDAgaXMgdGhhdCB0aGUgU1RB J3Mgc2hvdWxkIHN0YXJ0IGxpc3RlbmluZyBvbiB0aGUgb3RoZXINCj4gPiA+ID4gY2hhbm5lbCBp bW1lZGlhdGVseSBhZnRlciByZWNlaXZpbmcgdGhlIGFjdGlvbiBmcmFtZSAoYmVjYXVzZSB0aGUN Cj4gPiA+ID4gc3dpdGNoIHdpbGwgaGFwcGVuIGF0IGFueSB0aW1lKS4NCj4gPiA+ID4gDQo+ID4g PiA+IElmIHdlIGRvbid0IHNlbmQgdGhlIGFjdGlvbiBmcmFtZSwgcGFzc2luZyBjb250ZXh0ID09 IDAgZnJvbSB0aGUNCj4gPiA+ID4gdXNlcnNwYWNlIGRvZXNuJ3QgbXVjaCBtYWtlIHNlbnNlLCBi ZWNhdXNlIHRoZSBjbGllbnRzIHdvbid0IGtub3cgd2UncmUNCj4gPiA+ID4gc3dpdGNoaW5nLiAg V2VsbCwgbWF5YmUgaXQgbWFrZXMgYSBiaXQgb2Ygc2Vuc2UsIGlmIHRoZXJlIGFyZSBubw0KPiA+ ID4gPiBjbGllbnRzIGNvbm5lY3RlZCwgYnV0IG5ldmVydGhlbGVzcy4NCj4gPiA+IA0KPiA+ID4g WWVhaCwgc3dpdGNoaW5nIHdpdGhvdXQgYWN0aW9uZnJhbWUgYW5kIGNvdW50ID09IDAgaXMgcHJl dHR5IHVzZWxlc3MuDQo+ID4gDQo+ID4gQWN0dWFsbHksIGlmIHRoZSB1c2Vyc3BhY2UgcmVxdWVz dHMgY291bnQgPT0gMSwgd2Ugd29uJ3QgaGF2ZSBhbnkNCj4gPiBiZWFjb25zIGVpdGhlciwgYmVj YXVzZSAxIG1lYW5zICJqdXN0IGJlZm9yZSB0aGUgbmV4dCBUQlRUIi4gIFNvIGZvcg0KPiA+IGNv dW50ID09IDEgKGNvbWluZyBmcm9tIHRoZSB1c2Vyc3BhY2UpIHdlIHNob3VsZG4ndCBjb25maWd1 cmUgdGhlDQo+ID4gYmVhY29uLCBzaW5jZSB3ZSB3b24ndCBzZW5kIGl0LiAgV2UgbmVlZCB0aGUg YWN0aW9uIGZyYW1lIGZvciB0aGlzIGNhc2UNCj4gPiB0b28uDQo+ID4gDQo+IA0KPiBIbW0sIHJp Z2h0LCB3ZSB3aWxsIGRlY3JlbWVudCB0aGUgY291bnRlciBiZWZvcmUgc2VuZGluZyBpdCBvdXQg Li4uDQoNCkkgdGhpbmsgdGhpcyB3b24ndCBiZSBhIHByb2JsZW0gYW55bW9yZSB3aGVuIEkgY2hh bmdlIHRoZSBjb2RlIHRvIHN3aXRjaA0KY2hhbm5lbCBpbW1lZGlhdGVseSAod2l0aG91dCBzZXR0 aW5nIHRoZSBiZWFjb25zKSBpbiBjYXNlIG9mIGNvdW50ID09IDANCmFuZCBjb3VudCA9PSAxLg0K DQoNCj4gPiA+IEZvciBBUCBtb2RlLCBJIGd1ZXNzIHRoZSByaWdodCBwbGFjZSB0byBpbXBsZW1l bnQgdGhlIGFjdGlvbiBmcmFtZXMgd291bGQNCj4gPiA+IGJlIGhvc3RhcD8gVGhpcyB3b3VsZCBh dCBsZWFzdCBhbGxvdyBncmVhdCBmbGV4aWJpbGl0eSB3aXRoIENTQSwgRUNTQQ0KPiA+ID4gYW5k IGFsbCB0aGUgbmV3IGNoYW5uZWwgc3dpdGNoIElFcyBjb21pbmcgbm93LiBUaGUgYmVhY29ucyBh cmUgYWxzbw0KPiA+ID4gZ2VuZXJhdGVkIGluIHVzZXJzcGFjZSwgYWZ0ZXIgYWxsLg0KPiA+IA0K PiA+IFdoYXQgbmV3IGNoYW5uZWwgY2hhbm5lbCBzd2l0Y2ggSUVzPw0KPiANCj4gUmlnaHQgbm93 IHdlIGFsc28gaGF2ZSBleHRlbmRlbmRlZCBjaGFubmVsIHN3aXRjaCBhbm5vdW5jZW1lbnRzIChF Q1NBKSwgYW5kIA0KPiBzZWNvbmRhcnkgY2hhbm5lbCBvZmZzZXQsIGFuZCB0aGVyZSBhcmUgbW9y ZSB0byBjb21lIHdpdGggODAyLjExYWMgSSB0aGluay4gSSANCj4gaGF2ZSBub3Qgc3R1ZGllZCBp dCB5ZXQgKGFuZCBJIGRvbid0IGhhdmUgYWNjZXNzIHRvIDgwMi4xMSBkcmFmdHMpLCBidXQgaGF2 ZSBhIA0KPiBsb29rIGF0IHRoYXQ6DQo+IA0KPiBodHRwczovL21lbnRvci5pZWVlLm9yZy84MDIu MTEvZGNuLzEzLzExLTEzLTAxMDUtMDAtMDBhYy1sYjE5MC1wcm9wb3NlZC0NCj4gcmVzb2x1dGlv bi1vbi1jaWQtNzM2Ny1hbmQtNzM2OC5kb2N4JmVpPVRfTjRVdUhXR2NtdDRBVFFqNER3QmcmdXNn PUFGUWpDTkU1RS0NCj4gYnBxUkdRTTctUXdHMEw0VGlVM09PTGlnJmJ2bT1idi41NTk4MDI3Nixk LmJHRSZjYWQ9cmphDQo+IA0KPiAobm90IG9ubHkgdGhlIC5kb2MgZm9ybWF0IGlzIHVnbHkpDQoN ClllcywgeW91J3JlIHJpZ2h0LiAgSW4gMTFhYyB0aGVyZSBhcmUgYSBmZXcgbW9yZSBlbGVtZW50 cyB0aGF0IHNob3VsZCBnbw0KaW4gdGhlIENTQSBhY3Rpb24gZnJhbWUuICBBbmQgc29tZSB3cmFw cGVycyBhbmQgb3RoZXIgdGhpbmdzIHRoYXQgZ28gaW4NCnRoZSBiZWFjb25zIGFuZCBwcm9iZV9y ZXNwcyBkdXJpbmcgQ1NBLg0KDQpCdXQgc3RpbGwsIGZvciB0aGUgYWN0aW9uIGZyYW1lcywgdGhl c2UgYXJlIHJhdGhlciBzdGF0aWMgYW5kIEkgZG9uJ3QNCnRoaW5rIGl0J3MgcmVhbGx5IGJhZCB0 byBkbyBpdCBpbiBtYWM4MDIxMSBpdHNlbGYuICBCdXQgSSBkb24ndCBoYXZlIGENCnN0cm9uZyBv cGluaW9uIHJlZ2FyZGluZyB0aGlzLi4uDQoNCg0KPiA+IFlvdSdyZSByaWdodCB0aGF0IGl0IG1p Z2h0IG1ha2Ugc2Vuc2UgdG8gaW1wbGVtZW50IHRoZSBhY3Rpb24gZnJhbWVzIGluDQo+ID4gaG9z dGFwLiAgQnV0IE9UT0gsIHRoZSBhY3Rpb24gZnJhbWUgaXMgcXVpdGUgc2ltcGxlIGFuZCBtYWM4 MDIxMSBzaG91bGQNCj4gPiBoYXZlIGFsbCB0aGUgaW5mb3JtYXRpb24gbmVlZGVkIHRvIHNlbmQg aXQgb3V0Lg0KPiA+IA0KPiA+ID4gQlRXLCBJJ3ZlIGp1c3QgY2hlY2tlZCBhbmQgdGhlIFdpRmkg QWxsaWFuY2UgcmVxdWlyZXMgYXQgbGVhc3QgNSBiZWFjb25zDQo+ID4gPiB3aXRoIENTQS1JRXMg dG8gcGFzcyB0aGUgODAyLjExaCB0ZXN0LiA6KQ0KPiA+IA0KPiA+IERvIHlvdSBtZWFuIHRoYXQg dGhlIHRlc3RzIG9ubHkgY2hlY2sgd2hlbiBjb3VudCBzdGFydHMgYXMgPiA1Pw0KPiA+IA0KPiAN Cj4gSXQgY2hlY2tzIGlmIHRoZSBsYXN0IGJlYWNvbiBoYXN0IGEgQ1NBIElFIGluIHRoZSBiZWFj b24sIGFuZCBhbHNvIGlmIHRoZXJlIA0KPiBhcmUgNCBiZWFjb25zIGJlZm9yZSB0aGF0IGluY2x1 ZGluZyBhIENTQSBJRS4gSXQgZG9lcyBub3QgY2hlY2sgZm9yIHRoZSBjb3VudCANCj4gdGhvdWdo LCBidXQgdGhhdCdzIGltcGxpY2l0bHkgZ2l2ZW4gLi4uDQoNCk9rYXksIHNvIHdlIGp1c3QgbmVl ZCB0byByZW1lbWJlciB0byB1c2UgY291bnQgPj0gNiB3aGVuIHRlc3RpbmcuIDspDQoNCi0tDQpM dWNhLg0K -- 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 Tue, 2013-11-05 at 15:00 +0100, Simon Wunderlich wrote: > > On Tue, 2013-11-05 at 10:27 +0200, Luciano Coelho wrote: > > > On Mon, 2013-11-04 at 15:31 +0100, Simon Wunderlich wrote: > > > > > > Thanks for checking back - I've read the part in the spec again > > > > > > [1], and appearently you are right. > > > > > > > > > > > > With your proposed change, shouldn't we also change the behaviour > > > > > > if the userspace requests a channel switch with count = 0? I guess > > > > > > we should immediately change the channel then without waiting for > > > > > > beacons? I don't see the point in changing the beacons if we don't > > > > > > send them, after all. > > > > > > > > > > You're right, changing the beacons doesn't make sense in this case. > > > > > I'll change that and send v2. > > > > > > > > > > Another thing is that we are missing the action frames. The idea > > > > > with the count == 0 is that the STA's should start listening on the > > > > > other channel immediately after receiving the action frame (because > > > > > the switch will happen at any time). > > > > > > > > > > If we don't send the action frame, passing context == 0 from the > > > > > userspace doesn't much make sense, because the clients won't know > > > > > we're switching. Well, maybe it makes a bit of sense, if there are > > > > > no clients connected, but nevertheless. > > > > > > > > Yeah, switching without actionframe and count == 0 is pretty useless. > > > > > > Actually, if the userspace requests count == 1, we won't have any > > > beacons either, because 1 means "just before the next TBTT". So for > > > count == 1 (coming from the userspace) we shouldn't configure the > > > beacon, since we won't send it. We need the action frame for this case > > > too. > > > > Hmmm... Now there's something that is a bit unclear to me in the specs: > > > > "For nonmesh STAs, the Channel Switch Count field either is set to the > > number of TBTTs until the STA sending the Channel Switch Announcement > > element switches to the new channel or is set to 0." > > > > There's nothing specifying exactly when, relative to the beacon, the > > switch actually happens. Just before? Just after? Is the beacon that > > matches that TBTT transmitted in the current or in the next channel? > > > > For a value of 1, it's pretty clear: > > > > "A value of 1 indicates that the switch occurs immediately before the > > next TBTT." > > If it says for 1 "just before the next TBTT", then this would mean the next > beacon is on the next channel. I'd interpret then that for count=0, the > channel switch happens as soon as possible, and may happen any time before the > next TBTT. It says "just before the next TBTT" for value == 1, not for values > 1. ;) But yeah, my interpretation is the same as yours. We can extrapolate this to when value > 1 as well. > > I don't think we're doing that now. At least in the ath9k case, you > > switch the channel immediately after the beacon with count == 1, by > > calling ieee80211_csa_finish(). The correct would be just before the > > next beacon is sent. > > Hm, I think you are right, although I'm not sure how easy that would be > implementation-wise. Yes, I guess this would be pretty hard to do if we have beacon offload... > BTW, for that case I think we also have to fix the client side, which is > currently switching immediately for count = 1 and not waiting for the next > TBTT. (see end of ieee80211_sta_process_chanswitch(), the rest appears correct > though). Yes, I have seen that too. The problem is that this "immediately before" is not well specified either. It depends on many things, like how long it takes the hardware to physically switch to the next channel and start beaconing. I guess the only problem would be a hick up in the connection, if the AP switches at a different time than the STA does... > > A value of 0 is also unclear, because it doesn't refer to any TBTTs at > > all. So if you read it literally, the following would mean that it > > could at *any* time in the future: > > > > "A value of 0 indicates that the switch occurs at any time after the > > frame containing the element is transmitted." > > > > Am I missing something? What do you think? > > As said above, I'd interpret it as the change should happen as soon as > possible. If count = 1 means the change will happen "immediately before the > next TBTT", I would guess 0 would mean even before that, or at least not after > the "before the next TBTT". > > I agree that the spec is not perfectly clear about that, but I currently don't > see any other reasonable interpretation here ... Yes, I agree with your interpretations too. I was just being too literal. ;) In any case, we can only hope everyone interprets it in the same way... -- Cheers, Luca.
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 9993fcb..1e0d40f 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2376,8 +2376,12 @@ static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata, if (WARN_ON(counter_offset_beacon >= beacon_data_len)) return; - /* warn if the driver did not check for/react to csa completeness */ - if (WARN_ON(beacon_data[counter_offset_beacon] == 0)) + /* Warn if the driver did not check for/react to csa + * completeness. A beacon with CSA counter set to 0 should + * never occur, because a counter of 1 means switch just + * before the next beacon. + */ + if (WARN_ON(beacon_data[counter_offset_beacon] == 1)) return; beacon_data[counter_offset_beacon]--; @@ -2434,7 +2438,7 @@ bool ieee80211_csa_is_complete(struct ieee80211_vif *vif) if (WARN_ON(counter_beacon > beacon_data_len)) goto out; - if (beacon_data[counter_beacon] == 0) + if (beacon_data[counter_beacon] == 1) ret = true; out: rcu_read_unlock();
A beacon should never have a Channel Switch Announcement information element with a count of 0, because a count of 1 means switch just before the next beacon. So, if a count of 0 was valid in a beacon, it would have been transmitted in the next channel already, which is useless. A CSA count equal to zero is only meaningful in action frames or probe_responses. Fix the ieee80211_csa_is_complete() and ieee80211_update_csa() functions accordingly. Cc: Simon Wunderlich <sw@simonwunderlich.de> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com> --- Hi Simon (et al), I identified this issue while playing around with CSA. I noticed that we are sending a CSA beaon with count == 0, which should not happen. The last beacon visible in the current channel (ie. before the switch) contains a CSA IE with count == 1. I wanted to check with you if my proposed change would have any side-effects, especially with the ath9k driver, which is the only user of this code in the mainline at the moment. The potential danger here is if you don't check ieee80211_csa_is_complete() before you send the first CSA beacon out. With the previous code, there would always be a beacon with CSA (count == 0), but now, if the count starts with 1, there won't be any. If you don't check, my patch will probably introduce a WARN when the ath9k driver tries to get the beacon without checking for CSA completion.. Any other comments or a sanity check would also be appreciated. -- Cheers, Luca. net/mac80211/tx.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)