[RFC v2 1/2] mac80211: don't transmit beacon with CSA count 0

[RFC v2 1/2] mac80211: don't transmit beacon with CSA count 0

[Luciano Coelho]

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>
---
In v2, updated the documentation to reflect the change.
---
 include/net/mac80211.h | 10 ++++++----
 net/mac80211/tx.c      | 10 +++++++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index f386c48..2d0c6ab 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2676,11 +2676,13 @@ enum ieee80211_roc_type {
  * @channel_switch_beacon: Starts a channel switch to a new channel.
  *	Beacons are modified to include CSA or ECSA IEs before calling this
  *	function. The corresponding count fields in these IEs must be
- *	decremented, and when they reach zero the driver must call
+ *	decremented, and when they reach 1 the driver must call
  *	ieee80211_csa_finish(). Drivers which use ieee80211_beacon_get()
  *	get the csa counter decremented by mac80211, but must check if it is
- *	zero using ieee80211_csa_is_complete() after the beacon has been
+ *	1 using ieee80211_csa_is_complete() after the beacon has been
  *	transmitted and then call ieee80211_csa_finish().
+ *	If the CSA count starts as zero or 1, this function will not be called,
+ *	since there won't be any time to beacon before the switch anyway.
  *
  * @join_ibss: Join an IBSS (on an IBSS interface); this is called after all
  *	information in bss_conf is set up and the beacon can be retrieved. A
@@ -3379,13 +3381,13 @@ static inline struct sk_buff *ieee80211_beacon_get(struct ieee80211_hw *hw,
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  *
  * After a channel switch announcement was scheduled and the counter in this
- * announcement hit zero, this function must be called by the driver to
+ * announcement hits 1, this function must be called by the driver to
  * notify mac80211 that the channel can be changed.
  */
 void ieee80211_csa_finish(struct ieee80211_vif *vif);
 
 /**
- * ieee80211_csa_is_complete - find out if counters reached zero
+ * ieee80211_csa_is_complete - find out if counters reached 1
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  *
  * This function returns whether the channel switch counters reached zero.
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();
-- 
1.8.4.rc3

[RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Luciano Coelho]

With a CSA count of 0, we won't transmit any CSA beacons, because the
switch will happen before the next TBTT.  To avoid extra work and
potential confusion in the drivers, complete the CSA immediately,
instead of waiting for the driver to call ieee80211_csa_finish().

To keep things simpler, we also switch immediately when the CSA count
is 1, while in theory we should delay the switch until just before the
next TBTT.

Additionally, move the ieee80211_csa_finish() function to cfg.c,
where it makes more sense (and since we call it from cfg.c now).

Cc: Simon Wunderlich <sw@simonwunderlich.de>
Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
Simon, I think with this we won't need any changes in ath9k, right?
---
 net/mac80211/cfg.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------
 net/mac80211/tx.c  |  9 ---------
 2 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b0a651c..fe5de2c 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2854,6 +2854,15 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
 	return new_beacon;
 }
 
+void ieee80211_csa_finish(struct ieee80211_vif *vif)
+{
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
+
+	ieee80211_queue_work(&sdata->local->hw,
+			     &sdata->csa_finalize_work);
+}
+EXPORT_SYMBOL(ieee80211_csa_finish);
+
 void ieee80211_csa_finalize_work(struct work_struct *work)
 {
 	struct ieee80211_sub_if_data *sdata =
@@ -2912,7 +2921,7 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_chanctx_conf *chanctx_conf;
 	struct ieee80211_chanctx *chanctx;
-	int err, num_chanctx;
+	int err, num_chanctx, changed = 0;
 
 	if (!list_empty(&local->roc_list) || local->scanning)
 		return -EBUSY;
@@ -2951,19 +2960,42 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
-		sdata->csa_counter_offset_beacon =
-			params->counter_offset_beacon;
-		sdata->csa_counter_offset_presp = params->counter_offset_presp;
 		sdata->u.ap.next_beacon =
 			cfg80211_beacon_dup(&params->beacon_after);
 		if (!sdata->u.ap.next_beacon)
 			return -ENOMEM;
 
+		/*
+		 * With a count of 0, we don't have to wait for any
+		 * TBTT before switching, so complete the CSA
+		 * immediately.  In theory, with a count == 1 we
+		 * should delay the switch until just before the next
+		 * TBTT, but that would complicate things so we switch
+		 * immediately too.  If we would delay the switch
+		 * until the next TBTT, we would have to set the probe
+		 * response here.
+		 *
+		 * TODO: A channel switch with count <= 1 without
+		 * sending a CSA action frame is kind of useless,
+		 * because the clients won't know we're changing
+		 * channels.  The action frame must be implemented
+		 * either here or in the userspace.
+		 */
+		if (params->count <= 1) {
+			ieee80211_csa_finish(&sdata->vif);
+			break;
+		}
+
+		sdata->csa_counter_offset_beacon =
+			params->counter_offset_beacon;
+		sdata->csa_counter_offset_presp = params->counter_offset_presp;
 		err = ieee80211_assign_beacon(sdata, &params->beacon_csa);
 		if (err < 0) {
 			kfree(sdata->u.ap.next_beacon);
 			return err;
 		}
+		changed |= err;
+
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		if (!sdata->vif.bss_conf.ibss_joined)
@@ -2991,9 +3023,17 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		    params->chandef.chan->band)
 			return -EINVAL;
 
+		/* see comments and TODO in the NL80211_IFTYPE_AP block */
+		if (params->count <= 1) {
+			ieee80211_csa_finish(&sdata->vif);
+			break;
+		}
+
 		err = ieee80211_ibss_csa_beacon(sdata, params);
 		if (err < 0)
 			return err;
+		changed |= err;
+
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -3009,8 +3049,10 @@ static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	local->csa_chandef = params->chandef;
 	sdata->vif.csa_active = true;
 
-	ieee80211_bss_info_change_notify(sdata, err);
-	drv_channel_switch_beacon(sdata, &params->chandef);
+	ieee80211_bss_info_change_notify(sdata, changed);
+
+	if (params->count > 1)
+		drv_channel_switch_beacon(sdata, &params->chandef);
 
 	return 0;
 }
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1e0d40f..ac4b79f 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2343,15 +2343,6 @@ static int ieee80211_beacon_add_tim(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-void ieee80211_csa_finish(struct ieee80211_vif *vif)
-{
-	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
-
-	ieee80211_queue_work(&sdata->local->hw,
-			     &sdata->csa_finalize_work);
-}
-EXPORT_SYMBOL(ieee80211_csa_finish);
-
 static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
 				 struct beacon_data *beacon)
 {
-- 
1.8.4.rc3

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Johannes Berg]

