[PATCH v8 0/5] CSA beacon count changes

[PATCH v8 0/5] CSA beacon count changes

[Luciano Coelho]

In v8:

* removed bogus sdata_unlock from ieee80211_csa_finalize();

In v7:

* (hopefully) a fix for the kfree_rcu() oops (thanks Chun-Yeow);
* removed stray extra space at the top of cfg.c (thanks Johannes);
* removed the TODO action frame comment for mesh and IBSS (thanks Simon);

Chun-Yeow, it would be great if you could retest this, at least once
with count > 1 and once with count <= 1.

Simon, would you have the time to test the IBSS case as well? I'd
really appreciate that! :)

--
Cheers,
Luca.

Luciano Coelho (5):
  mac80211: refactor ieee80211_ibss_process_chanswitch()
  mac80211: align ieee80211_ibss_csa_beacon() with
    ieee80211_csa_beacon()
  mac80211: refactor ieee80211_mesh_process_chanswitch()
  mac80211: align ieee80211_mesh_csa_beacon() with
    ieee80211_csa_beacon()
  mac80211: only set CSA beacon when at least one beacon must be
    transmitted

 include/net/mac80211.h     |  10 ++--
 net/mac80211/cfg.c         | 119 +++++++++++++++++++++++++++++++++++----------
 net/mac80211/ibss.c        |  78 +++++++----------------------
 net/mac80211/ieee80211_i.h |  11 +++--
 net/mac80211/mesh.c        |  81 +++++++-----------------------
 net/mac80211/tx.c          |  19 +++-----
 net/mac80211/util.c        |   1 -
 7 files changed, 150 insertions(+), 169 deletions(-)

-- 
1.8.4.3

[PATCH v8 1/5] mac80211: refactor ieee80211_ibss_process_chanswitch()

[Luciano Coelho]

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

Change-Id: I265a12c7f825dc20535bad1197a81437310d0086
Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 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 754069c..b5bc27e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3048,8 +3048,8 @@ unlock:
 	sdata_unlock(sdata);
 }
 
-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 0f1fb5d..3514aab 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->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 32bae21..a7b4d55 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1442,6 +1442,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.3

[PATCH v8 2/5] 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>
---
 net/mac80211/ibss.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 3514aab..23e035f 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,11 +559,17 @@ 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)
-		return err;
+		goto out;
 
-	return 0;
+	if (err) {
+		ieee80211_bss_info_change_notify(sdata, err);
+		err = 0;
+	}
+out:
+	sdata_unlock(sdata);
+
+	return err;
 }
 
 void ieee80211_ibss_stop(struct ieee80211_sub_if_data *sdata)
-- 
1.8.4.3

Re: [PATCH v8 2/5] mac80211: align ieee80211_ibss_csa_beacon() with ieee80211_csa_beacon()

[Jones Desougi]

On 05/12/13 16:25, Luciano Coelho wrote:
> 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>
> ---
>  net/mac80211/ibss.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
> index 3514aab..23e035f 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,11 +559,17 @@ 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)
> -		return err;
> +		goto out;
>  
> -	return 0;
> +	if (err) {
> +		ieee80211_bss_info_change_notify(sdata, err);
> +		err = 0;
> +	}
> +out:
> +	sdata_unlock(sdata);

Why not simplify to something like this?
	/* generate the beacon */
	err = ieee80211_ibss_csa_beacon(sdata, NULL);
	if (err > 0) {
		ieee80211_bss_info_change_notify(sdata, err);
		err = 0;
	}
	sdata_unlock(sdata);

> +
> +	return err;
>  }
>  
>  void ieee80211_ibss_stop(struct ieee80211_sub_if_data *sdata)
> 

[PATCH v8 3/5] mac80211: refactor ieee80211_mesh_process_chanswitch()

[Luciano Coelho]

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

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 net/mac80211/cfg.c         | 10 +++++--
 net/mac80211/ieee80211_i.h |  6 +++-
 net/mac80211/mesh.c        | 68 ++++++++++------------------------------------
 net/mac80211/util.c        |  1 -
 4 files changed, 27 insertions(+), 58 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b5bc27e..790c15d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3156,9 +3156,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		    params->chandef.chan->band)
 			return -EINVAL;
 
