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

[RFC v3 1/4] 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.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In v2, updated the documentation to reflect the change.  This is going
to be sent to linux-wireless for comments from the community too.

In v3, removed the part of the documentation that belongs in the next
patch.
    
 include/net/mac80211.h |  8 ++++----
 net/mac80211/tx.c      | 10 +++++++---
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 7ceed99..ec6ed6d 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2680,10 +2680,10 @@ 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().
  *
  * @join_ibss: Join an IBSS (on an IBSS interface); this is called after all
@@ -3383,13 +3383,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 c558b24..57d9feb 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2409,8 +2409,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]--;
@@ -2476,7 +2480,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 v3 2/4] mac80211: refactor ieee80211_ibss_process_chanswitch()

[Luciano Coelho]

Refactor ieee80211_ibss_process_chanswitch() to use
ieee80211_channel_switch() and avoid code duplication.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---

This is a new patch in the series to make things ready for patch 4/4.

 net/mac80211/cfg.c         |  4 ++--
 net/mac80211/ibss.c        | 58 +++++++---------------------------------------
 net/mac80211/ieee80211_i.h |  2 ++
 3 files changed, 12 insertions(+), 52 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 95667b0..44b2f51 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3014,8 +3014,8 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	cfg80211_ch_switch_notify(sdata->dev, &local->csa_chandef);
 }
 
-static int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
-				    struct cfg80211_csa_settings *params)
+int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
+			     struct cfg80211_csa_settings *params)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_local *local = sdata->local;
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 531be04..548eae8 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -784,18 +784,10 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	struct cfg80211_csa_settings params;
 	struct ieee80211_csa_ie csa_ie;
 	struct ieee80211_if_ibss *ifibss = &sdata->u.ibss;
-	struct ieee80211_chanctx_conf *chanctx_conf;
-	struct ieee80211_chanctx *chanctx;
 	enum nl80211_channel_type ch_type;
-	int err, num_chanctx;
+	int err;
 	u32 sta_flags;
 