On Tue, 2013-11-05 at 22:54 +0200, Luciano Coelho wrote:
> With a CSA count of 0, we won't transmit any CSA beacons, because the
> switch will happen before the next TBTT.  To avoid extra work and
> potential confusion in the drivers, complete the CSA immediately,
> instead of waiting for the driver to call ieee80211_csa_finish().

Does it make sense to go to the workqueue rather than do it inline?

Maybe we should just reject this? I'm a bit worried that this might
confuse drivers that expect a certain order of calls?

johannes

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Coelho, Luciano]

T24gV2VkLCAyMDEzLTExLTA2IGF0IDExOjUxICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBUdWUsIDIwMTMtMTEtMDUgYXQgMjI6NTQgKzAyMDAsIEx1Y2lhbm8gQ29lbGhvIHdyb3Rl
Og0KPiA+IFdpdGggYSBDU0EgY291bnQgb2YgMCwgd2Ugd29uJ3QgdHJhbnNtaXQgYW55IENTQSBi
ZWFjb25zLCBiZWNhdXNlIHRoZQ0KPiA+IHN3aXRjaCB3aWxsIGhhcHBlbiBiZWZvcmUgdGhlIG5l
eHQgVEJUVC4gIFRvIGF2b2lkIGV4dHJhIHdvcmsgYW5kDQo+ID4gcG90ZW50aWFsIGNvbmZ1c2lv
biBpbiB0aGUgZHJpdmVycywgY29tcGxldGUgdGhlIENTQSBpbW1lZGlhdGVseSwNCj4gPiBpbnN0
ZWFkIG9mIHdhaXRpbmcgZm9yIHRoZSBkcml2ZXIgdG8gY2FsbCBpZWVlODAyMTFfY3NhX2Zpbmlz
aCgpLg0KPiANCj4gRG9lcyBpdCBtYWtlIHNlbnNlIHRvIGdvIHRvIHRoZSB3b3JrcXVldWUgcmF0
aGVyIHRoYW4gZG8gaXQgaW5saW5lPw0KDQpJIGd1ZXNzIG5vdCwgaXQgY291bGQgYmUgZG9uZSBp
bmxpbmUgdG9vLg0KDQoNCj4gTWF5YmUgd2Ugc2hvdWxkIGp1c3QgcmVqZWN0IHRoaXM/IEknbSBh
IGJpdCB3b3JyaWVkIHRoYXQgdGhpcyBtaWdodA0KPiBjb25mdXNlIGRyaXZlcnMgdGhhdCBleHBl
Y3QgYSBjZXJ0YWluIG9yZGVyIG9mIGNhbGxzPw0KDQpBRkFJQ1Qgb25seSBhdGg5ayBpcyB1c2lu
ZyB0aGlzIGluIHRoZSBtYWlubGluZS4gIFRoaXMgY2hhbmdlIGlzIG9ubHkNCnJlbGF0ZWQgdG8g
dGhlIEFQIHJvbGUsIG5vdCB0byB0aGUgY2xpZW50IHJvbGUgKG9mIHdoaWNoIHRoZXJlIGFyZSBt
YW55DQpvdGhlciB1c2VycykuDQoNCkluIHRlcm1zIG9mIHRoZSBvcmRlciBvZiBjYWxscywgdGhl
IGRpZmZlcmVuY2UgaXMgdGhhdCBpbiB0aGUgcGFzdCB3ZQ0KaGFkIHRoaXM6DQoNCjEuIFNldCBD
U0EgYmVhY29uOw0KMi4gV2hlbiBjb3VudCByZWFjaGVzIDEsIHRoZSBkcml2ZXIgY2FsbHMgaWVl
ZTgwMjExX2NzYV9maW5pc2goKTsNCjMuIFdlIGNhbGwgZHJ2X2NoYW5nZV9jaGFuY3R4KCk7DQo0
LiBTZXQgbmV3IGNoYW5uZWwgYmVhY29uLg0KDQooVGhpcyBjb250aW51ZXMgdG8gYmUgdGhlIGNh
c2Ugd2l0aCBteSBwYXRjaCB3aGVuIGNvdW50ID4gMSkNCg0KV2l0aCBteSBwYXRjaCwgaWYgY291
bnQgPD0gMSwgd2UgZG8gdGhpcyBpbnN0ZWFkOg0KDQoxLiBDYWxsIGRydl9jaGFuZ2VfY2hhbmN0
eCgpOw0KMi4gU2V0IG5ldyBjaGFubmVsIGJlYWNvbi4NCg0KVGhlIG1haW4gcHJvYmxlbSB3aXRo
b3V0IG15IHBhdGNoIGlzIHRoYXQgdGhlIGRyaXZlciBzaG91bGRuJ3QgYmVhY29uDQp3aXRoIHRo
ZSBDU0EgZWxlbWVudCB3aGVuIHRoZSBjb3VudCBzdGFydHMgPD0gMSwgc28gaXQgd29uJ3QgaGF2
ZSBhDQpjaGFuY2UgdG8gY2hlY2sgaWYgdGhlIGNvdW50IHJlYWNoZWQgMSB0byBjYWxsIGllZWU4
MDIxMV9jc2FfZmluaXNoKCkuDQoNClRoZSBvdGhlciB3YXkgdG8gZG8gdGhpcyBpczoNCg0KMS4g
U2V0IENTQSBiZWFjb247DQoyLiBCZWZvcmUgc2V0dGluZyB0aGUgYmVhY29uLCB0aGUgZHJpdmVy
IGNoZWNrcyBpZiB0aGUgY291bnQgaXMgPD0gMTsNCjMuIElmIHRoYXQncyB0aGUgY2FzZSwgdGhl
IGRyaXZlciBjYWxscyBpZWVlODAyMTFfY3NhX2ZpbmlzaCgpDQogICBpbW1lZGlhdGVsbHk7IG90
aGVyd2lzZSBpdCB3YWl0cyB1bnRpbCB0aGUgY291bnQgYmVjb21lcyAxOw0KNC4gV2UgY2FsbCBk
cnZfY2hhbmdlX2NoYW5jdHgoKTsNCjUuIFNldCBuZXcgY2hhbm5lbCBiZWFjb24uDQoNCldpdGgg
dGhpcywgd2UgZ2V0IHNvbWUgdXNlbGVzcyBjaHVybiBhbmQgbmVlZCB0byBtYWtlIHN1cmUgdGhl
IGRyaXZlcnMNCmRvIHRoZSByaWdodCB0aGluZy4uLg0KDQpEbyB5b3UgaGF2ZSBhbnkgb3RoZXIg
cHJvcG9zYWw/DQoNCi0tDQpDaGVlcnMsDQpMdWNhLg0K

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Johannes Berg]