-		err = ieee80211_mesh_csa_beacon(sdata, params, true);
-		if (err < 0)
+		if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_NONE)
+			ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_INIT;
+
+		err = ieee80211_mesh_csa_beacon(sdata, params,
+			(ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT));
+		if (err < 0) {
+			ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
 			return err;
+		}
 		break;
 #endif
 	default:
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a7b4d55..cf38a74 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -611,7 +611,11 @@ struct ieee80211_if_mesh {
 	struct ps_data ps;
 	/* Channel Switching Support */
 	struct mesh_csa_settings __rcu *csa;
-	bool chsw_init;
+	enum {
+		IEEE80211_MESH_CSA_ROLE_NONE,
+		IEEE80211_MESH_CSA_ROLE_INIT,
+		IEEE80211_MESH_CSA_ROLE_REPEATER,
+	} csa_role;
 	u8 chsw_ttl;
 	u16 pre_value;
 };
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 330d1f7..980cc12 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -685,7 +685,7 @@ ieee80211_mesh_build_beacon(struct ieee80211_if_mesh *ifmsh)
 		*pos++ = csa->settings.count;
 		*pos++ = WLAN_EID_CHAN_SWITCH_PARAM;
 		*pos++ = 6;
-		if (ifmsh->chsw_init) {
+		if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT) {
 			*pos++ = ifmsh->mshcfg.dot11MeshTTL;
 			*pos |= WLAN_EID_CHAN_SWITCH_PARAM_INITIATOR;
 		} else {
@@ -853,19 +853,11 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 {
 	struct cfg80211_csa_settings params;
 	struct ieee80211_csa_ie csa_ie;
-	struct ieee80211_chanctx_conf *chanctx_conf;
-	struct ieee80211_chanctx *chanctx;
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	enum ieee80211_band band = ieee80211_get_sdata_band(sdata);
-	int err, num_chanctx;
+	int err;
 	u32 sta_flags;
 
-	if (sdata->vif.csa_active)
-		return true;
-
-	if (!ifmsh->mesh_id)
-		return false;
-
 	sta_flags = IEEE80211_STA_DISABLE_VHT;
 	switch (sdata->vif.bss_conf.chandef.width) {
 	case NL80211_CHAN_WIDTH_20_NOHT:
@@ -890,10 +882,6 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 	params.chandef = csa_ie.chandef;
 	params.count = csa_ie.count;
 
-	if (sdata->vif.bss_conf.chandef.chan->band !=
-	    params.chandef.chan->band)
-		return false;
-
 	if (!cfg80211_chandef_usable(sdata->local->hw.wiphy, &params.chandef,
 				     IEEE80211_CHAN_DISABLED)) {
 		sdata_info(sdata,
@@ -916,25 +904,6 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 		return false;
 	}
 
-	rcu_read_lock();
-	chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
-	if (!chanctx_conf)
-		goto failed_chswitch;
-
-	/* don't handle for multi-VIF cases */
-	chanctx = container_of(chanctx_conf, struct ieee80211_chanctx, conf);
-	if (chanctx->refcount > 1)
-		goto failed_chswitch;
-
-	num_chanctx = 0;
-	list_for_each_entry_rcu(chanctx, &sdata->local->chanctx_list, list)
-		num_chanctx++;
-
-	if (num_chanctx > 1)
-		goto failed_chswitch;
-
-	rcu_read_unlock();
-
 	mcsa_dbg(sdata,
 		 "received channel switch announcement to go to channel %d MHz\n",
 		 params.chandef.chan->center_freq);
@@ -945,27 +914,16 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 	else
 		ifmsh->chsw_ttl = 0;
 
-	if (ifmsh->chsw_ttl > 0)
-		if (ieee80211_mesh_csa_beacon(sdata, &params, false) < 0)
-			return false;
-
-	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);
+	if (ifmsh->chsw_ttl == 0)
+		return false;
 
-	sdata->csa_chandef = params.chandef;
-	sdata->vif.csa_active = true;
+	ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_REPEATER;
 
-	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) < 0)
+		return false;
 
 	return true;
-failed_chswitch:
-	rcu_read_unlock();
-	return false;
 }
 
 static void
@@ -1075,7 +1033,8 @@ static void ieee80211_mesh_rx_bcn_presp(struct ieee80211_sub_if_data *sdata,
 		ifmsh->sync_ops->rx_bcn_presp(sdata,
 			stype, mgmt, &elems, rx_status);
 
-	if (!ifmsh->chsw_init)
+	if (ifmsh->csa_role != IEEE80211_MESH_CSA_ROLE_INIT &&
+	    !sdata->vif.csa_active)
 		ieee80211_mesh_process_chnswitch(sdata, &elems, true);
 }
 
@@ -1086,7 +1045,7 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
 	int ret = 0;
 
 	/* Reset the TTL value and Initiator flag */
-	ifmsh->chsw_init = false;
+	ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
 	ifmsh->chsw_ttl = 0;
 
 	/* Remove the CSA and MCSP elements from the beacon */