-	if (sdata->vif.csa_active)
-		return true;
-
-	if (!sdata->vif.bss_conf.ibss_joined)
-		return false;
-
 	sta_flags = IEEE80211_STA_DISABLE_VHT;
 	switch (ifibss->chandef.width) {
 	case NL80211_CHAN_WIDTH_5:
@@ -826,9 +818,6 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	params.count = csa_ie.count;
 	params.chandef = csa_ie.chandef;
 
-	if (ifibss->chandef.chan->band != params.chandef.chan->band)
-		goto disconnect;
-
 	switch (ifibss->chandef.width) {
 	case NL80211_CHAN_WIDTH_20_NOHT:
 	case NL80211_CHAN_WIDTH_20:
@@ -884,29 +873,6 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 		params.radar_required = true;
 	}
 
-	rcu_read_lock();
-	chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
-	if (!chanctx_conf) {
-		rcu_read_unlock();
-		goto disconnect;
-	}
-
-	/* don't handle for multi-VIF cases */
-	chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
-	if (chanctx->refcount > 1) {
-		rcu_read_unlock();
-		goto disconnect;
-	}
-	num_chanctx = 0;
-	list_for_each_entry_rcu(chanctx, &sdata->local->chanctx_list, list)
-		num_chanctx++;
-
-	if (num_chanctx > 1) {
-		rcu_read_unlock();
-		goto disconnect;
-	}
-	rcu_read_unlock();
-
 	/* all checks done, now perform the channel switch. */
 	ibss_dbg(sdata,
 		 "received channel switch announcement to go to channel %d MHz\n",
@@ -914,19 +880,9 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 
 	params.block_tx = !!csa_ie.mode;
 
-	ieee80211_ibss_csa_beacon(sdata, &params);
-	sdata->csa_radar_required = params.radar_required;
-
-	if (params.block_tx)
-		ieee80211_stop_queues_by_reason(&sdata->local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
-
-	sdata->local->csa_chandef = params.chandef;
-	sdata->vif.csa_active = true;
-
-	ieee80211_bss_info_change_notify(sdata, err);
-	drv_channel_switch_beacon(sdata, &params.chandef);
+	if (ieee80211_channel_switch(sdata->local->hw.wiphy, sdata->dev,
+				     &params))
+		goto disconnect;
 
 	ieee80211_ibss_csa_mark_radar(sdata);
 
@@ -962,7 +918,8 @@ ieee80211_rx_mgmt_spectrum_mgmt(struct ieee80211_sub_if_data *sdata,
 	if (len < required_len)
 		return;
 
-	ieee80211_ibss_process_chanswitch(sdata, elems, false);
+	if (!sdata->vif.csa_active)
+		ieee80211_ibss_process_chanswitch(sdata, elems, false);
 }
 
 static void ieee80211_rx_mgmt_deauth_ibss(struct ieee80211_sub_if_data *sdata,
@@ -1143,7 +1100,8 @@ static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
 		goto put_bss;
 
 	/* process channel switch */
-	if (ieee80211_ibss_process_chanswitch(sdata, elems, true))
+	if (sdata->vif.csa_active ||
+	    ieee80211_ibss_process_chanswitch(sdata, elems, true))
 		goto put_bss;
 
 	/* same BSSID */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 29dc505..90bc5ae 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1414,6 +1414,8 @@ void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
 
 /* channel switch handling */
 void ieee80211_csa_finalize_work(struct work_struct *work);
+int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
+			     struct cfg80211_csa_settings *params);
 
 /* interface handling */
 int ieee80211_iface_init(void);
-- 
1.8.4.rc3

[RFC v3 3/4] mac80211: align ieee80211_ibss_csa_beacon() with ieee80211_csa_beacon()

[Luciano Coelho]

The return value of ieee80211_ibss_csa_beacon is not aligned with the
return value of ieee80211_csa_beacon().  For consistency and to be
able to use both functions with similar code, change
ieee80211_ibss_csa_beacon() not to send the bss changed notification
itself, but return what has changed so the caller can send the
notification instead.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---

This is a new patch in the series to make things ready for patch 4/4.


 net/mac80211/ibss.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 548eae8..1c19df3 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -522,7 +522,7 @@ int ieee80211_ibss_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	if (csa_settings)
 		ieee80211_send_action_csa(sdata, csa_settings);
 
-	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON);
+	return BSS_CHANGED_BEACON;
  out:
 	return ret;
 }
@@ -559,9 +559,15 @@ int ieee80211_ibss_finish_csa(struct ieee80211_sub_if_data *sdata)
 
 	/* generate the beacon */
 	err = ieee80211_ibss_csa_beacon(sdata, NULL);
-	sdata_unlock(sdata);
-	if (err < 0)
+	if (err < 0) {
+		sdata_unlock(sdata);
 		return err;
+	}
+
+	if (err)
+		ieee80211_bss_info_change_notify(sdata, err);
+
+	sdata_unlock(sdata);
 
 	return 0;
 }
-- 
1.8.4.rc3

[RFC v3 4/4] 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.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---

In v3:

* moved the documentation change that explains that the
  channel_switch_beacon() driver op is not called when count <= 1;
  (Johannes)

* call finalize work immediately instead of doing it through the
  workqueue. (Johannes)

* instead of checking (params->count > 1), check the changed variable
  to decide whether to call drv_channel_switch_beacon() (and
  ieee80211_bss_info_change_notify() as well);


 include/net/mac80211.h |  2 ++
 net/mac80211/cfg.c     | 74 +++++++++++++++++++++++++++++++++++++++++---------
 net/mac80211/tx.c      |  9 ------
 3 files changed, 63 insertions(+), 22 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ec6ed6d..f08c76a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2685,6 +2685,8 @@ enum ieee80211_roc_type {
  *	get the csa counter decremented by mac80211, but must check if it is
  *	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
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 44b2f51..9baf469 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2956,17 +2956,20 @@ cfg80211_beacon_dup(struct cfg80211_beacon_data *beacon)
 	return new_beacon;
 }
 
-void ieee80211_csa_finalize_work(struct work_struct *work)
+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_csa_finalize(struct ieee80211_sub_if_data *sdata)
 {
-	struct ieee80211_sub_if_data *sdata =
-		container_of(work, struct ieee80211_sub_if_data,
-			     csa_finalize_work);
 	struct ieee80211_local *local = sdata->local;
 	int err, changed = 0;
 
-	if (!ieee80211_sdata_running(sdata))
-		return;
-
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &local->csa_chandef,
 					   &changed);
@@ -3014,6 +3017,18 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	cfg80211_ch_switch_notify(sdata->dev, &local->csa_chandef);
 }
 