On Wed, 2013-11-06 at 11:28 +0000, Coelho, Luciano wrote:

> In terms of the order of calls, the difference is that in the past we
> had this:
> 
> 1. Set CSA beacon;
> 2. When count reaches 1, the driver calls ieee80211_csa_finish();
> 3. We call drv_change_chanctx();
> 4. Set new channel beacon.
> 
> (This continues to be the case with my patch when count > 1)
> 
> With my patch, if count <= 1, we do this instead:
> 
> 1. Call drv_change_chanctx();
> 2. Set new channel beacon.
> 
> The main problem without my patch is that the driver shouldn't beacon
> with the CSA element when the count starts <= 1, so it won't have a
> chance to check if the count reached 1 to call ieee80211_csa_finish().

I think the other difference is that one calls
drv_channel_switch_beacon()? The driver might do some channel
preparations there, though I guess you can audit all the drivers (well,
one ...) :)

But that'd need some more documentation, otherwise I'd guess people
would start to rely on drv_channel_switch_beacon() and it would mostly
work - hence my question of whether it makes sense to refuse it at all
in the case of <=1.

johannes

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Coelho, Luciano]

T24gV2VkLCAyMDEzLTExLTA2IGF0IDEyOjMxICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBXZWQsIDIwMTMtMTEtMDYgYXQgMTE6MjggKzAwMDAsIENvZWxobywgTHVjaWFubyB3cm90
ZToNCj4gDQo+ID4gSW4gdGVybXMgb2YgdGhlIG9yZGVyIG9mIGNhbGxzLCB0aGUgZGlmZmVyZW5j
ZSBpcyB0aGF0IGluIHRoZSBwYXN0IHdlDQo+ID4gaGFkIHRoaXM6DQo+ID4gDQo+ID4gMS4gU2V0
IENTQSBiZWFjb247DQo+ID4gMi4gV2hlbiBjb3VudCByZWFjaGVzIDEsIHRoZSBkcml2ZXIgY2Fs
bHMgaWVlZTgwMjExX2NzYV9maW5pc2goKTsNCj4gPiAzLiBXZSBjYWxsIGRydl9jaGFuZ2VfY2hh
bmN0eCgpOw0KPiA+IDQuIFNldCBuZXcgY2hhbm5lbCBiZWFjb24uDQo+ID4gDQo+ID4gKFRoaXMg
Y29udGludWVzIHRvIGJlIHRoZSBjYXNlIHdpdGggbXkgcGF0Y2ggd2hlbiBjb3VudCA+IDEpDQo+
ID4gDQo+ID4gV2l0aCBteSBwYXRjaCwgaWYgY291bnQgPD0gMSwgd2UgZG8gdGhpcyBpbnN0ZWFk
Og0KPiA+IA0KPiA+IDEuIENhbGwgZHJ2X2NoYW5nZV9jaGFuY3R4KCk7DQo+ID4gMi4gU2V0IG5l
dyBjaGFubmVsIGJlYWNvbi4NCj4gPiANCj4gPiBUaGUgbWFpbiBwcm9ibGVtIHdpdGhvdXQgbXkg
cGF0Y2ggaXMgdGhhdCB0aGUgZHJpdmVyIHNob3VsZG4ndCBiZWFjb24NCj4gPiB3aXRoIHRoZSBD
U0EgZWxlbWVudCB3aGVuIHRoZSBjb3VudCBzdGFydHMgPD0gMSwgc28gaXQgd29uJ3QgaGF2ZSBh
DQo+ID4gY2hhbmNlIHRvIGNoZWNrIGlmIHRoZSBjb3VudCByZWFjaGVkIDEgdG8gY2FsbCBpZWVl
ODAyMTFfY3NhX2ZpbmlzaCgpLg0KPiANCj4gSSB0aGluayB0aGUgb3RoZXIgZGlmZmVyZW5jZSBp
cyB0aGF0IG9uZSBjYWxscw0KPiBkcnZfY2hhbm5lbF9zd2l0Y2hfYmVhY29uKCk/DQoNClJpZ2h0
LCB0aGlzIHdhcyBwYXJ0IG9mIG15ICJTZXQgQ1NBIGJlYWNvbiIgaXRlbSwgSSBzaG91bGQgaGF2
ZSBiZWVuDQptb3JlIGV4cGxpY2l0Lg0KDQoNCj4gVGhlIGRyaXZlciBtaWdodCBkbyBzb21lIGNo
YW5uZWwNCj4gcHJlcGFyYXRpb25zIHRoZXJlLCB0aG91Z2ggSSBndWVzcyB5b3UgY2FuIGF1ZGl0
IGFsbCB0aGUgZHJpdmVycyAod2VsbCwNCj4gb25lIC4uLikgOikNCg0KTHVja2lseSwgYWxsIHRo
ZSBkcml2ZXJzIHRoYXQgdXNlIHRoaXMgKHdlbGwsIG9uZS4uLiA6UCkgZG9uJ3QgZG8NCmFueXRo
aW5nIGZ1bmt5IHdpdGggdGhpcyBjYWxsLiAgYXRoOWsgb25seSB1c2VzIHRoaXMgY2FsbCBtYXJr
IHRoYXQNCnRoZXJlIGlzIGFuIG9uZ29pbmcgQ1NBLCB3aGljaCBpcyBub3QgbmVlZGVkIGluIHRo
ZSBpbW1lZGlhdGUgc3dpdGNoDQpjYXNlLg0KDQoNCj4gQnV0IHRoYXQnZCBuZWVkIHNvbWUgbW9y
ZSBkb2N1bWVudGF0aW9uLCBvdGhlcndpc2UgSSdkIGd1ZXNzIHBlb3BsZQ0KPiB3b3VsZCBzdGFy
dCB0byByZWx5IG9uIGRydl9jaGFubmVsX3N3aXRjaF9iZWFjb24oKSBhbmQgaXQgd291bGQgbW9z
dGx5DQo+IHdvcmsgLSBoZW5jZSBteSBxdWVzdGlvbiBvZiB3aGV0aGVyIGl0IG1ha2VzIHNlbnNl
IHRvIHJlZnVzZSBpdCBhdCBhbGwNCj4gaW4gdGhlIGNhc2Ugb2YgPD0xLg0KDQpSaWdodC4gIEkg
YWN0dWFsbHkgYWRkZWQgdGhpcyB0byB0aGUgZG9jdW1lbnRhdGlvbiwgYnV0IEkgYWNjaWRlbnRh
bGx5DQpzZW50IGl0IGFzIHBhcnQgb2YgcGF0Y2ggMS8yOg0KDQo+ICsrKyBiL2luY2x1ZGUvbmV0
L21hYzgwMjExLmgNCj4gQEAgLTI2NzYsMTEgKzI2NzYsMTMgQEAgZW51bSBpZWVlODAyMTFfcm9j
X3R5cGUgew0KPiAgICogQGNoYW5uZWxfc3dpdGNoX2JlYWNvbjogU3RhcnRzIGEgY2hhbm5lbCBz
d2l0Y2ggdG8gYSBuZXcgY2hhbm5lbC4NClsuLi5dDQo+ICsgKiAgICAgSWYgdGhlIENTQSBjb3Vu
dCBzdGFydHMgYXMgemVybyBvciAxLCB0aGlzIGZ1bmN0aW9uIHdpbGwgbm90IGJlIGNhbGxlZCwN
Cj4gKyAqICAgICBzaW5jZSB0aGVyZSB3b24ndCBiZSBhbnkgdGltZSB0byBiZWFjb24gYmVmb3Jl
IHRoZSBzd2l0Y2ggYW55d2F5Lg0KDQpUaGlzIGNoYW5nZSBzaG91bGQgYmUgaW4gcGF0Y2ggMi8y
IChhY3R1YWxseSB0aGVzZSB0d28gcGF0Y2hlcyBzaG91bGQNCnByb2JhYmx5IGJlIHNxdWFzaGVk
LCBJIGp1c3Qga2VwdCB0aGVtIHNlcGFyYXRlIHNvIHRoYXQgcmV2aWV3aW5nIG15IHYyDQp3b3Vs
ZCBiZSBlYXNpZXIpLg0KDQpJIGNhbiBlbGFib3JhdGUgbW9yZSBpbiB0aGUgZG9jdW1lbnRhdGlv
biBpZiB5b3UgdGhpbmsgaXQncyBuZWNlc3NhcnkuDQoNCi0tDQpDaGVlcnMsDQpMdWNhLg0K

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Johannes Berg]