@@ -1200,7 +1159,8 @@ static void mesh_rx_csa_frame(struct ieee80211_sub_if_data *sdata,
 
 	ifmsh->pre_value = pre_value;
 
-	if (!ieee80211_mesh_process_chnswitch(sdata, &elems, false)) {
+	if (!sdata->vif.csa_active &&
+	    !ieee80211_mesh_process_chnswitch(sdata, &elems, false)) {
 		mcsa_dbg(sdata, "Failed to process CSA action frame");
 		return;
 	}
@@ -1355,7 +1315,7 @@ void ieee80211_mesh_init_sdata(struct ieee80211_sub_if_data *sdata)
 	mesh_rmc_init(sdata);
 	ifmsh->last_preq = jiffies;
 	ifmsh->next_perr = jiffies;
-	ifmsh->chsw_init = false;
+	ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
 	/* Allocate all mesh structures when creating the first mesh interface. */
 	if (!mesh_allocated)
 		ieee80211s_init();
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 4a376a7..0dd7911 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2498,7 +2498,6 @@ int ieee80211_send_action_csa(struct ieee80211_sub_if_data *sdata,
 			ifmsh->pre_value++;
 		put_unaligned_le16(ifmsh->pre_value, pos);/* Precedence Value */
 		pos += 2;
-		ifmsh->chsw_init = true;
 	}
 
 	ieee80211_tx_skb(sdata, skb);
-- 
1.8.4.3

Re: [PATCH v8 3/5] mac80211: refactor ieee80211_mesh_process_chanswitch()

[Johannes Berg]

On Thu, 2013-12-05 at 17:25 +0200, Luciano Coelho wrote:

> +++ b/net/mac80211/util.c
> @@ -2498,7 +2498,6 @@ int ieee80211_send_action_csa(struct ieee80211_sub_if_data *sdata,
>  			ifmsh->pre_value++;
>  		put_unaligned_le16(ifmsh->pre_value, pos);/* Precedence Value */
>  		pos += 2;
> -		ifmsh->chsw_init = true;

Where did this go? I'm asking mostly because mac80211.git has a patch
that changed this code so I'm wondering how to resolve the conflict ...

johannes

Re: [PATCH v8 3/5] mac80211: refactor ieee80211_mesh_process_chanswitch()

[Coelho, Luciano]

T24gVGh1LCAyMDEzLTEyLTA1IGF0IDE3OjA1ICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBUaHUsIDIwMTMtMTItMDUgYXQgMTc6MjUgKzAyMDAsIEx1Y2lhbm8gQ29lbGhvIHdyb3Rl
Og0KPiANCj4gPiArKysgYi9uZXQvbWFjODAyMTEvdXRpbC5jDQo+ID4gQEAgLTI0OTgsNyArMjQ5
OCw2IEBAIGludCBpZWVlODAyMTFfc2VuZF9hY3Rpb25fY3NhKHN0cnVjdCBpZWVlODAyMTFfc3Vi
X2lmX2RhdGEgKnNkYXRhLA0KPiA+ICAJCQlpZm1zaC0+cHJlX3ZhbHVlKys7DQo+ID4gIAkJcHV0
X3VuYWxpZ25lZF9sZTE2KGlmbXNoLT5wcmVfdmFsdWUsIHBvcyk7LyogUHJlY2VkZW5jZSBWYWx1
ZSAqLw0KPiA+ICAJCXBvcyArPSAyOw0KPiA+IC0JCWlmbXNoLT5jaHN3X2luaXQgPSB0cnVlOw0K
PiANCj4gV2hlcmUgZGlkIHRoaXMgZ28/IEknbSBhc2tpbmcgbW9zdGx5IGJlY2F1c2UgbWFjODAy
MTEuZ2l0IGhhcyBhIHBhdGNoDQo+IHRoYXQgY2hhbmdlZCB0aGlzIGNvZGUgc28gSSdtIHdvbmRl
cmluZyBob3cgdG8gcmVzb2x2ZSB0aGUgY29uZmxpY3QgLi4uDQoNClRoaXMgYmVjYW1lIHRoZSBp
Zm1zaC0+Y3NhX3JvbGUuICBpZm1zaC0+Y2hzd19pbml0ID09IHRydWUgaXMgbm93DQppZm1zaC0+
Y3NhX3JvbGUgPT0gSUVFRTgwMjExX01FU0hfQ1NBX1JPTEVfSU5JVC4NCg0KLS0NCkx1Y2EuDQo=

[PATCH v8 4/5] mac80211: align ieee80211_mesh_csa_beacon() with ieee80211_csa_beacon()

[Luciano Coelho]

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

Change-Id: If4c111fd874a3e17a5df98d306f7f1c8fcaa6a1a
Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 net/mac80211/mesh.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 980cc12..5476ad9 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -1089,12 +1089,10 @@ int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
 		return ret;
 	}
 
-	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON);
-
 	if (csa_action)
 		ieee80211_send_action_csa(sdata, csa_settings);
 
-	return 0;
+	return BSS_CHANGED_BEACON;
 }
 
 static int mesh_fwd_csa_frame(struct ieee80211_sub_if_data *sdata,
-- 
1.8.4.3

Re: [PATCH v8 4/5] mac80211: align ieee80211_mesh_csa_beacon() with ieee80211_csa_beacon()

[Johannes Berg]

On Thu, 2013-12-05 at 17:25 +0200, Luciano Coelho wrote:
> The return value of ieee80211_mesh_csa_beacon is not aligned with the
> return value of ieee80211_csa_beacon() and
> ieee80211_ibss_csa_beacon().  For consistency and to be able to use
> both functions with similar code, change ieee80211_mesh_csa_beacon()
> not to send the bss changed notification itself, but return what has
> changed so the caller can send the notification instead.

It seems to me that the caller should be updated?

johannes

Re: [PATCH v8 4/5] mac80211: align ieee80211_mesh_csa_beacon() with ieee80211_csa_beacon()

[Coelho, Luciano]

T24gVGh1LCAyMDEzLTEyLTA1IGF0IDE3OjA0ICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBUaHUsIDIwMTMtMTItMDUgYXQgMTc6MjUgKzAyMDAsIEx1Y2lhbm8gQ29lbGhvIHdyb3Rl
Og0KPiA+IFRoZSByZXR1cm4gdmFsdWUgb2YgaWVlZTgwMjExX21lc2hfY3NhX2JlYWNvbiBpcyBu
b3QgYWxpZ25lZCB3aXRoIHRoZQ0KPiA+IHJldHVybiB2YWx1ZSBvZiBpZWVlODAyMTFfY3NhX2Jl
YWNvbigpIGFuZA0KPiA+IGllZWU4MDIxMV9pYnNzX2NzYV9iZWFjb24oKS4gIEZvciBjb25zaXN0
ZW5jeSBhbmQgdG8gYmUgYWJsZSB0byB1c2UNCj4gPiBib3RoIGZ1bmN0aW9ucyB3aXRoIHNpbWls
YXIgY29kZSwgY2hhbmdlIGllZWU4MDIxMV9tZXNoX2NzYV9iZWFjb24oKQ0KPiA+IG5vdCB0byBz
ZW5kIHRoZSBic3MgY2hhbmdlZCBub3RpZmljYXRpb24gaXRzZWxmLCBidXQgcmV0dXJuIHdoYXQg
aGFzDQo+ID4gY2hhbmdlZCBzbyB0aGUgY2FsbGVyIGNhbiBzZW5kIHRoZSBub3RpZmljYXRpb24g
aW5zdGVhZC4NCj4gDQo+IEl0IHNlZW1zIHRvIG1lIHRoYXQgdGhlIGNhbGxlciBzaG91bGQgYmUg
dXBkYXRlZD8NCg0KV2l0aCB0aGlzIHBhdGNoIG9ubHksIHRoZSBjYWxsZXIganVzdCBjaGVja3Mg
aWYgdGhlIHJldHVybiB2YWx1ZSBpcyA8IDANCihpZS4gYW4gZXJyb3IpLCBvdGhlcndpc2UgaXQg
YXNzdW1lcyBpdCB3YXMgb2theS4gIFdpdGggdGhpcyBwYXRjaCwgd2UNCnN0aWxsIHJldHVybiA8
IDAgb24gZXJyb3JzLCBidXQgd2UgcmV0dXJuIHBvc2l0aXZlLCBzYXlpbmcgdGhhdCB0aGUNCmJl
YWNvbiBoYXMgY2hhbmdlZC4NCg0KTXkgbmV4dCBwYXRjaCAoNS81KSBjaGFuZ2VzIHRoZSBjYWxs
ZXIgc28gdGhhdCB0aGUgdmFsdWUgaXMgdXNlZA0KcHJvcGVybHkgKGllLiBPUmluZyB0aGUgcmV0
dXJuIHZhbHVlIHRvIHRoZSBjaGFuZ2VkIHZhcmlhYmxlKS4NCg0KLS0NCkx1Y2EuDQo=

Re: [PATCH v8 4/5] mac80211: align ieee80211_mesh_csa_beacon() with ieee80211_csa_beacon()

[Johannes Berg]

On Thu, 2013-12-05 at 18:53 +0000, Coelho, Luciano wrote:
> On Thu, 2013-12-05 at 17:04 +0100, Johannes Berg wrote:
> > On Thu, 2013-12-05 at 17:25 +0200, Luciano Coelho wrote:
> > > The return value of ieee80211_mesh_csa_beacon is not aligned with the
> > > return value of ieee80211_csa_beacon() and
> > > ieee80211_ibss_csa_beacon().  For consistency and to be able to use
> > > both functions with similar code, change ieee80211_mesh_csa_beacon()
> > > not to send the bss changed notification itself, but return what has
> > > changed so the caller can send the notification instead.
> > 
> > It seems to me that the caller should be updated?
> 
> With this patch only, the caller just checks if the return value is < 0
> (ie. an error), otherwise it assumes it was okay.  With this patch, we
> still return < 0 on errors, but we return positive, saying that the
> beacon has changed.
> 
> My next patch (5/5) changes the caller so that the value is used
> properly (ie. ORing the return value to the changed variable).

But doesn't that leave this patch broken?

johannes

Re: [PATCH v8 4/5] mac80211: align ieee80211_mesh_csa_beacon() with ieee80211_csa_beacon()

[Coelho, Luciano]

T24gVGh1LCAyMDEzLTEyLTA1IGF0IDE5OjUzICswMTAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBUaHUsIDIwMTMtMTItMDUgYXQgMTg6NTMgKzAwMDAsIENvZWxobywgTHVjaWFubyB3cm90
ZToNCj4gPiBPbiBUaHUsIDIwMTMtMTItMDUgYXQgMTc6MDQgKzAxMDAsIEpvaGFubmVzIEJlcmcg
d3JvdGU6DQo+ID4gPiBPbiBUaHUsIDIwMTMtMTItMDUgYXQgMTc6MjUgKzAyMDAsIEx1Y2lhbm8g
Q29lbGhvIHdyb3RlOg0KPiA+ID4gPiBUaGUgcmV0dXJuIHZhbHVlIG9mIGllZWU4MDIxMV9tZXNo
X2NzYV9iZWFjb24gaXMgbm90IGFsaWduZWQgd2l0aCB0aGUNCj4gPiA+ID4gcmV0dXJuIHZhbHVl
IG9mIGllZWU4MDIxMV9jc2FfYmVhY29uKCkgYW5kDQo+ID4gPiA+IGllZWU4MDIxMV9pYnNzX2Nz
YV9iZWFjb24oKS4gIEZvciBjb25zaXN0ZW5jeSBhbmQgdG8gYmUgYWJsZSB0byB1c2UNCj4gPiA+
ID4gYm90aCBmdW5jdGlvbnMgd2l0aCBzaW1pbGFyIGNvZGUsIGNoYW5nZSBpZWVlODAyMTFfbWVz
aF9jc2FfYmVhY29uKCkNCj4gPiA+ID4gbm90IHRvIHNlbmQgdGhlIGJzcyBjaGFuZ2VkIG5vdGlm
aWNhdGlvbiBpdHNlbGYsIGJ1dCByZXR1cm4gd2hhdCBoYXMNCj4gPiA+ID4gY2hhbmdlZCBzbyB0
aGUgY2FsbGVyIGNhbiBzZW5kIHRoZSBub3RpZmljYXRpb24gaW5zdGVhZC4NCj4gPiA+IA0KPiA+
ID4gSXQgc2VlbXMgdG8gbWUgdGhhdCB0aGUgY2FsbGVyIHNob3VsZCBiZSB1cGRhdGVkPw0KPiA+
IA0KPiA+IFdpdGggdGhpcyBwYXRjaCBvbmx5LCB0aGUgY2FsbGVyIGp1c3QgY2hlY2tzIGlmIHRo
ZSByZXR1cm4gdmFsdWUgaXMgPCAwDQo+ID4gKGllLiBhbiBlcnJvciksIG90aGVyd2lzZSBpdCBh
c3N1bWVzIGl0IHdhcyBva2F5LiAgV2l0aCB0aGlzIHBhdGNoLCB3ZQ0KPiA+IHN0aWxsIHJldHVy
biA8IDAgb24gZXJyb3JzLCBidXQgd2UgcmV0dXJuIHBvc2l0aXZlLCBzYXlpbmcgdGhhdCB0aGUN
Cj4gPiBiZWFjb24gaGFzIGNoYW5nZWQuDQo+ID4gDQo+ID4gTXkgbmV4dCBwYXRjaCAoNS81KSBj
aGFuZ2VzIHRoZSBjYWxsZXIgc28gdGhhdCB0aGUgdmFsdWUgaXMgdXNlZA0KPiA+IHByb3Blcmx5
IChpZS4gT1JpbmcgdGhlIHJldHVybiB2YWx1ZSB0byB0aGUgY2hhbmdlZCB2YXJpYWJsZSkuDQo+
IA0KPiBCdXQgZG9lc24ndCB0aGF0IGxlYXZlIHRoaXMgcGF0Y2ggYnJva2VuPw0KDQpOby4gIFdl
IHVzZWQgdG8gcmV0dXJuIDAgaW4gY2FzZSBvZiBzdWNjZXNzIChvciAtRU5PTUVNIG9uIGZhaWx1
cmUpLg0KVGhlIGNhbGxlciBjaGVja3MgdGhlIHJldHVybiB2YWx1ZSBsaWtlIHRoaXM6DQoNCgkJ
ZXJyID0gaWVlZTgwMjExX21lc2hfY3NhX2JlYWNvbihzZGF0YSwgcGFyYW1zLA0KCQkJKGlmbXNo
LT5jc2Ffcm9sZSA9PSBJRUVFODAyMTFfTUVTSF9DU0FfUk9MRV9JTklUKSk7DQoJCWlmIChlcnIg
PCAwKSB7DQoJCQlpZm1zaC0+Y3NhX3JvbGUgPSBJRUVFODAyMTFfTUVTSF9DU0FfUk9MRV9OT05F
Ow0KCQkJcmV0dXJuIGVycjsNCgkJfQ0KDQpXaXRoIG15IGNoYW5nZSwgaW5zdGVhZCBvZiByZXR1
cm5pbmcgMCBvbiBzdWNjZXNzLCB3ZSByZXR1cm4NCkJTU19DSEFOR0VEX0JFQUNPTi4gIFRoZSBp
ZiAoZXJyIDwgMCkgd2lsbCBzdGlsbCBldmFsdWF0ZSB0byBmYWxzZSwgc28NCml0IHdpbGwgYmUg
dGhlIHNhbWUgYXMgYmVmb3JlLg0KDQpOb3RlIHRoYXQgdGhpcyBwYXRjaCBpcyBvbiB0b3Agb2Yg
dGhlICJtYWM4MDIxMTogcmVmYWN0b3INCmllZWU4MDIxMV9tZXNoX2NoYW5zd2l0Y2goKSIuDQoN
Ci0tDQpMdWNhLg0K

[PATCH v8 5/5] mac80211: only set CSA beacon when at least one beacon must be transmitted

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

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>
---
 include/net/mac80211.h     |  10 ++--
 net/mac80211/cfg.c         | 113 ++++++++++++++++++++++++++++++++++-----------
 net/mac80211/ibss.c        |   6 ---
 net/mac80211/ieee80211_i.h |   3 +-
 net/mac80211/mesh.c        |   9 ++--
 net/mac80211/tx.c          |  19 +++-----
 6 files changed, 104 insertions(+), 56 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c014acc..5cc9a79 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2730,11 +2730,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
@@ -3429,13 +3431,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/cfg.c b/net/mac80211/cfg.c
index 790c15d..ff7aa17 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2982,21 +2982,19 @@ 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 =
-		container_of(work, struct ieee80211_sub_if_data,
-			     csa_finalize_work);
-	struct ieee80211_local *local = sdata->local;
-	int err, changed = 0;
+	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
 
-	sdata_lock(sdata);
-	/* AP might have been stopped while waiting for the lock. */
-	if (!sdata->vif.csa_active)
-		goto unlock;
+	ieee80211_queue_work(&sdata->local->hw,
+			     &sdata->csa_finalize_work);
+}
+EXPORT_SYMBOL(ieee80211_csa_finish);
 