+void ieee80211_csa_finalize_work(struct work_struct *work)
+{
+	struct ieee80211_sub_if_data *sdata =
+		container_of(work, struct ieee80211_sub_if_data,
+			     csa_finalize_work);
+
+	if (!ieee80211_sdata_running(sdata))
+		return;
+
+	ieee80211_csa_finalize(sdata);
+}
+
 int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 			     struct cfg80211_csa_settings *params)
 {
@@ -3022,7 +3037,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	struct ieee80211_chanctx_conf *chanctx_conf;
 	struct ieee80211_chanctx *chanctx;
 	struct ieee80211_if_mesh __maybe_unused *ifmsh;
-	int err, num_chanctx;
+	int err, num_chanctx, changed = 0;
 
 	if (!list_empty(&local->roc_list) || local->scanning)
 		return -EBUSY;
@@ -3061,19 +3076,42 @@ 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_finalize(sdata);
+			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)
@@ -3101,9 +3139,17 @@ 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_finalize(sdata);
+			break;
+		}
+
 		err = ieee80211_ibss_csa_beacon(sdata, params);
 		if (err < 0)
 			return err;
+		changed |= err;
+
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
@@ -3139,8 +3185,10 @@ 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);
+	if (changed) {
+		ieee80211_bss_info_change_notify(sdata, changed);
+		drv_channel_switch_beacon(sdata, &params->chandef);
+	}
 
 	return 0;
 }
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 57d9feb..f324ab7 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2372,15 +2372,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 v3 1/4] mac80211: don't transmit beacon with CSA count 0

[Luca Coelho]

While rebasing this patches to send series out, I noticed that mesh CSA
was also recently merged, so I'll have to take care of that too.  But I
decided to send these out anyway, so review can start.

I'll send a v4 with mesh taken into consideration as well.

Heheh, this started as a simple 2 liner patch, and look where we are
now. :D

--
Cheers,
Luca.

On Fri, 2013-11-08 at 16:39 +0200, Luciano Coelho wrote:
> 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.
> 
> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
> ---
> In v2, updated the documentation to reflect the change.  This is going
> to be sent to linux-wireless for comments from the community too.
> 
> In v3, removed the part of the documentation that belongs in the next
> patch.
>     
>  include/net/mac80211.h |  8 ++++----
>  net/mac80211/tx.c      | 10 +++++++---
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 7ceed99..ec6ed6d 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -2680,10 +2680,10 @@ 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().
>   *
>   * @join_ibss: Join an IBSS (on an IBSS interface); this is called after all
> @@ -3383,13 +3383,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 c558b24..57d9feb 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -2409,8 +2409,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]--;
> @@ -2476,7 +2480,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();

Re: [RFC v3 1/4] mac80211: don't transmit beacon with CSA count 0

[Johannes Berg]

On Fri, 2013-11-08 at 16:39 +0200, Luciano Coelho wrote:
> 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.

These seem fine to me - maybe we should have some CSA tests in Jouni's
hwsim test scripts, for the various cases? :)

johannes

Re: [RFC v3 1/4] mac80211: don't transmit beacon with CSA count 0

[Coelho, Luciano]