On Wed, 2013-11-06 at 11:52 +0000, Coelho, Luciano wrote:

> Right.  I actually added this to the documentation, but I accidentally
> sent it as part of patch 1/2:
> 
> > +++ b/include/net/mac80211.h
> > @@ -2676,11 +2676,13 @@ enum ieee80211_roc_type {
> >   * @channel_switch_beacon: Starts a channel switch to a new channel.
> [...]
> > + *     If the CSA count starts as zero or 1, this function will not be called,
> > + *     since there won't be any time to beacon before the switch anyway.
> 
> This change should be in patch 2/2 (actually these two patches should
> probably be squashed, I just kept them separate so that reviewing my v2
> would be easier).
> 
> I can elaborate more in the documentation if you think it's necessary.

Ah, ok, I guess that's fine.

johannes

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Simon Wunderlich]

Hey Luca,

> With a CSA count of 0, we won't transmit any CSA beacons, because the
> switch will happen before the next TBTT.  To avoid extra work and
> potential confusion in the drivers, complete the CSA immediately,
> instead of waiting for the driver to call ieee80211_csa_finish().
> 
> To keep things simpler, we also switch immediately when the CSA count
> is 1, while in theory we should delay the switch until just before the
> next TBTT.
> 
> Additionally, move the ieee80211_csa_finish() function to cfg.c,
> where it makes more sense (and since we call it from cfg.c now).
> 
> Cc: Simon Wunderlich <sw@simonwunderlich.de>
> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
> ---
> Simon, I think with this we won't need any changes in ath9k, right?