-	if (!ieee80211_sdata_running(sdata))
-		goto unlock;
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	int err, changed = 0;
 
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
@@ -3048,6 +3046,29 @@ unlock:
 	sdata_unlock(sdata);
 }
 
+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);
+
+	sdata_lock(sdata);
+	/* AP might have been stopped while waiting for the lock. */
+	if (!sdata->vif.csa_active)
+		goto unlock;
+
+	if (!ieee80211_sdata_running(sdata))
+		goto unlock;
+
+	if (!ieee80211_sdata_running(sdata))
+		return;
+
+	ieee80211_csa_finalize(sdata);
+
+unlock:
+	sdata_unlock(sdata);
+}
+
 int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 			     struct cfg80211_csa_settings *params)
 {
@@ -3056,7 +3077,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;
 
 	lockdep_assert_held(&sdata->wdev.mtx);
 
@@ -3097,19 +3118,40 @@ 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)
+			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)
@@ -3137,9 +3179,16 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		    params->chandef.chan->band)
 			return -EINVAL;
 
-		err = ieee80211_ibss_csa_beacon(sdata, params);
-		if (err < 0)
-			return err;
+		/* see comments in the NL80211_IFTYPE_AP block */
+		if (params->count > 1) {
+			err = ieee80211_ibss_csa_beacon(sdata, params);
+			if (err < 0)
+				return err;
+			changed |= err;
+		}
+
+		ieee80211_send_action_csa(sdata, params);
+
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
@@ -3159,12 +3208,19 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_NONE)
 			ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_INIT;
 