T24gTW9uLCAyMDEzLTExLTExIGF0IDE0OjU5ICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBGcmksIDIwMTMtMTEtMDggYXQgMTY6MzkgKzAyMDAsIEx1Y2lhbm8gQ29lbGhvIHdyb3Rl
Og0KPiA+IEEgYmVhY29uIHNob3VsZCBuZXZlciBoYXZlIGEgQ2hhbm5lbCBTd2l0Y2ggQW5ub3Vu
Y2VtZW50IGluZm9ybWF0aW9uDQo+ID4gZWxlbWVudCB3aXRoIGEgY291bnQgb2YgMCwgYmVjYXVz
ZSBhIGNvdW50IG9mIDEgbWVhbnMgc3dpdGNoIGp1c3QNCj4gPiBiZWZvcmUgdGhlIG5leHQgYmVh
Y29uLiAgU28sIGlmIGEgY291bnQgb2YgMCB3YXMgdmFsaWQgaW4gYSBiZWFjb24sIGl0DQo+ID4g
d291bGQgaGF2ZSBiZWVuIHRyYW5zbWl0dGVkIGluIHRoZSBuZXh0IGNoYW5uZWwgYWxyZWFkeSwg
d2hpY2ggaXMNCj4gPiB1c2VsZXNzLiAgQSBDU0EgY291bnQgZXF1YWwgdG8gemVybyBpcyBvbmx5
IG1lYW5pbmdmdWwgaW4gYWN0aW9uDQo+ID4gZnJhbWVzIG9yIHByb2JlX3Jlc3BvbnNlcy4NCj4g
PiANCj4gPiBGaXggdGhlIGllZWU4MDIxMV9jc2FfaXNfY29tcGxldGUoKSBhbmQgaWVlZTgwMjEx
X3VwZGF0ZV9jc2EoKQ0KPiA+IGZ1bmN0aW9ucyBhY2NvcmRpbmdseS4NCj4gDQo+IFRoZXNlIHNl
ZW0gZmluZSB0byBtZSAtIG1heWJlIHdlIHNob3VsZCBoYXZlIHNvbWUgQ1NBIHRlc3RzIGluIEpv
dW5pJ3MNCj4gaHdzaW0gdGVzdCBzY3JpcHRzLCBmb3IgdGhlIHZhcmlvdXMgY2FzZXM/IDopDQoN
CkNvb2wgdGhhdCB0aGlzIGlzIGZpbmUgd2l0aCB5b3UuICBJIGNhbiBwcm9jZWVkIG1ha2luZyB0
aGUgZmluYWwgY2hhbmdlcw0KZm9yIHY0IHRoZW4uIDspDQoNCkknbGwgdGFrZSBhIGxvb2sgaW50
byB0aGUgaHdzaW0gdGVzdCBzY3JpcHRzLg0KDQotLQ0KQ2hlZXJzLA0KTHVjYS4NCg==

Re: [RFC v3 1/4] mac80211: don't transmit beacon with CSA count 0

[Simon Wunderlich]

> 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.

Just to make future bisecting easier, shouldn't this patch go after "only set 
CSA beacon when at least one beacon must be transmitted" in the series? 
Otherwise userspace may change the channel with count=0 and hit a warning 
here, because this is not supported anymore.

Apart from that, the series looks fine. I'd like to test the next v4 then.

I guess we still need to do the action frame part - for IBSS mode we already 
have code but we probably need to make sure that these packets go out before 
actually changing the channel. Do you plan to work on that?

Thanks for your work. :)
   Simon

Re: [RFC v3 1/4] mac80211: don't transmit beacon with CSA count 0

[Coelho, Luciano]