I think you still need to change ieee80211_ibss_process_chanswitch() which 
also calls ieee80211_ibss_csa_beacon() and drv_channel_switch_beacon().

Also, I don't think that this is sufficient for IBSS mode - changing immediately 
without sending any action frame will just split the IBSS network. Action 
frames are currently set in ieee80211_ibss_csa_beacon() as well, with this 
change we should probably move that to another position.

Even if we don't allow count <=1 from userspace as Johannes suggested, we 
still have to fix the CSA handling from other STAs in IBSS.

Cheers,
    Simon

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Coelho, Luciano]

T24gV2VkLCAyMDEzLTExLTA2IGF0IDEyOjQ3ICswMTAwLCBTaW1vbiBXdW5kZXJsaWNoIHdyb3Rl
Og0KPiBIZXkgTHVjYSwNCj4gDQo+ID4gV2l0aCBhIENTQSBjb3VudCBvZiAwLCB3ZSB3b24ndCB0
cmFuc21pdCBhbnkgQ1NBIGJlYWNvbnMsIGJlY2F1c2UgdGhlDQo+ID4gc3dpdGNoIHdpbGwgaGFw
cGVuIGJlZm9yZSB0aGUgbmV4dCBUQlRULiAgVG8gYXZvaWQgZXh0cmEgd29yayBhbmQNCj4gPiBw
b3RlbnRpYWwgY29uZnVzaW9uIGluIHRoZSBkcml2ZXJzLCBjb21wbGV0ZSB0aGUgQ1NBIGltbWVk
aWF0ZWx5LA0KPiA+IGluc3RlYWQgb2Ygd2FpdGluZyBmb3IgdGhlIGRyaXZlciB0byBjYWxsIGll
ZWU4MDIxMV9jc2FfZmluaXNoKCkuDQo+ID4gDQo+ID4gVG8ga2VlcCB0aGluZ3Mgc2ltcGxlciwg
d2UgYWxzbyBzd2l0Y2ggaW1tZWRpYXRlbHkgd2hlbiB0aGUgQ1NBIGNvdW50DQo+ID4gaXMgMSwg
d2hpbGUgaW4gdGhlb3J5IHdlIHNob3VsZCBkZWxheSB0aGUgc3dpdGNoIHVudGlsIGp1c3QgYmVm
b3JlIHRoZQ0KPiA+IG5leHQgVEJUVC4NCj4gPiANCj4gPiBBZGRpdGlvbmFsbHksIG1vdmUgdGhl
IGllZWU4MDIxMV9jc2FfZmluaXNoKCkgZnVuY3Rpb24gdG8gY2ZnLmMsDQo+ID4gd2hlcmUgaXQg
bWFrZXMgbW9yZSBzZW5zZSAoYW5kIHNpbmNlIHdlIGNhbGwgaXQgZnJvbSBjZmcuYyBub3cpLg0K
PiA+IA0KPiA+IENjOiBTaW1vbiBXdW5kZXJsaWNoIDxzd0BzaW1vbnd1bmRlcmxpY2guZGU+DQo+
ID4gU2lnbmVkLW9mZi1ieTogTHVjaWFubyBDb2VsaG8gPGx1Y2lhbm8uY29lbGhvQGludGVsLmNv
bT4NCj4gPiAtLS0NCj4gPiBTaW1vbiwgSSB0aGluayB3aXRoIHRoaXMgd2Ugd29uJ3QgbmVlZCBh
bnkgY2hhbmdlcyBpbiBhdGg5aywgcmlnaHQ/DQo+IA0KPiBJIHRoaW5rIHlvdSBzdGlsbCBuZWVk
IHRvIGNoYW5nZSBpZWVlODAyMTFfaWJzc19wcm9jZXNzX2NoYW5zd2l0Y2goKSB3aGljaCANCj4g
YWxzbyBjYWxscyBpZWVlODAyMTFfaWJzc19jc2FfYmVhY29uKCkgYW5kIGRydl9jaGFubmVsX3N3
aXRjaF9iZWFjb24oKS4NCg0KT2theSwgSSBhZG1pdCBJIGRpZG4ndCBsb29rIGludG8gSUJTUyB0
aGF0IG11Y2guLi4gKmFnYWluKi4gOiggSSBhc3N1bWVkDQp0aGF0IHNpbXBseSBub3QgY2FsbGlu
ZyBpZWVlODAyMTFfaWJzc19jc2FfYmVhY29uKCkgd291bGQgaGF2ZSB0aGUgcmlnaHQNCmVmZmVj
dC4NCg0KDQo+IEFsc28sIEkgZG9uJ3QgdGhpbmsgdGhhdCB0aGlzIGlzIHN1ZmZpY2llbnQgZm9y
IElCU1MgbW9kZSAtIGNoYW5naW5nIGltbWVkaWF0ZWx5IA0KPiB3aXRob3V0IHNlbmRpbmcgYW55
IGFjdGlvbiBmcmFtZSB3aWxsIGp1c3Qgc3BsaXQgdGhlIElCU1MgbmV0d29yay4gQWN0aW9uIA0K
PiBmcmFtZXMgYXJlIGN1cnJlbnRseSBzZXQgaW4gaWVlZTgwMjExX2lic3NfY3NhX2JlYWNvbigp
IGFzIHdlbGwsIHdpdGggdGhpcyANCj4gY2hhbmdlIHdlIHNob3VsZCBwcm9iYWJseSBtb3ZlIHRo
YXQgdG8gYW5vdGhlciBwb3NpdGlvbi4NCg0KT2theSwgSSdsbCBsb29rIGludG8gdGhpcy4gIElu
IEFQIG1vZGUsIHN3aXRjaGluZyB3aXRob3V0IHNlbmRpbmcgdGhlDQphY3Rpb24gZnJhbWUgd2ls
bCBhbHNvIGJyZWFrIHRoZSBjb25uZWN0aW9uLCBidXQgYXQgbGVhc3QgaW4gdGhlIEFQIGNhc2UN
CnRoZSB1c2Vyc3BhY2UgY291bGQgc2VuZCB0aGUgYWN0aW9uIGZyYW1lIGJlZm9yZSBjYWxsaW5n
IGNoYW5uZWwgc3dpdGNoDQppbiBjZmc4MDIxMS4NCg0KDQo+IEV2ZW4gaWYgd2UgZG9uJ3QgYWxs
b3cgY291bnQgPD0xIGZyb20gdXNlcnNwYWNlIGFzIEpvaGFubmVzIHN1Z2dlc3RlZCwNCg0KQWgs
IEkgaGFkIG1pc3VuZGVyc3Rvb2QgSm9oYW5uZXMuICBJIHRob3VnaHQgaGUgbWVhbnQgdG8gcmVq
ZWN0IHRoZQ0KcGF0Y2guIDpQDQoNClJlamVjdGluZyBjb3VudCA8PSAxIGNvdWxkIGJlIGEgdGVt
cG9yYXJ5IHNvbHV0aW9uIHVudGlsIHdlIGltcGxlbWVudA0KdGhlIGFjdGlvbiBmcmFtZXMgcHJv
cGVybHkuICBVbmxlc3Mgd2UgZGVjaWRlIHRoYXQgdGhlIGFjdGlvbiBmcmFtZXMNCnNob3VsZCBi
ZSBoYW5kbGVkIGluIHVzZXJzcGFjZSBhcyB5b3UgcHJvcG9zZWQuDQoNCj4gd2Ugc3RpbGwgaGF2
ZSB0byBmaXggdGhlIENTQSBoYW5kbGluZyBmcm9tIG90aGVyIFNUQXMgaW4gSUJTUy4NCg0KV2hh
dCBkbyB5b3UgbWVhbj8gQmVjYXVzZSB0aGV5IGFyZSB3YWl0aW5nIGZvciBiZWFjb24gd2l0aCBj
b3VudCA9PSAwDQpiZWZvcmUgc3dpdGNoaW5nPyBJIGd1ZXNzIHRoaXMgaXMgYWxzbyB3cm9uZyBp
biBCU1MgU1RBcy4gIEknbGwgY2hlY2suDQoNCi0tDQpMdWNhLg0K

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Johannes Berg]