-		err = ieee80211_mesh_csa_beacon(sdata, params,
-			(ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT));
-		if (err < 0) {
-			ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
-			return err;
+		/* see comments in the NL80211_IFTYPE_AP block */
+		if (params->count > 1) {
+			err = ieee80211_mesh_csa_beacon(sdata, params);
+			if (err < 0) {
+				ifmsh->csa_role = IEEE80211_MESH_CSA_ROLE_NONE;
+				return err;
+			}
+			changed |= err;
 		}
+
+		if (ifmsh->csa_role == IEEE80211_MESH_CSA_ROLE_INIT)
+			ieee80211_send_action_csa(sdata, params);
+
 		break;
 #endif
 	default:
@@ -3181,8 +3237,13 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	sdata->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);
+	} else {
+		/* if the beacon didn't change, we can finalize immediately */
+		ieee80211_csa_finalize(sdata);
+	}
 
 	return 0;
 }
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 23e035f..595902c 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -516,12 +516,6 @@ int ieee80211_ibss_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	if (old_presp)
 		kfree_rcu(old_presp, rcu_head);
 
-	/* it might not send the beacon for a while. send an action frame
-	 * immediately to announce the channel switch.
-	 */
-	if (csa_settings)
-		ieee80211_send_action_csa(sdata, csa_settings);
-
 	return BSS_CHANGED_BEACON;
  out:
 	return ret;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index cf38a74..9b5f1bc 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1400,8 +1400,7 @@ void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata);
 void ieee80211_mesh_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 				   struct sk_buff *skb);
 int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