T24gTW9uLCAyMDEzLTExLTExIGF0IDE1OjU3ICswMTAwLCBTaW1vbiBXdW5kZXJsaWNoIHdyb3Rl
Og0KPiA+IEEgYmVhY29uIHNob3VsZCBuZXZlciBoYXZlIGEgQ2hhbm5lbCBTd2l0Y2ggQW5ub3Vu
Y2VtZW50IGluZm9ybWF0aW9uDQo+ID4gZWxlbWVudCB3aXRoIGEgY291bnQgb2YgMCwgYmVjYXVz
ZSBhIGNvdW50IG9mIDEgbWVhbnMgc3dpdGNoIGp1c3QNCj4gPiBiZWZvcmUgdGhlIG5leHQgYmVh
Y29uLiAgU28sIGlmIGEgY291bnQgb2YgMCB3YXMgdmFsaWQgaW4gYSBiZWFjb24sIGl0DQo+ID4g
d291bGQgaGF2ZSBiZWVuIHRyYW5zbWl0dGVkIGluIHRoZSBuZXh0IGNoYW5uZWwgYWxyZWFkeSwg
d2hpY2ggaXMNCj4gPiB1c2VsZXNzLiAgQSBDU0EgY291bnQgZXF1YWwgdG8gemVybyBpcyBvbmx5
IG1lYW5pbmdmdWwgaW4gYWN0aW9uDQo+ID4gZnJhbWVzIG9yIHByb2JlX3Jlc3BvbnNlcy4NCj4g
PiANCj4gPiBGaXggdGhlIGllZWU4MDIxMV9jc2FfaXNfY29tcGxldGUoKSBhbmQgaWVlZTgwMjEx
X3VwZGF0ZV9jc2EoKQ0KPiA+IGZ1bmN0aW9ucyBhY2NvcmRpbmdseS4NCj4gDQo+IEp1c3QgdG8g
bWFrZSBmdXR1cmUgYmlzZWN0aW5nIGVhc2llciwgc2hvdWxkbid0IHRoaXMgcGF0Y2ggZ28gYWZ0
ZXIgIm9ubHkgc2V0IA0KPiBDU0EgYmVhY29uIHdoZW4gYXQgbGVhc3Qgb25lIGJlYWNvbiBtdXN0
IGJlIHRyYW5zbWl0dGVkIiBpbiB0aGUgc2VyaWVzPyANCj4gT3RoZXJ3aXNlIHVzZXJzcGFjZSBt
YXkgY2hhbmdlIHRoZSBjaGFubmVsIHdpdGggY291bnQ9MCBhbmQgaGl0IGEgd2FybmluZyANCj4g
aGVyZSwgYmVjYXVzZSB0aGlzIGlzIG5vdCBzdXBwb3J0ZWQgYW55bW9yZS4NCg0KTXkgcGxhbiB3
YXMgdG8gc3F1YXNoIHRoZW0sIGJlY2F1c2UgdGhleSByZWFsbHkgYmVsb25nIHRvZ2V0aGVyLiAg
V2hhdA0KZG8geW91IHRoaW5rPw0KDQoNCg0KPiBBcGFydCBmcm9tIHRoYXQsIHRoZSBzZXJpZXMg
bG9va3MgZmluZS4gSSdkIGxpa2UgdG8gdGVzdCB0aGUgbmV4dCB2NCB0aGVuLg0KDQpHcmVhdCEg
SSdsbCB0cnkgdG8gc2VuZCB2NCBzb29uLCBob3BlZnVsbHkgdGhlIG1pc3NpbmcgbWVzaCBwYXJ0
IGRvZXNuJ3QNCnR1cm4gb3V0IHRvIGJlIHRvbyBjb21wbGljYXRlZC4gOykNCg0KDQo+IEkgZ3Vl
c3Mgd2Ugc3RpbGwgbmVlZCB0byBkbyB0aGUgYWN0aW9uIGZyYW1lIHBhcnQgLSBmb3IgSUJTUyBt
b2RlIHdlIGFscmVhZHkgDQo+IGhhdmUgY29kZSBidXQgd2UgcHJvYmFibHkgbmVlZCB0byBtYWtl
IHN1cmUgdGhhdCB0aGVzZSBwYWNrZXRzIGdvIG91dCBiZWZvcmUgDQo+IGFjdHVhbGx5IGNoYW5n
aW5nIHRoZSBjaGFubmVsLiBEbyB5b3UgcGxhbiB0byB3b3JrIG9uIHRoYXQ/DQoNClllcywgSSds
bCBsb29rIGludG8gaXQuICBJIHRoaW5rIGl0IHNob3VsZCBiZSBwcmV0dHkgc2ltcGxlLCBlc3Bl
Y2lhbGx5DQpub3cgdGhhdCB3ZSBhbHJlYWR5IGhhdmUgSUJTUyBhbmQgTUVTSCBzZW5kaW5nIGl0
IG91dC4NCg0KDQo+IFRoYW5rcyBmb3IgeW91ciB3b3JrLiA6KQ0KDQpUaGFua3MgZm9yIHlvdXIg
aGVscCEgOykNCg0KLS0NCkx1Y2EuDQoNCg==

Re: [RFC v3 1/4] mac80211: don't transmit beacon with CSA count 0

[Simon Wunderlich]

> > Just to make future bisecting easier, shouldn't this patch go after "only
> > set CSA beacon when at least one beacon must be transmitted" in the
> > series? Otherwise userspace may change the channel with count=0 and hit
> > a warning here, because this is not supported anymore.
> 
> My plan was to squash them, because they really belong together.  What
> do you think?
> 

Good idea, it belongs together anyway. 

> > Apart from that, the series looks fine. I'd like to test the next v4
> > then.
> 
> Great! I'll try to send v4 soon, hopefully the missing mesh part doesn't
> turn out to be too complicated. ;)

OK

> > I guess we still need to do the action frame part - for IBSS mode we
> > already have code but we probably need to make sure that these packets
> > go out before actually changing the channel. Do you plan to work on
> > that?
> 
> Yes, I'll look into it.  I think it should be pretty simple, especially
> now that we already have IBSS and MESH sending it out.

Great, thanks!
    Simon