On Wed, 2013-11-06 at 12:03 +0000, Coelho, Luciano wrote:

> > Even if we don't allow count <=1 from userspace as Johannes suggested,
> 
> Ah, I had misunderstood Johannes.  I thought he meant to reject the
> patch. :P
> 
> Rejecting count <= 1 could be a temporary solution until we implement
> the action frames properly.  Unless we decide that the action frames
> should be handled in userspace as you proposed.

Well even then - the question is who would ever want to use and test
count<=1?

johannes

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Coelho, Luciano]

T24gV2VkLCAyMDEzLTExLTA2IGF0IDEzOjA2ICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBXZWQsIDIwMTMtMTEtMDYgYXQgMTI6MDMgKzAwMDAsIENvZWxobywgTHVjaWFubyB3cm90
ZToNCj4gDQo+ID4gPiBFdmVuIGlmIHdlIGRvbid0IGFsbG93IGNvdW50IDw9MSBmcm9tIHVzZXJz
cGFjZSBhcyBKb2hhbm5lcyBzdWdnZXN0ZWQsDQo+ID4gDQo+ID4gQWgsIEkgaGFkIG1pc3VuZGVy
c3Rvb2QgSm9oYW5uZXMuICBJIHRob3VnaHQgaGUgbWVhbnQgdG8gcmVqZWN0IHRoZQ0KPiA+IHBh
dGNoLiA6UA0KPiA+IA0KPiA+IFJlamVjdGluZyBjb3VudCA8PSAxIGNvdWxkIGJlIGEgdGVtcG9y
YXJ5IHNvbHV0aW9uIHVudGlsIHdlIGltcGxlbWVudA0KPiA+IHRoZSBhY3Rpb24gZnJhbWVzIHBy
b3Blcmx5LiAgVW5sZXNzIHdlIGRlY2lkZSB0aGF0IHRoZSBhY3Rpb24gZnJhbWVzDQo+ID4gc2hv
dWxkIGJlIGhhbmRsZWQgaW4gdXNlcnNwYWNlIGFzIHlvdSBwcm9wb3NlZC4NCj4gDQo+IFdlbGwg
ZXZlbiB0aGVuIC0gdGhlIHF1ZXN0aW9uIGlzIHdobyB3b3VsZCBldmVyIHdhbnQgdG8gdXNlIGFu
ZCB0ZXN0DQo+IGNvdW50PD0xPw0KDQpJIGd1ZXNzIHRoZSByZWFzb24gdG8gaGF2ZSB0aGlzIGlz
IHdoZW4geW91IG11c3QgZXZhY3VhdGUgdGhlIGNoYW5uZWwNCmltbWVkaWF0ZWx5LCBzbyB5b3Ug
dHJhbnNtaXQgdGhlIGFjdGlvbiBmcmFtZSBhbmQganVtcCBvdXQuICBJZiB5b3VyDQpiZWFjb24g
aW50ZXJ2YWwgaXMgbG9uZyAoZm9yIHdoYXRldmVyIHJlYXNvbiksIHdhaXRpbmcgZm9yIHRoZSBu
ZXh0IFRCVFQNCnRvIGFubm91bmNlIHRoZSBDU0EgbWF5IHRha2UgbG9uZ2VyIHRoYW4gdGhlIHRp
bWUgcmVxdWlyZWQgdG8gc3RvcA0KdHJhbnNtaXR0aW5nLg0KDQpBbHNvLCBpbiBhICJzdG9wIHRy
YW5zbWl0dGluZyBpbW1lZGlhdGVseSIgc2NlbmFyaW8sIHVzaW5nIGFuIGFjdGlvbg0KZnJhbWUg
d2l0aCBjb3VudCA8PSAxIGhlbHBzIHJlZHVjZSBkYXRhIGhpY2N1cHMgdGhhdCB3b3VsZCBoYXBw
ZW4gaWYNCmNvdW50ID4gMSBhbmQgdGhlIGNoYW5uZWwgc3dpdGNoIG1vZGUgaXMgImRvbid0IHRy
YW5zbWl0Ii4NCg0KLS0NCkx1Y2EuDQo=

Re: [RFC v2 2/2] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Johannes Berg]

On Wed, 2013-11-06 at 12:22 +0000, Coelho, Luciano wrote:

> > Well even then - the question is who would ever want to use and test
> > count<=1?
> 
> I guess the reason to have this is when you must evacuate the channel
> immediately, so you transmit the action frame and jump out.  If your
> beacon interval is long (for whatever reason), waiting for the next TBTT
> to announce the CSA may take longer than the time required to stop
> transmitting.

Fair point, so I guess we'd want to also test that :)

johannes