-			      struct cfg80211_csa_settings *csa_settings,
-			      bool csa_action);
+			      struct cfg80211_csa_settings *csa_settings);
 int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata);
 
 /* scan/BSS handling */
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 5476ad9..a7fad0d 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -1051,7 +1051,8 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
 	/* Remove the CSA and MCSP elements from the beacon */
 	tmp_csa_settings = rcu_dereference(ifmsh->csa);
 	rcu_assign_pointer(ifmsh->csa, NULL);
-	kfree_rcu(tmp_csa_settings, rcu_head);
+	if (tmp_csa_settings)
+		kfree_rcu(tmp_csa_settings, rcu_head);
 	ret = ieee80211_mesh_rebuild_beacon(sdata);
 	if (ret)
 		return -EINVAL;
@@ -1064,8 +1065,7 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
 }
 
 int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
-			      struct cfg80211_csa_settings *csa_settings,
-			      bool csa_action)
+			      struct cfg80211_csa_settings *csa_settings)
 {
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
 	struct mesh_csa_settings *tmp_csa_settings;
@@ -1089,9 +1089,6 @@ int ieee80211_mesh_csa_beacon(struct ieee80211_sub_if_data *sdata,
 		return ret;
 	}
 
-	if (csa_action)
-		ieee80211_send_action_csa(sdata, csa_settings);
-
 	return BSS_CHANGED_BEACON;
 }
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 6d59e21..b86a679 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2397,15 +2397,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)
 {
@@ -2434,8 +2425,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]--;
@@ -2501,7 +2496,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.3

Re: [PATCH v8 5/5] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Simon Wunderlich]

Hey Luca,

let me add first that there is another locking regression introduced by my 
latest fixes for the IBSS mode. I'll send a fix in a minute, please include it 
when you test/rebase.

Appearently there are a few locking problems in your patch too (do you have a 
chance to test it, somehow?), see comments inline:

> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -2982,21 +2982,19 @@ 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 =
> -		container_of(work, struct ieee80211_sub_if_data,
> -			     csa_finalize_work);
> -	struct ieee80211_local *local = sdata->local;
> -	int err, changed = 0;
> +	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> 
> -	sdata_lock(sdata);
> -	/* AP might have been stopped while waiting for the lock. */
> -	if (!sdata->vif.csa_active)
> -		goto unlock;
> +	ieee80211_queue_work(&sdata->local->hw,
> +			     &sdata->csa_finalize_work);
> +}
> +EXPORT_SYMBOL(ieee80211_csa_finish);
> 
> -	if (!ieee80211_sdata_running(sdata))
> -		goto unlock;
> +static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	int err, changed = 0;
> 
>  	sdata->radar_required = sdata->csa_radar_required;
>  	err = ieee80211_vif_change_channel(sdata, &changed);

This hunk is a little hard to understand, but at least in the resulting file I 
have here, csa_finalize() does sdata_unlock but never locks it. Lockdep 
complains about locking imbalance, and actually that's true ...

> @@ -3048,6 +3046,29 @@ unlock:
>  	sdata_unlock(sdata);
>  }
> 
> +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);
> +
> +	sdata_lock(sdata);
> +	/* AP might have been stopped while waiting for the lock. */
> +	if (!sdata->vif.csa_active)
> +		goto unlock;
> +
> +	if (!ieee80211_sdata_running(sdata))
> +		goto unlock;
> +
> +	if (!ieee80211_sdata_running(sdata))
> +		return;

Why are you checking the same thing twice? The second time without unlocking, 
that's unsafe too ...

> +
> +	ieee80211_csa_finalize(sdata);
> +
> +unlock:
> +	sdata_unlock(sdata);
> +}
> +
>  int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>  			     struct cfg80211_csa_settings *params)
>  {

Cheers,
    Simon

Re: [PATCH v8 5/5] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Yeoh Chun-Yeow]

> diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
> index 5476ad9..a7fad0d 100644
> --- a/net/mac80211/mesh.c
> +++ b/net/mac80211/mesh.c
> @@ -1051,7 +1051,8 @@ int ieee80211_mesh_finish_csa(struct ieee80211_sub_if_data *sdata)
>         /* Remove the CSA and MCSP elements from the beacon */
>         tmp_csa_settings = rcu_dereference(ifmsh->csa);
>         rcu_assign_pointer(ifmsh->csa, NULL);
> -       kfree_rcu(tmp_csa_settings, rcu_head);
> +       if (tmp_csa_settings)
> +               kfree_rcu(tmp_csa_settings, rcu_head);

Yes, this solves the problem of kernel oops.

----
Chun-Yeow

Re: [PATCH v8 5/5] mac80211: only set CSA beacon when at least one beacon must be transmitted

[Coelho, Luciano]

T24gRnJpLCAyMDEzLTEyLTA2IGF0IDE4OjM0ICswODAwLCBZZW9oIENodW4tWWVvdyB3cm90ZToN
Cj4gPiBkaWZmIC0tZ2l0IGEvbmV0L21hYzgwMjExL21lc2guYyBiL25ldC9tYWM4MDIxMS9tZXNo
LmMNCj4gPiBpbmRleCA1NDc2YWQ5Li5hN2ZhZDBkIDEwMDY0NA0KPiA+IC0tLSBhL25ldC9tYWM4
MDIxMS9tZXNoLmMNCj4gPiArKysgYi9uZXQvbWFjODAyMTEvbWVzaC5jDQo+ID4gQEAgLTEwNTEs
NyArMTA1MSw4IEBAIGludCBpZWVlODAyMTFfbWVzaF9maW5pc2hfY3NhKHN0cnVjdCBpZWVlODAy
MTFfc3ViX2lmX2RhdGEgKnNkYXRhKQ0KPiA+ICAgICAgICAgLyogUmVtb3ZlIHRoZSBDU0EgYW5k
IE1DU1AgZWxlbWVudHMgZnJvbSB0aGUgYmVhY29uICovDQo+ID4gICAgICAgICB0bXBfY3NhX3Nl
dHRpbmdzID0gcmN1X2RlcmVmZXJlbmNlKGlmbXNoLT5jc2EpOw0KPiA+ICAgICAgICAgcmN1X2Fz
c2lnbl9wb2ludGVyKGlmbXNoLT5jc2EsIE5VTEwpOw0KPiA+IC0gICAgICAga2ZyZWVfcmN1KHRt
cF9jc2Ffc2V0dGluZ3MsIHJjdV9oZWFkKTsNCj4gPiArICAgICAgIGlmICh0bXBfY3NhX3NldHRp
bmdzKQ0KPiA+ICsgICAgICAgICAgICAgICBrZnJlZV9yY3UodG1wX2NzYV9zZXR0aW5ncywgcmN1
X2hlYWQpOw0KPiANCj4gWWVzLCB0aGlzIHNvbHZlcyB0aGUgcHJvYmxlbSBvZiBrZXJuZWwgb29w
cy4NCg0KR3JlYXQsIHRoYW5rcyBmb3IgdGVzdGluZyENCg0KSSdsbCBzZW5kIGEgdjkgd2l0aCBm
aXhlcyBmb3IgU2ltb24ncyBjb21tZW50cyBhbmQgcmViYXNlIGl0IG9uIHRvcCBvZg0KaGlzIGxv
Y2sgZml4IGFzIHdlbGwuICBUb2RheSBpcyBob2xpZGF5IGhlcmUsIHNvIHByb2JhYmx5IG9uIE1v
bmRheS4NCg0KLS0NCkx1Y2EuDQoNCg0K