[PATCH 0/4] mac80211/cfg80211: CSA fixes/cleanups

[PATCH 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Michal Kazior]

Hi,

The patchset fixes CSA tx queue locking and
introduces interface teardown upon CSA failure
during finalization.

The patch [3/4] was previously posted as an RFC.
It has been rebased and function documentation
and commit messages were improved since then.

This is based on mac80211-next/master
(5a970df8990d173e7e4092952f2e3da1de69b27d) with
beacon_interval patch[1] applied.

[1]: http://marc.info/?l=linux-wireless&m=139385275518528&w=2


Michal Kazior (4):
  mac80211: fix CSA tx queue locking
  mac80211: split CSA finalize function
  cfg80211: export interface stopping function
  mac80211: disconnect iface if CSA unexpectedly fails

 include/net/cfg80211.h     |  15 +++++
 include/net/mac80211.h     |   4 +-
 net/mac80211/cfg.c         | 153 ++++++++++++++++++++++++++++++++++-----------
 net/mac80211/ieee80211_i.h |   2 +
 net/mac80211/iface.c       |   4 ++
 net/mac80211/mlme.c        |  27 ++++----
 net/wireless/ap.c          |   4 +-
 net/wireless/core.c        |  44 ++++++++++---
 net/wireless/core.h        |   7 +++
 net/wireless/mesh.c        |   4 +-
 net/wireless/trace.h       |  15 +++++
 net/wireless/util.c        |   5 ++
 12 files changed, 222 insertions(+), 62 deletions(-)

-- 
1.8.5.3

[PATCH 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

It was possible for tx queues to be stuck locked
if AP CSA finalization failed. In that case
stop_ap nor do_stop woke the queues up. This means
it was impossible to perform tx at all until
driver was reloaded or a successful CSA was
performed later.

It was possible to solve this in a simpler manner
however this is more robust and future proof
(having multi-vif CSA in mind).

Now csa_active and newly introduced csa_block_tx
are write-protected by both sdata_lock and
local->mtx so either one can be held for read
access. This way it's easy to iterate over all
interfaces to see if there's any interface with an
active CSA.

It is still possible to have tx queues stopped
after CSA failure but as soon as offending
interfaces are stopped from userspace (stop_ap or
ifdown) tx queues are woken up.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 include/net/mac80211.h     |  4 ++-
 net/mac80211/cfg.c         | 83 ++++++++++++++++++++++++++++++++++++++--------
 net/mac80211/ieee80211_i.h |  2 ++
 net/mac80211/iface.c       |  4 +++
 net/mac80211/mlme.c        | 27 ++++++++-------
 5 files changed, 93 insertions(+), 27 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 86faa41..d284411 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1111,7 +1111,9 @@ enum ieee80211_vif_flags {
  * @addr: address of this interface
  * @p2p: indicates whether this AP or STA interface is a p2p
  *	interface, i.e. a GO or p2p-sta respectively
- * @csa_active: marks whether a channel switch is going on
+ * @csa_active: marks whether a channel switch is going on. Internally it is
+ *	write-protected by sdata_lock and local->mtx so holding either is fine
+ *	for read access.
  * @driver_flags: flags/capabilities the driver has for this interface,
  *	these need to be set (or cleared) when the interface is added
  *	or, if supported by the driver, the interface type is changed
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aaa59d7..3dce573 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1075,6 +1075,45 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+static bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	lockdep_assert_held(&local->mtx);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+
+		if (!sdata->vif.csa_active)
+			continue;
+
+		if (!sdata->csa_block_tx)
+			continue;
+
+		rcu_read_unlock();
+		return true;
+	}
+	rcu_read_unlock();
+
+	return false;
+}
+
+void ieee80211_recalc_csa_block_tx(struct ieee80211_local *local)
+{
+	lockdep_assert_held(&local->mtx);
+
+	if (ieee80211_csa_needs_block_tx(local))
+		ieee80211_stop_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	else
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+}
+
 static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -1092,7 +1131,11 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
 
 	/* abort any running channel switch */
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	ieee80211_recalc_csa_block_tx(local);
+	mutex_unlock(&local->mtx);
+
 	kfree(sdata->u.ap.next_beacon);
 	sdata->u.ap.next_beacon = NULL;
 
@@ -3017,11 +3060,10 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	int err, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
-	mutex_lock(&local->mtx);
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
-	mutex_unlock(&local->mtx);
 	if (WARN_ON(err < 0))
 		return;
 
@@ -3062,10 +3104,6 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
-					IEEE80211_MAX_QUEUE_MAP,
-					IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 }
 
@@ -3074,8 +3112,11 @@ 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);
+	struct ieee80211_local *local = sdata->local;
 
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
+
 	/* AP might have been stopped while waiting for the lock. */
 	if (!sdata->vif.csa_active)
 		goto unlock;
@@ -3084,8 +3125,10 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 		goto unlock;
 
 	ieee80211_csa_finalize(sdata);
+	ieee80211_recalc_csa_block_tx(local);
 
 unlock:
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
 }
 
@@ -3212,8 +3255,8 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-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;
@@ -3222,6 +3265,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	int err, num_chanctx, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
 	if (!list_empty(&local->roc_list) || local->scanning)
 		return -EBUSY;
@@ -3263,15 +3307,12 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 
 	sdata->csa_radar_required = params->radar_required;
-
-	if (params->block_tx)
-		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	sdata->csa_chandef = params->chandef;
+	sdata->csa_block_tx = params->block_tx;
 	sdata->vif.csa_active = true;
 
+	ieee80211_recalc_csa_block_tx(local);
+
 	if (changed) {
 		ieee80211_bss_info_change_notify(sdata, changed);
 		drv_channel_switch_beacon(sdata, &params->chandef);
@@ -3283,6 +3324,20 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+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;
+	int err;
+
+	mutex_lock(&local->mtx);
+	err = __ieee80211_channel_switch(wiphy, dev, params);
+	mutex_unlock(&local->mtx);
+
+	return err;
+}
+
 static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 			     struct cfg80211_mgmt_tx_params *params,
 			     u64 *cookie)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8603dfb..75dec8c 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -754,6 +754,7 @@ struct ieee80211_sub_if_data {
 	int csa_counter_offset_beacon;
 	int csa_counter_offset_presp;
 	bool csa_radar_required;
+	bool csa_block_tx; /* write-protected by sdata_lock and local->mtx */
 	struct cfg80211_chan_def csa_chandef;
 
 	/* used to reconfigure hardware SM PS */
@@ -1461,6 +1462,7 @@ void ieee80211_sw_roc_work(struct work_struct *work);
 void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
 
 /* channel switch handling */
+void ieee80211_recalc_csa_block_tx(struct ieee80211_local *local);
 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);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index be198f4..b82ee53 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -829,8 +829,12 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	cancel_work_sync(&sdata->recalc_smps);
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	ieee80211_recalc_csa_block_tx(local);
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
+
 	cancel_work_sync(&sdata->csa_finalize_work);
 
 	cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 46b62bb..0a6e77cf 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -952,15 +952,15 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->csa_chandef.chan;
 
-	/* XXX: wait for a beacon first? */
-	ieee80211_wake_queues_by_reason(&local->hw,
-					IEEE80211_MAX_QUEUE_MAP,
-					IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	ieee80211_bss_info_change_notify(sdata, changed);
 
  out:
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	/* XXX: wait for a beacon first? */
+	ieee80211_recalc_csa_block_tx(local);
+	mutex_unlock(&local->mtx);
+
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
 	sdata_unlock(sdata);
 }
@@ -1077,12 +1077,13 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	mutex_unlock(&local->chanctx_mtx);
 
 	sdata->csa_chandef = csa_ie.chandef;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = true;
+	sdata->csa_block_tx = csa_ie.mode;
 
-	if (csa_ie.mode)
-		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
+	ieee80211_recalc_csa_block_tx(local);
+	mutex_unlock(&local->mtx);
 
 	if (local->ops->channel_switch) {
 		/* use driver's channel switch callback */
@@ -2022,6 +2023,7 @@ EXPORT_SYMBOL(ieee80211_ap_probereq_get);
 
 static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 {
+	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
 
@@ -2035,10 +2037,11 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 			       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
 			       true, frame_buf);
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
-					IEEE80211_MAX_QUEUE_MAP,
-					IEEE80211_QUEUE_STOP_REASON_CSA);
+	ieee80211_recalc_csa_block_tx(local);
+	mutex_unlock(&local->mtx);
 
 	cfg80211_tx_mlme_mgmt(sdata->dev, frame_buf,
 			      IEEE80211_DEAUTH_FRAME_LEN);
-- 
1.8.5.3

Re: [PATCH 1/4] mac80211: fix CSA tx queue locking

[Johannes Berg]

On Wed, 2014-03-05 at 15:27 +0100, Michal Kazior wrote:

> +void ieee80211_recalc_csa_block_tx(struct ieee80211_local *local)
> +{
> +	lockdep_assert_held(&local->mtx);
> +
> +	if (ieee80211_csa_needs_block_tx(local))
> +		ieee80211_stop_queues_by_reason(&local->hw,
> +					IEEE80211_MAX_QUEUE_MAP,
> +					IEEE80211_QUEUE_STOP_REASON_CSA);
> +	else
> +		ieee80211_wake_queues_by_reason(&local->hw,
> +					IEEE80211_MAX_QUEUE_MAP,
> +					IEEE80211_QUEUE_STOP_REASON_CSA);
> +}

I don't like this function, even if technically stop is idempotent, it
still seems strange to call this "recalc".

> @@ -1092,7 +1131,11 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
>  	old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
>  
>  	/* abort any running channel switch */
> +	mutex_lock(&local->mtx);
>  	sdata->vif.csa_active = false;
> +	ieee80211_recalc_csa_block_tx(local);
> +	mutex_unlock(&local->mtx);

In fact, here you don't care about the stop queues part at all, afaict.


>  	sdata->csa_chandef = params->chandef;
> +	sdata->csa_block_tx = params->block_tx;

doesn't that more belong to the previous patch?

johannes

Re: [PATCH 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

On 10 March 2014 17:27, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2014-03-05 at 15:27 +0100, Michal Kazior wrote:
>
>> +void ieee80211_recalc_csa_block_tx(struct ieee80211_local *local)
>> +{
>> +     lockdep_assert_held(&local->mtx);
>> +
>> +     if (ieee80211_csa_needs_block_tx(local))
>> +             ieee80211_stop_queues_by_reason(&local->hw,
>> +                                     IEEE80211_MAX_QUEUE_MAP,
>> +                                     IEEE80211_QUEUE_STOP_REASON_CSA);
>> +     else
>> +             ieee80211_wake_queues_by_reason(&local->hw,
>> +                                     IEEE80211_MAX_QUEUE_MAP,
>> +                                     IEEE80211_QUEUE_STOP_REASON_CSA);
>> +}
>
> I don't like this function, even if technically stop is idempotent, it
> still seems strange to call this "recalc".
>
>> @@ -1092,7 +1131,11 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
>>       old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
>>
>>       /* abort any running channel switch */
>> +     mutex_lock(&local->mtx);
>>       sdata->vif.csa_active = false;
>> +     ieee80211_recalc_csa_block_tx(local);
>> +     mutex_unlock(&local->mtx);
>
> In fact, here you don't care about the stop queues part at all, afaict.

I wanted to make the stop/wake code decision in one place. I
understand you prefer to have:

 if (can_wake)
  wake()

and

 if (!can_wake)
  stop()

calls explicitly instead of the "recalc" function?


>>       sdata->csa_chandef = params->chandef;
>> +     sdata->csa_block_tx = params->block_tx;
>
> doesn't that more belong to the previous patch?

It belongs here.

CSA tx queue blocking is global. If you want to support multi- CSA
then you need to keep queues blocked until last CSA that requires tx
being blocked is completed. To do that you need to store which
interface CSA requested block_tx.

I should probably include this note in the commit message.


Michał

Re: [PATCH 1/4] mac80211: fix CSA tx queue locking

[Johannes Berg]

On Tue, 2014-03-11 at 08:20 +0100, Michal Kazior wrote:

> > I don't like this function, even if technically stop is idempotent, it
> > still seems strange to call this "recalc".

> I wanted to make the stop/wake code decision in one place. I
> understand you prefer to have:
> 
>  if (can_wake)
>   wake()
> 
> and
> 
>  if (!can_wake)
>   stop()
> 
> calls explicitly instead of the "recalc" function?

Yes, I think that would be easier to understand? With the "recalc" you
have to understand that it's idempotent.

> >>       sdata->csa_chandef = params->chandef;
> >> +     sdata->csa_block_tx = params->block_tx;
> >
> > doesn't that more belong to the previous patch?
> 
> It belongs here.
> 
> CSA tx queue blocking is global. If you want to support multi- CSA
> then you need to keep queues blocked until last CSA that requires tx
> being blocked is completed. To do that you need to store which
> interface CSA requested block_tx.
> 
> I should probably include this note in the commit message.

Ok :)

johannes

[PATCH 2/4] mac80211: split CSA finalize function

[Michal Kazior]

Improves readability and modularity.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 61 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 3dce573..4654823 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3054,25 +3054,11 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
 }
 EXPORT_SYMBOL(ieee80211_csa_finish);
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
+					  u32 *changed)
 {
-	struct ieee80211_local *local = sdata->local;
-	int err, changed = 0;
-
-	sdata_assert_lock(sdata);
-	lockdep_assert_held(&local->mtx);
-
-	sdata->radar_required = sdata->csa_radar_required;
-	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (WARN_ON(err < 0))
-		return;
-
-	if (!local->use_chanctx) {
-		local->_oper_chandef = sdata->csa_chandef;
-		ieee80211_hw_config(local, 0);
-	}
+	int err;
 
-	sdata->vif.csa_active = false;
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
 		err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
@@ -3080,30 +3066,57 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		sdata->u.ap.next_beacon = NULL;
 
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		err = ieee80211_ibss_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
 		err = ieee80211_mesh_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #endif
 	default:
 		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 changed = 0;
+	int err;
+
+	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
+
+	sdata->radar_required = sdata->csa_radar_required;
+	err = ieee80211_vif_change_channel(sdata, &changed);
+	if (WARN_ON(err < 0))
 		return;
+
+	if (!local->use_chanctx) {
+		local->_oper_chandef = sdata->csa_chandef;
+		ieee80211_hw_config(local, 0);
 	}
 
-	ieee80211_bss_info_change_notify(sdata, changed);
+	sdata->vif.csa_active = false;
+
+	err = ieee80211_set_after_csa_beacon(sdata, &changed);
+	if (err)
+		return;
 
+	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 }
 
-- 
1.8.5.3

[PATCH 3/4] cfg80211: export interface stopping function

[Michal Kazior]

This exports a new cfg80211_stop_iface() function.

This is intended for driver internal interface
combination management and channel switching.

Due to locking issues (it re-enters driver) the
call is asynchronous and uses cfg80211 event
list/worker.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 include/net/cfg80211.h | 15 +++++++++++++++
 net/wireless/ap.c      |  4 ++--
 net/wireless/core.c    | 44 +++++++++++++++++++++++++++++++++++++-------
 net/wireless/core.h    |  7 +++++++
 net/wireless/mesh.c    |  4 ++--
 net/wireless/trace.h   | 15 +++++++++++++++
 net/wireless/util.c    |  5 +++++
 7 files changed, 83 insertions(+), 11 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff3af16..c52a787 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4682,6 +4682,21 @@ void cfg80211_crit_proto_stopped(struct wireless_dev *wdev, gfp_t gfp);
  */
 unsigned int ieee80211_get_num_supported_channels(struct wiphy *wiphy);
 
+/**
+ * cfg80211_stop_iface - stop given interface
+ *
+ * @wiphy: the wiphy
+ * @wdev: wireless device
+ *
+ * Stop interface as if AP was stopped, IBSS/mesh left, STA disconnected.
+ *
+ * This is intended for channel switching and internal driver interface
+ * combination management.
+ *
+ * Note: This doesn't need any locks and is asynchronous.
+ */
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/wireless/ap.c b/net/wireless/ap.c
index 3e02ade..bdad1f9 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -6,8 +6,8 @@
 #include "rdev-ops.h"
 
 
-static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
-			      struct net_device *dev, bool notify)
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 086cddd..8b36954 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -751,23 +751,23 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 		rdev->num_running_monitor_ifaces += num;
 }
 
-void cfg80211_leave(struct cfg80211_registered_device *rdev,
-		    struct wireless_dev *wdev)
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev)
 {
 	struct net_device *dev = wdev->netdev;
 
 	ASSERT_RTNL();
+	ASSERT_WDEV_LOCK(wdev);
 
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
-		cfg80211_leave_ibss(rdev, dev, true);
+		__cfg80211_leave_ibss(rdev, dev, true);
 		break;
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_STATION:
 		if (rdev->sched_scan_req && dev == rdev->sched_scan_req->dev)
 			__cfg80211_stop_sched_scan(rdev, false);
 
-		wdev_lock(wdev);
 #ifdef CONFIG_CFG80211_WEXT
 		kfree(wdev->wext.ie);
 		wdev->wext.ie = NULL;
@@ -776,20 +776,50 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 #endif
 		cfg80211_disconnect(rdev, dev,
 				    WLAN_REASON_DEAUTH_LEAVING, true);
-		wdev_unlock(wdev);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:
-		cfg80211_leave_mesh(rdev, dev);
+		__cfg80211_leave_mesh(rdev, dev);
 		break;
 	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_P2P_GO:
-		cfg80211_stop_ap(rdev, dev, true);
+		__cfg80211_stop_ap(rdev, dev, true);
 		break;
 	default:
 		break;
 	}
 }
 
+void cfg80211_leave(struct cfg80211_registered_device *rdev,
+		    struct wireless_dev *wdev)
+{
+	ASSERT_RTNL();
+
+	wdev_lock(wdev);
+	__cfg80211_leave(rdev, wdev);
+	wdev_unlock(wdev);
+}
+
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
+{
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+	struct cfg80211_event *ev;
+	unsigned long flags;
+
+	trace_cfg80211_stop_iface(wiphy, wdev);
+
+	ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+	if (!ev)
+		return;
+
+	ev->type = EVENT_STOPPED;
+
+	spin_lock_irqsave(&wdev->event_lock, flags);
+	list_add_tail(&ev->list, &wdev->event_list);
+	spin_unlock_irqrestore(&wdev->event_lock, flags);
+	queue_work(cfg80211_wq, &rdev->event_work);
+}
+EXPORT_SYMBOL(cfg80211_stop_iface);
+
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 					 unsigned long state, void *ptr)
 {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 3975ffa..c652763f 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -182,6 +182,7 @@ enum cfg80211_event_type {
 	EVENT_ROAMED,
 	EVENT_DISCONNECTED,
 	EVENT_IBSS_JOINED,
+	EVENT_STOPPED,
 };
 
 struct cfg80211_event {
@@ -275,6 +276,8 @@ int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 		       struct net_device *dev,
 		       struct mesh_setup *setup,
 		       const struct mesh_config *conf);
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev);
 int cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
 			struct net_device *dev);
 int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
@@ -282,6 +285,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 			      struct cfg80211_chan_def *chandef);
 
 /* AP */
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify);
 int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
 		     struct net_device *dev, bool notify);
 
@@ -464,6 +469,8 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 			       enum nl80211_iftype iftype, int num);
 
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev);
 void cfg80211_leave(struct cfg80211_registered_device *rdev,
 		    struct wireless_dev *wdev);
 
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index 5af5cc6..b5c304e 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -262,8 +262,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 	return 0;
 }
 
-static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
-				 struct net_device *dev)
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index aabccf1..24acf96 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2615,6 +2615,21 @@ TRACE_EVENT(cfg80211_ft_event,
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(target_ap))
 );
 
+TRACE_EVENT(cfg80211_stop_iface,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
+	TP_ARGS(wiphy, wdev),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		WDEV_ENTRY
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		WDEV_ASSIGN;
+	),
+	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
+		  WIPHY_PR_ARG, WDEV_PR_ARG)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/net/wireless/util.c b/net/wireless/util.c
index dadc934..fa59606 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -789,6 +789,8 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
 	unsigned long flags;
 	const u8 *bssid = NULL;
 
+	ASSERT_RTNL();
+
 	spin_lock_irqsave(&wdev->event_lock, flags);
 	while (!list_empty(&wdev->event_list)) {
 		ev = list_first_entry(&wdev->event_list,
@@ -823,6 +825,9 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
 			__cfg80211_ibss_joined(wdev->netdev, ev->ij.bssid,
 					       ev->ij.channel);
 			break;
+		case EVENT_STOPPED:
+			__cfg80211_leave(wiphy_to_dev(wdev->wiphy), wdev);
+			break;
 		}
 		wdev_unlock(wdev);
 
-- 
1.8.5.3

[PATCH 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

It doesn't make much sense to leave a cripled
interface running.

As a side effect this will unblock tx queues with
CSA reason immediately after failure instead of
until after userspace requests interface to stop.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 4654823..e174cc3 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3091,7 +3091,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
@@ -3103,7 +3103,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
 	if (WARN_ON(err < 0))
-		return;
+		return err;
 
 	if (!local->use_chanctx) {
 		local->_oper_chandef = sdata->csa_chandef;
@@ -3114,10 +3114,12 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	err = ieee80211_set_after_csa_beacon(sdata, &changed);
 	if (err)
-		return;
+		return err;
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
+
+	return 0;
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3126,6 +3128,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 		container_of(work, struct ieee80211_sub_if_data,
 			     csa_finalize_work);
 	struct ieee80211_local *local = sdata->local;
+	int err;
 
 	sdata_lock(sdata);
 	mutex_lock(&local->mtx);
@@ -3137,7 +3140,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	if (!ieee80211_sdata_running(sdata))
 		goto unlock;
 
-	ieee80211_csa_finalize(sdata);
+	err = ieee80211_csa_finalize(sdata);
+	if (err) {
+		sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
+		cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev);
+		goto unlock;
+	}
+
 	ieee80211_recalc_csa_block_tx(local);
 
 unlock:
-- 
1.8.5.3

[PATCH v2 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Michal Kazior]

Hi,

The patchset fixes CSA tx queue locking and
introduces interface teardown upon CSA failure
during finalization.

I originally posted this some time ago.

First two patches have been included in my recent
RFC for general multi-vif csa changeset overview
purposes. Hopefully this isn't too confusing..

v2:
 * get rid of idempotency

Michal Kazior (4):
  mac80211: fix CSA tx queue locking
  mac80211: split CSA finalize function
  cfg80211: export interface stopping function
  mac80211: disconnect iface if CSA unexpectedly fails

 include/net/cfg80211.h     |  15 +++++
 include/net/mac80211.h     |   4 +-
 net/mac80211/cfg.c         | 148 +++++++++++++++++++++++++++++++++------------
 net/mac80211/ieee80211_i.h |   2 +
 net/mac80211/iface.c       |   7 +++
 net/mac80211/mlme.c        |  30 ++++++---
 net/wireless/ap.c          |   4 +-
 net/wireless/core.c        |  44 +++++++++++---
 net/wireless/core.h        |   7 +++
 net/wireless/mesh.c        |   4 +-
 net/wireless/trace.h       |  15 +++++
 net/wireless/util.c        |   5 ++
 12 files changed, 226 insertions(+), 59 deletions(-)

-- 
1.8.5.3

[PATCH v2 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

It was possible for tx queues to be stuck locked
if AP CSA finalization failed. In that case
stop_ap nor do_stop woke the queues up. This means
it was impossible to perform tx at all until
driver was reloaded or a successful CSA was
performed later.

It was possible to solve this in a simpler manner
however this is more robust and future proof
(having multi-vif CSA in mind).

New sdata->csa_block_tx is introduced to keep
track of which interfaces requested tx to be
blocked for CSA. This is required because mac80211
locks all tx queues for that purpose. This means
queues must be unlocked only when last tx-blocking
CSA interface is finished.

It is still possible to have tx queues stopped
after CSA failure but as soon as offending
interfaces are stopped from userspace (stop_ap or
ifdown) tx queues are woken up properly.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * get rid of idempotency

 include/net/mac80211.h     |  4 ++-
 net/mac80211/cfg.c         | 78 +++++++++++++++++++++++++++++++++++++---------
 net/mac80211/ieee80211_i.h |  2 ++
 net/mac80211/iface.c       |  7 +++++
 net/mac80211/mlme.c        | 30 ++++++++++++------
 5 files changed, 97 insertions(+), 24 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 8ce9ae1..91e90a2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1113,7 +1113,9 @@ enum ieee80211_vif_flags {
  * @addr: address of this interface
  * @p2p: indicates whether this AP or STA interface is a p2p
  *	interface, i.e. a GO or p2p-sta respectively
- * @csa_active: marks whether a channel switch is going on
+ * @csa_active: marks whether a channel switch is going on. Internally it is
+ *	write-protected by sdata_lock and local->mtx so holding either is fine
+ *	for read access.
  * @driver_flags: flags/capabilities the driver has for this interface,
  *	these need to be set (or cleared) when the interface is added
  *	or, if supported by the driver, the interface type is changed
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 137d379..7737884 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1084,6 +1084,31 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	lockdep_assert_held(&local->mtx);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+
+		if (!sdata->vif.csa_active)
+			continue;
+
+		if (!sdata->csa_block_tx)
+			continue;
+
+		rcu_read_unlock();
+		return true;
+	}
+	rcu_read_unlock();
+
+	return false;
+}
+
 static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -1101,7 +1126,14 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
 
 	/* abort any running channel switch */
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
+
 	kfree(sdata->u.ap.next_beacon);
 	sdata->u.ap.next_beacon = NULL;
 
@@ -3025,11 +3057,10 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	int err, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
-	mutex_lock(&local->mtx);
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
-	mutex_unlock(&local->mtx);
 	if (WARN_ON(err < 0))
 		return;
 
@@ -3070,10 +3101,6 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
-					IEEE80211_MAX_QUEUE_MAP,
-					IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 }
 
@@ -3082,8 +3109,11 @@ 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);
+	struct ieee80211_local *local = sdata->local;
 
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
+
 	/* AP might have been stopped while waiting for the lock. */
 	if (!sdata->vif.csa_active)
 		goto unlock;
@@ -3092,8 +3122,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 		goto unlock;
 
 	ieee80211_csa_finalize(sdata);
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
 
 unlock:
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
 }
 
@@ -3220,8 +3255,8 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-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;
@@ -3230,6 +3265,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	int err, num_chanctx, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
 	if (!list_empty(&local->roc_list) || local->scanning)
 		return -EBUSY;
@@ -3271,15 +3307,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 
 	sdata->csa_radar_required = params->radar_required;
-
-	if (params->block_tx)
-		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	sdata->csa_chandef = params->chandef;
+	sdata->csa_block_tx = params->block_tx;
 	sdata->vif.csa_active = true;
 
+	if (sdata->csa_block_tx)
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+
 	if (changed) {
 		ieee80211_bss_info_change_notify(sdata, changed);
 		drv_channel_switch_beacon(sdata, &params->chandef);
@@ -3291,6 +3327,20 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+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;
+	int err;
+
+	mutex_lock(&local->mtx);
+	err = __ieee80211_channel_switch(wiphy, dev, params);
+	mutex_unlock(&local->mtx);
+
+	return err;
+}
+
 static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 			     struct cfg80211_mgmt_tx_params *params,
 			     u64 *cookie)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3b73c3e..02fe9f4 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -754,6 +754,7 @@ struct ieee80211_sub_if_data {
 	int csa_counter_offset_beacon;
 	int csa_counter_offset_presp;
 	bool csa_radar_required;
+	bool csa_block_tx; /* write-protected by sdata_lock and local->mtx */
 	struct cfg80211_chan_def csa_chandef;
 
 	/* context reservation -- protected with chanctx_mtx */
@@ -1466,6 +1467,7 @@ void ieee80211_sw_roc_work(struct work_struct *work);
 void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
 
 /* channel switch handling */
+bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local);
 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);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index e753d72..7ab702a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -838,8 +838,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	cancel_work_sync(&sdata->recalc_smps);
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
+
 	cancel_work_sync(&sdata->csa_finalize_work);
 
 	cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index bbc2175..2ca1472 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -952,15 +952,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->csa_chandef.chan;
 
-	/* XXX: wait for a beacon first? */
-	ieee80211_wake_queues_by_reason(&local->hw,
-					IEEE80211_MAX_QUEUE_MAP,
-					IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	ieee80211_bss_info_change_notify(sdata, changed);
 
  out:
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	/* XXX: wait for a beacon first? */
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
+
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
 	sdata_unlock(sdata);
 }
@@ -1077,12 +1080,16 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	mutex_unlock(&local->chanctx_mtx);
 
 	sdata->csa_chandef = csa_ie.chandef;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = true;
+	sdata->csa_block_tx = csa_ie.mode;
 
-	if (csa_ie.mode)
+	if (sdata->csa_block_tx)
 		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
 	if (local->ops->channel_switch) {
 		/* use driver's channel switch callback */
@@ -2022,6 +2029,7 @@ EXPORT_SYMBOL(ieee80211_ap_probereq_get);
 
 static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 {
+	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
 
@@ -2035,10 +2043,14 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 			       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
 			       true, frame_buf);
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
 	cfg80211_tx_mlme_mgmt(sdata->dev, frame_buf,
 			      IEEE80211_DEAUTH_FRAME_LEN);
-- 
1.8.5.3

Re: [PATCH v2 1/4] mac80211: fix CSA tx queue locking

[Luca Coelho]

On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
> It was possible for tx queues to be stuck locked
> if AP CSA finalization failed.

I think it's clearer if you say "tx queue stopping" or something to
avoid confusion with the word "locking".  Same applies to the commit
subject.

[...]
> It is still possible to have tx queues stopped
> after CSA failure but as soon as offending
> interfaces are stopped from userspace (stop_ap or
> ifdown) tx queues are woken up properly.

Isn't there any way to avoid this? I mean, if you have identified some
cases, why not fix them too? :)

[...]
> @@ -3092,8 +3122,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>  		goto unlock;
>  
>  	ieee80211_csa_finalize(sdata);
> +	if (!ieee80211_csa_needs_block_tx(local))
> +		ieee80211_wake_queues_by_reason(&local->hw,
> +					IEEE80211_MAX_QUEUE_MAP,
> +					IEEE80211_QUEUE_STOP_REASON_CSA);

This should remain inside ieee80211_csa_finalize() as it was before,
because otherwise you won't wake up the queues in case of an immediate
switch.  Look at the bottom of your new  __ieee80211_channel_switch(),
we call ieee80211_csa_finalize() directly if the beacon didn't change.

Actually, in the beacon-didn't-change case, we should probably not even
stop the queues?

[...]
> @@ -3271,15 +3307,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>  		return err;
>  
>  	sdata->csa_radar_required = params->radar_required;
> -
> -	if (params->block_tx)
> -		ieee80211_stop_queues_by_reason(&local->hw,
> -				IEEE80211_MAX_QUEUE_MAP,
> -				IEEE80211_QUEUE_STOP_REASON_CSA);
> -
>  	sdata->csa_chandef = params->chandef;
> +	sdata->csa_block_tx = params->block_tx;
>  	sdata->vif.csa_active = true;
>  
> +	if (sdata->csa_block_tx)
> +		ieee80211_wake_queues_by_reason(&local->hw,
> +					IEEE80211_MAX_QUEUE_MAP,
> +					IEEE80211_QUEUE_STOP_REASON_CSA);

Shouldn't this be *stop* queues?

You can call ieee80211_stop_queues_by_reason() even if it is already
stopped for this reason, but maybe you could call
ieee80211_csa_needs_block_tx() here to avoid eventually calling it
multiple times.  I think this is also a bit more consistent.

	if(ieee80211_csa_needs_block_tx(...))
		ieee80211_stop_queues_by_reason(...)


[...]
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index bbc2175..2ca1472 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -952,15 +952,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
>  	/* XXX: shouldn't really modify cfg80211-owned data! */
>  	ifmgd->associated->channel = sdata->csa_chandef.chan;
>  
> -	/* XXX: wait for a beacon first? */
> -	ieee80211_wake_queues_by_reason(&local->hw,
> -					IEEE80211_MAX_QUEUE_MAP,
> -					IEEE80211_QUEUE_STOP_REASON_CSA);
> -
>  	ieee80211_bss_info_change_notify(sdata, changed);
>  
>   out:
> +	mutex_lock(&local->mtx);
>  	sdata->vif.csa_active = false;
> +	/* XXX: wait for a beacon first? */
> +	if (!ieee80211_csa_needs_block_tx(local))
> +		ieee80211_wake_queues_by_reason(&local->hw,
> +					IEEE80211_MAX_QUEUE_MAP,
> +					IEEE80211_QUEUE_STOP_REASON_CSA);
> +	mutex_unlock(&local->mtx);

Instead of moving this to the out label, I guess the right place for it
would be in ieee80211_set_disassoc().  Then we would catch the
csa_connection_drop_work cases plus a few other cases that seem to be
missing(?).

[...]
> @@ -1077,12 +1080,16 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
>  	mutex_unlock(&local->chanctx_mtx);
>  
>  	sdata->csa_chandef = csa_ie.chandef;
> +
> +	mutex_lock(&local->mtx);
>  	sdata->vif.csa_active = true;
> +	sdata->csa_block_tx = csa_ie.mode;
>  
> -	if (csa_ie.mode)
> +	if (sdata->csa_block_tx)
>  		ieee80211_stop_queues_by_reason(&local->hw,
> -				IEEE80211_MAX_QUEUE_MAP,
> -				IEEE80211_QUEUE_STOP_REASON_CSA);
> +					IEEE80211_MAX_QUEUE_MAP,
> +					IEEE80211_QUEUE_STOP_REASON_CSA);
> +	mutex_unlock(&local->mtx);

Same thing as before, maybe it's more consistent to call
ieee80211_csa_needs_block_tx()?

[...]
> @@ -2035,10 +2043,14 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
>  			       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
>  			       true, frame_buf);
>  	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
> +
> +	mutex_lock(&local->mtx);
>  	sdata->vif.csa_active = false;
> -	ieee80211_wake_queues_by_reason(&sdata->local->hw,
> +	if (!ieee80211_csa_needs_block_tx(local))
> +		ieee80211_wake_queues_by_reason(&local->hw,
>  					IEEE80211_MAX_QUEUE_MAP,
>  					IEEE80211_QUEUE_STOP_REASON_CSA);
> +	mutex_unlock(&local->mtx);

As I said before, it's probably better to do this in
ieee80211_set_disassoc().  But maybe in a separate patch?

--
Luca.

Re: [PATCH v2 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

On 24 March 2014 13:25, Luca Coelho <luca@coelho.fi> wrote:
> On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
>> It was possible for tx queues to be stuck locked
>> if AP CSA finalization failed.
>
> I think it's clearer if you say "tx queue stopping" or something to
> avoid confusion with the word "locking".  Same applies to the commit
> subject.
>
> [...]
>> It is still possible to have tx queues stopped
>> after CSA failure but as soon as offending
>> interfaces are stopped from userspace (stop_ap or
>> ifdown) tx queues are woken up properly.
>
> Isn't there any way to avoid this? I mean, if you have identified some
> cases, why not fix them too? :)
>
> [...]
>> @@ -3092,8 +3122,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>>               goto unlock;
>>
>>       ieee80211_csa_finalize(sdata);
>> +     if (!ieee80211_csa_needs_block_tx(local))
>> +             ieee80211_wake_queues_by_reason(&local->hw,
>> +                                     IEEE80211_MAX_QUEUE_MAP,
>> +                                     IEEE80211_QUEUE_STOP_REASON_CSA);
>
> This should remain inside ieee80211_csa_finalize() as it was before,
> because otherwise you won't wake up the queues in case of an immediate
> switch.  Look at the bottom of your new  __ieee80211_channel_switch(),
> we call ieee80211_csa_finalize() directly if the beacon didn't change.

Good point. I''l fix it.


> Actually, in the beacon-didn't-change case, we should probably not even
> stop the queues?

I think we should stop the queues. Take this for example:

2 ap vifs. ap1 starts csa cs_count=100 block_tx=0, ap2 starts csa
cs_count=0 block_tx=1.

In this case ap2 won't switch immediately but will stay around until
ap1 interface is done.


>
> [...]
>> @@ -3271,15 +3307,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
>>               return err;
>>
>>       sdata->csa_radar_required = params->radar_required;
>> -
>> -     if (params->block_tx)
>> -             ieee80211_stop_queues_by_reason(&local->hw,
>> -                             IEEE80211_MAX_QUEUE_MAP,
>> -                             IEEE80211_QUEUE_STOP_REASON_CSA);
>> -
>>       sdata->csa_chandef = params->chandef;
>> +     sdata->csa_block_tx = params->block_tx;
>>       sdata->vif.csa_active = true;
>>
>> +     if (sdata->csa_block_tx)
>> +             ieee80211_wake_queues_by_reason(&local->hw,
>> +                                     IEEE80211_MAX_QUEUE_MAP,
>> +                                     IEEE80211_QUEUE_STOP_REASON_CSA);
>
> Shouldn't this be *stop* queues?

Oops.. :-)


> You can call ieee80211_stop_queues_by_reason() even if it is already
> stopped for this reason, but maybe you could call
> ieee80211_csa_needs_block_tx() here to avoid eventually calling it
> multiple times.  I think this is also a bit more consistent.
>
>         if(ieee80211_csa_needs_block_tx(...))
>                 ieee80211_stop_queues_by_reason(...)

I fail to see how this would avoid calling it multiple times, or
rather, where is it being called multiple number of times now that it
can be prevented?

If you call ieee80211_csa_needs_block_tx() you check the block_tx
anyway. Since csa_active is guaranteed to be true here already you
just need to check block_tx.


> [...]
>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> index bbc2175..2ca1472 100644
>> --- a/net/mac80211/mlme.c
>> +++ b/net/mac80211/mlme.c
>> @@ -952,15 +952,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
>>       /* XXX: shouldn't really modify cfg80211-owned data! */
>>       ifmgd->associated->channel = sdata->csa_chandef.chan;
>>
>> -     /* XXX: wait for a beacon first? */
>> -     ieee80211_wake_queues_by_reason(&local->hw,
>> -                                     IEEE80211_MAX_QUEUE_MAP,
>> -                                     IEEE80211_QUEUE_STOP_REASON_CSA);
>> -
>>       ieee80211_bss_info_change_notify(sdata, changed);
>>
>>   out:
>> +     mutex_lock(&local->mtx);
>>       sdata->vif.csa_active = false;
>> +     /* XXX: wait for a beacon first? */
>> +     if (!ieee80211_csa_needs_block_tx(local))
>> +             ieee80211_wake_queues_by_reason(&local->hw,
>> +                                     IEEE80211_MAX_QUEUE_MAP,
>> +                                     IEEE80211_QUEUE_STOP_REASON_CSA);
>> +     mutex_unlock(&local->mtx);
>
> Instead of moving this to the out label, I guess the right place for it
> would be in ieee80211_set_disassoc().  Then we would catch the
> csa_connection_drop_work cases plus a few other cases that seem to be
> missing(?).

Good point. Will do.


Michał

Re: [PATCH v2 1/4] mac80211: fix CSA tx queue locking

[Luca Coelho]

On Mon, 2014-03-24 at 13:58 +0100, Michal Kazior wrote:
> On 24 March 2014 13:25, Luca Coelho <luca@coelho.fi> wrote:
> > On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
> > Actually, in the beacon-didn't-change case, we should probably not even
> > stop the queues?
> 
> I think we should stop the queues. Take this for example:
> 
> 2 ap vifs. ap1 starts csa cs_count=100 block_tx=0, ap2 starts csa
> cs_count=0 block_tx=1.
> 
> In this case ap2 won't switch immediately but will stay around until
> ap1 interface is done.

Yes, this would be okay for count=0.  But for count=1 making ap2 wait
until ap1 is done could be problematic.  The STAs will be on the other
channel waiting after 1 beacon but it will take up to 99 more beacons
for ap2 to switch.  They'll probably drop the connection (or try to
roam) due to beacon loss.

I think it would be better to force ap2 to use the same count as ap1 (or
whatever is left in ap1's count).


> > You can call ieee80211_stop_queues_by_reason() even if it is already
> > stopped for this reason, but maybe you could call
> > ieee80211_csa_needs_block_tx() here to avoid eventually calling it
> > multiple times.  I think this is also a bit more consistent.
> >
> >         if(ieee80211_csa_needs_block_tx(...))
> >                 ieee80211_stop_queues_by_reason(...)
> 
> I fail to see how this would avoid calling it multiple times, or
> rather, where is it being called multiple number of times now that it
> can be prevented?
> 
> If you call ieee80211_csa_needs_block_tx() you check the block_tx
> anyway. Since csa_active is guaranteed to be true here already you
> just need to check block_tx.

Yeah, forget it.

--
Luca.

[PATCH v2 2/4] mac80211: split CSA finalize function

[Michal Kazior]

Improves readability and modularity.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 61 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 7737884..65768ef 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3051,25 +3051,11 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
 }
 EXPORT_SYMBOL(ieee80211_csa_finish);
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
+					  u32 *changed)
 {
-	struct ieee80211_local *local = sdata->local;
-	int err, changed = 0;
-
-	sdata_assert_lock(sdata);
-	lockdep_assert_held(&local->mtx);
-
-	sdata->radar_required = sdata->csa_radar_required;
-	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (WARN_ON(err < 0))
-		return;
-
-	if (!local->use_chanctx) {
-		local->_oper_chandef = sdata->csa_chandef;
-		ieee80211_hw_config(local, 0);
-	}
+	int err;
 
-	sdata->vif.csa_active = false;
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
 		err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
@@ -3077,30 +3063,57 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		sdata->u.ap.next_beacon = NULL;
 
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		err = ieee80211_ibss_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
 		err = ieee80211_mesh_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #endif
 	default:
 		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 changed = 0;
+	int err;
+
+	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
+
+	sdata->radar_required = sdata->csa_radar_required;
+	err = ieee80211_vif_change_channel(sdata, &changed);
+	if (WARN_ON(err < 0))
 		return;
+
+	if (!local->use_chanctx) {
+		local->_oper_chandef = sdata->csa_chandef;
+		ieee80211_hw_config(local, 0);
 	}
 
-	ieee80211_bss_info_change_notify(sdata, changed);
+	sdata->vif.csa_active = false;
+
+	err = ieee80211_set_after_csa_beacon(sdata, &changed);
+	if (err)
+		return;
 
+	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 }
 
-- 
1.8.5.3

[PATCH v2 3/4] cfg80211: export interface stopping function

[Michal Kazior]

This exports a new cfg80211_stop_iface() function.

This is intended for driver internal interface
combination management and channel switching.

Due to locking issues (it re-enters driver) the
call is asynchronous and uses cfg80211 event
list/worker.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 include/net/cfg80211.h | 15 +++++++++++++++
 net/wireless/ap.c      |  4 ++--
 net/wireless/core.c    | 44 +++++++++++++++++++++++++++++++++++++-------
 net/wireless/core.h    |  7 +++++++
 net/wireless/mesh.c    |  4 ++--
 net/wireless/trace.h   | 15 +++++++++++++++
 net/wireless/util.c    |  5 +++++
 7 files changed, 83 insertions(+), 11 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2f28ac4..7f0cbfb 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4705,6 +4705,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
 				const u8 radar_detect,
 				const int iftype_num[NUM_NL80211_IFTYPES]);
 
+/*
+ * cfg80211_stop_iface - stop given interface
+ *
+ * @wiphy: the wiphy
+ * @wdev: wireless device
+ *
+ * Stop interface as if AP was stopped, IBSS/mesh left, STA disconnected.
+ *
+ * This is intended for channel switching and internal driver interface
+ * combination management.
+ *
+ * Note: This doesn't need any locks and is asynchronous.
+ */
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/wireless/ap.c b/net/wireless/ap.c
index 3e02ade..bdad1f9 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -6,8 +6,8 @@
 #include "rdev-ops.h"
 
 
-static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
-			      struct net_device *dev, bool notify)
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 7fca03e..b2f064f 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -748,23 +748,23 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 		rdev->num_running_monitor_ifaces += num;
 }
 
-void cfg80211_leave(struct cfg80211_registered_device *rdev,
-		    struct wireless_dev *wdev)
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev)
 {
 	struct net_device *dev = wdev->netdev;
 
 	ASSERT_RTNL();
+	ASSERT_WDEV_LOCK(wdev);
 
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
-		cfg80211_leave_ibss(rdev, dev, true);
+		__cfg80211_leave_ibss(rdev, dev, true);
 		break;
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_STATION:
 		if (rdev->sched_scan_req && dev == rdev->sched_scan_req->dev)
 			__cfg80211_stop_sched_scan(rdev, false);
 
-		wdev_lock(wdev);
 #ifdef CONFIG_CFG80211_WEXT
 		kfree(wdev->wext.ie);
 		wdev->wext.ie = NULL;
@@ -773,14 +773,13 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 #endif
 		cfg80211_disconnect(rdev, dev,
 				    WLAN_REASON_DEAUTH_LEAVING, true);
-		wdev_unlock(wdev);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:
-		cfg80211_leave_mesh(rdev, dev);
+		__cfg80211_leave_mesh(rdev, dev);
 		break;
 	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_P2P_GO:
-		cfg80211_stop_ap(rdev, dev, true);
+		__cfg80211_stop_ap(rdev, dev, true);
 		break;
 	default:
 		break;
@@ -789,6 +788,37 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 	wdev->beacon_interval = 0;
 }
 
+void cfg80211_leave(struct cfg80211_registered_device *rdev,
+		    struct wireless_dev *wdev)
+{
+	ASSERT_RTNL();
+
+	wdev_lock(wdev);
+	__cfg80211_leave(rdev, wdev);
+	wdev_unlock(wdev);
+}
+
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
+{
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+	struct cfg80211_event *ev;
+	unsigned long flags;
+
+	trace_cfg80211_stop_iface(wiphy, wdev);
+
+	ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+	if (!ev)
+		return;
+
+	ev->type = EVENT_STOPPED;
+
+	spin_lock_irqsave(&wdev->event_lock, flags);
+	list_add_tail(&ev->list, &wdev->event_list);
+	spin_unlock_irqrestore(&wdev->event_lock, flags);
+	queue_work(cfg80211_wq, &rdev->event_work);
+}
+EXPORT_SYMBOL(cfg80211_stop_iface);
+
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 					 unsigned long state, void *ptr)
 {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 85340a9..bffb36c 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -181,6 +181,7 @@ enum cfg80211_event_type {
 	EVENT_ROAMED,
 	EVENT_DISCONNECTED,
 	EVENT_IBSS_JOINED,
+	EVENT_STOPPED,
 };
 
 struct cfg80211_event {
@@ -270,6 +271,8 @@ int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 		       struct net_device *dev,
 		       struct mesh_setup *setup,
 		       const struct mesh_config *conf);
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev);
 int cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
 			struct net_device *dev);
 int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
@@ -277,6 +280,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 			      struct cfg80211_chan_def *chandef);
 
 /* AP */
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify);
 int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
 		     struct net_device *dev, bool notify);
 
@@ -430,6 +435,8 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 			       enum nl80211_iftype iftype, int num);
 
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev);
 void cfg80211_leave(struct cfg80211_registered_device *rdev,
 		    struct wireless_dev *wdev);
 
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index b8a7a35..e02301b 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -237,8 +237,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 	return 0;
 }
 
-static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
-				 struct net_device *dev)
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index aabccf1..24acf96 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2615,6 +2615,21 @@ TRACE_EVENT(cfg80211_ft_event,
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(target_ap))
 );
 
+TRACE_EVENT(cfg80211_stop_iface,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
+	TP_ARGS(wiphy, wdev),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		WDEV_ENTRY
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		WDEV_ASSIGN;
+	),
+	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
+		  WIPHY_PR_ARG, WDEV_PR_ARG)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 2984aff..241f0ad 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -789,6 +789,8 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
 	unsigned long flags;
 	const u8 *bssid = NULL;
 
+	ASSERT_RTNL();
+
 	spin_lock_irqsave(&wdev->event_lock, flags);
 	while (!list_empty(&wdev->event_list)) {
 		ev = list_first_entry(&wdev->event_list,
@@ -823,6 +825,9 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
 			__cfg80211_ibss_joined(wdev->netdev, ev->ij.bssid,
 					       ev->ij.channel);
 			break;
+		case EVENT_STOPPED:
+			__cfg80211_leave(wiphy_to_dev(wdev->wiphy), wdev);
+			break;
 		}
 		wdev_unlock(wdev);
 
-- 
1.8.5.3

Re: [PATCH v2 3/4] cfg80211: export interface stopping function

[Luca Coelho]

On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
> This exports a new cfg80211_stop_iface() function.
> 
> This is intended for driver internal interface
> combination management and channel switching.
> 
> Due to locking issues (it re-enters driver) the
> call is asynchronous and uses cfg80211 event
> list/worker.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

Looks good to me, with a couple of nitpicks.

[...]
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 2f28ac4..7f0cbfb 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4705,6 +4705,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
>  				const u8 radar_detect,
>  				const int iftype_num[NUM_NL80211_IFTYPES]);
>  
> +/*
> + * cfg80211_stop_iface - stop given interface
> + *
> + * @wiphy: the wiphy
> + * @wdev: wireless device
> + *
> + * Stop interface as if AP was stopped, IBSS/mesh left, STA disconnected.

This is not very clear.  Maybe you should say that this allows the
driver to _trigger_ an interface to be stopped, so that it's clear that
all the stopping flow will take place.


> + *
> + * This is intended for channel switching and internal driver interface
> + * combination management.

Not sure you need to say when this is needed.  The driver could call it
for any internal reason.

[...]
> @@ -789,6 +788,37 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
>  	wdev->beacon_interval = 0;
>  }
>  
> +void cfg80211_leave(struct cfg80211_registered_device *rdev,
> +		    struct wireless_dev *wdev)
> +{
> +	ASSERT_RTNL();
> +
> +	wdev_lock(wdev);
> +	__cfg80211_leave(rdev, wdev);
> +	wdev_unlock(wdev);
> +}
> +
> +void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
> +{
> +	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> +	struct cfg80211_event *ev;
> +	unsigned long flags;
> +
> +	trace_cfg80211_stop_iface(wiphy, wdev);
> +
> +	ev = kzalloc(sizeof(*ev), GFP_ATOMIC);

Is there any reason why you don't pass the GFP type to this function, as
in all other functions that allow the driver to queue events?

--
Luca.

Re: [PATCH v2 3/4] cfg80211: export interface stopping function

[Michal Kazior]

On 24 March 2014 14:56, Luca Coelho <luca@coelho.fi> wrote:
> On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
>> This exports a new cfg80211_stop_iface() function.
>>
>> This is intended for driver internal interface
>> combination management and channel switching.
>>
>> Due to locking issues (it re-enters driver) the
>> call is asynchronous and uses cfg80211 event
>> list/worker.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>> ---
>
> Looks good to me, with a couple of nitpicks.
>
> [...]
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 2f28ac4..7f0cbfb 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -4705,6 +4705,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
>>                               const u8 radar_detect,
>>                               const int iftype_num[NUM_NL80211_IFTYPES]);
>>
>> +/*
>> + * cfg80211_stop_iface - stop given interface
>> + *
>> + * @wiphy: the wiphy
>> + * @wdev: wireless device
>> + *
>> + * Stop interface as if AP was stopped, IBSS/mesh left, STA disconnected.
>
> This is not very clear.  Maybe you should say that this allows the
> driver to _trigger_ an interface to be stopped, so that it's clear that
> all the stopping flow will take place.

Yeah, it's async so "trigger" makes more sense.


>> + *
>> + * This is intended for channel switching and internal driver interface
>> + * combination management.
>
> Not sure you need to say when this is needed.  The driver could call it
> for any internal reason.

I'll remove this sentence then unless someone has an objection? :-)


> [...]
>> @@ -789,6 +788,37 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
>>       wdev->beacon_interval = 0;
>>  }
>>
>> +void cfg80211_leave(struct cfg80211_registered_device *rdev,
>> +                 struct wireless_dev *wdev)
>> +{
>> +     ASSERT_RTNL();
>> +
>> +     wdev_lock(wdev);
>> +     __cfg80211_leave(rdev, wdev);
>> +     wdev_unlock(wdev);
>> +}
>> +
>> +void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
>> +{
>> +     struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
>> +     struct cfg80211_event *ev;
>> +     unsigned long flags;
>> +
>> +     trace_cfg80211_stop_iface(wiphy, wdev);
>> +
>> +     ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
>
> Is there any reason why you don't pass the GFP type to this function, as
> in all other functions that allow the driver to queue events?

Not really. I'll add the argument.


Michał

Re: [PATCH v2 3/4] cfg80211: export interface stopping function

[Arend van Spriel]

On 21/03/14 14:31, Michal Kazior wrote:
> This exports a new cfg80211_stop_iface() function.
>
> This is intended for driver internal interface
> combination management and channel switching.
>
> Due to locking issues (it re-enters driver) the
> call is asynchronous and uses cfg80211 event
> list/worker.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---
>   include/net/cfg80211.h | 15 +++++++++++++++
>   net/wireless/ap.c      |  4 ++--
>   net/wireless/core.c    | 44 +++++++++++++++++++++++++++++++++++++-------
>   net/wireless/core.h    |  7 +++++++
>   net/wireless/mesh.c    |  4 ++--
>   net/wireless/trace.h   | 15 +++++++++++++++
>   net/wireless/util.c    |  5 +++++
>   7 files changed, 83 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 2f28ac4..7f0cbfb 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4705,6 +4705,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
>   				const u8 radar_detect,
>   				const int iftype_num[NUM_NL80211_IFTYPES]);
>
> +/*
> + * cfg80211_stop_iface - stop given interface
> + *
> + * @wiphy: the wiphy
> + * @wdev: wireless device
> + *
> + * Stop interface as if AP was stopped, IBSS/mesh left, STA disconnected.
> + *
> + * This is intended for channel switching and internal driver interface
> + * combination management.
> + *
> + * Note: This doesn't need any locks and is asynchronous.
> + */
> +void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev);
> +
>   /* Logging, debugging and troubleshooting/diagnostic helpers. */
>
>   /* wiphy_printk helpers, similar to dev_printk */
> diff --git a/net/wireless/ap.c b/net/wireless/ap.c
> index 3e02ade..bdad1f9 100644
> --- a/net/wireless/ap.c
> +++ b/net/wireless/ap.c
> @@ -6,8 +6,8 @@
>   #include "rdev-ops.h"
>
>
> -static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
> -			      struct net_device *dev, bool notify)
> +int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
> +		       struct net_device *dev, bool notify)
>   {
>   	struct wireless_dev *wdev = dev->ieee80211_ptr;
>   	int err;
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index 7fca03e..b2f064f 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -748,23 +748,23 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
>   		rdev->num_running_monitor_ifaces += num;
>   }
>
> -void cfg80211_leave(struct cfg80211_registered_device *rdev,
> -		    struct wireless_dev *wdev)
> +void __cfg80211_leave(struct cfg80211_registered_device *rdev,
> +		      struct wireless_dev *wdev)
>   {
>   	struct net_device *dev = wdev->netdev;
>
>   	ASSERT_RTNL();

If you have the assert check here do you need to do it from the call 
sites as well?

> +	ASSERT_WDEV_LOCK(wdev);

So you are changing behaviour here by requiring the wdev to be locked.

>   	switch (wdev->iftype) {
>   	case NL80211_IFTYPE_ADHOC:
> -		cfg80211_leave_ibss(rdev, dev, true);
> +		__cfg80211_leave_ibss(rdev, dev, true);
>   		break;
>   	case NL80211_IFTYPE_P2P_CLIENT:
>   	case NL80211_IFTYPE_STATION:
>   		if (rdev->sched_scan_req && dev == rdev->sched_scan_req->dev)
>   			__cfg80211_stop_sched_scan(rdev, false);
>
> -		wdev_lock(wdev);
>   #ifdef CONFIG_CFG80211_WEXT
>   		kfree(wdev->wext.ie);
>   		wdev->wext.ie = NULL;
> @@ -773,14 +773,13 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
>   #endif
>   		cfg80211_disconnect(rdev, dev,
>   				    WLAN_REASON_DEAUTH_LEAVING, true);
> -		wdev_unlock(wdev);
>   		break;
>   	case NL80211_IFTYPE_MESH_POINT:
> -		cfg80211_leave_mesh(rdev, dev);
> +		__cfg80211_leave_mesh(rdev, dev);
>   		break;
>   	case NL80211_IFTYPE_AP:
>   	case NL80211_IFTYPE_P2P_GO:
> -		cfg80211_stop_ap(rdev, dev, true);
> +		__cfg80211_stop_ap(rdev, dev, true);
>   		break;
>   	default:
>   		break;
> @@ -789,6 +788,37 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
>   	wdev->beacon_interval = 0;
>   }
>
> +void cfg80211_leave(struct cfg80211_registered_device *rdev,
> +		    struct wireless_dev *wdev)
> +{
> +	ASSERT_RTNL();

This assert if also in __cfg80211_leave()

> +	wdev_lock(wdev);
> +	__cfg80211_leave(rdev, wdev);
> +	wdev_unlock(wdev);
> +}
> +
> +void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
> +{
> +	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
> +	struct cfg80211_event *ev;
> +	unsigned long flags;
> +
> +	trace_cfg80211_stop_iface(wiphy, wdev);
> +
> +	ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
> +	if (!ev)
> +		return;
> +
> +	ev->type = EVENT_STOPPED;
> +
> +	spin_lock_irqsave(&wdev->event_lock, flags);
> +	list_add_tail(&ev->list, &wdev->event_list);
> +	spin_unlock_irqrestore(&wdev->event_lock, flags);
> +	queue_work(cfg80211_wq, &rdev->event_work);
> +}
> +EXPORT_SYMBOL(cfg80211_stop_iface);
> +
>   static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
>   					 unsigned long state, void *ptr)
>   {
> diff --git a/net/wireless/core.h b/net/wireless/core.h
> index 85340a9..bffb36c 100644
> --- a/net/wireless/core.h
> +++ b/net/wireless/core.h
> @@ -181,6 +181,7 @@ enum cfg80211_event_type {
>   	EVENT_ROAMED,
>   	EVENT_DISCONNECTED,
>   	EVENT_IBSS_JOINED,
> +	EVENT_STOPPED,
>   };
>
>   struct cfg80211_event {
> @@ -270,6 +271,8 @@ int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
>   		       struct net_device *dev,
>   		       struct mesh_setup *setup,
>   		       const struct mesh_config *conf);
> +int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
> +			  struct net_device *dev);
>   int cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
>   			struct net_device *dev);
>   int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
> @@ -277,6 +280,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
>   			      struct cfg80211_chan_def *chandef);
>
>   /* AP */
> +int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
> +		       struct net_device *dev, bool notify);
>   int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
>   		     struct net_device *dev, bool notify);
>
> @@ -430,6 +435,8 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
>   void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
>   			       enum nl80211_iftype iftype, int num);
>
> +void __cfg80211_leave(struct cfg80211_registered_device *rdev,
> +		      struct wireless_dev *wdev);
>   void cfg80211_leave(struct cfg80211_registered_device *rdev,
>   		    struct wireless_dev *wdev);
>
> diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
> index b8a7a35..e02301b 100644
> --- a/net/wireless/mesh.c
> +++ b/net/wireless/mesh.c
> @@ -237,8 +237,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
>   	return 0;
>   }
>
> -static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
> -				 struct net_device *dev)
> +int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
> +			  struct net_device *dev)
>   {
>   	struct wireless_dev *wdev = dev->ieee80211_ptr;
>   	int err;
> diff --git a/net/wireless/trace.h b/net/wireless/trace.h
> index aabccf1..24acf96 100644
> --- a/net/wireless/trace.h
> +++ b/net/wireless/trace.h
> @@ -2615,6 +2615,21 @@ TRACE_EVENT(cfg80211_ft_event,
>   		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(target_ap))
>   );
>
> +TRACE_EVENT(cfg80211_stop_iface,
> +	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
> +	TP_ARGS(wiphy, wdev),
> +	TP_STRUCT__entry(
> +		WIPHY_ENTRY
> +		WDEV_ENTRY
> +	),
> +	TP_fast_assign(
> +		WIPHY_ASSIGN;
> +		WDEV_ASSIGN;
> +	),
> +	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
> +		  WIPHY_PR_ARG, WDEV_PR_ARG)
> +);
> +
>   #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
>
>   #undef TRACE_INCLUDE_PATH
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 2984aff..241f0ad 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -789,6 +789,8 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
>   	unsigned long flags;
>   	const u8 *bssid = NULL;
>
> +	ASSERT_RTNL();

Also checked in __cfg80211_leave()

>   	spin_lock_irqsave(&wdev->event_lock, flags);
>   	while (!list_empty(&wdev->event_list)) {
>   		ev = list_first_entry(&wdev->event_list,
> @@ -823,6 +825,9 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
>   			__cfg80211_ibss_joined(wdev->netdev, ev->ij.bssid,
>   					       ev->ij.channel);
>   			break;
> +		case EVENT_STOPPED:
> +			__cfg80211_leave(wiphy_to_dev(wdev->wiphy), wdev);
> +			break;
>   		}
>   		wdev_unlock(wdev);
>
>

Regards,
Arend

Re: [PATCH v2 3/4] cfg80211: export interface stopping function

[Michal Kazior]

On 24 March 2014 15:32, Arend van Spriel <arend@broadcom.com> wrote:
> On 21/03/14 14:31, Michal Kazior wrote:

[...]

>> --- a/net/wireless/core.c
>> +++ b/net/wireless/core.c
>> @@ -748,23 +748,23 @@ void cfg80211_update_iface_num(struct
>> cfg80211_registered_device *rdev,
>>                 rdev->num_running_monitor_ifaces += num;
>>   }
>>
>> -void cfg80211_leave(struct cfg80211_registered_device *rdev,
>> -                   struct wireless_dev *wdev)
>> +void __cfg80211_leave(struct cfg80211_registered_device *rdev,
>> +                     struct wireless_dev *wdev)
>>   {
>>         struct net_device *dev = wdev->netdev;
>>
>>         ASSERT_RTNL();
>
>
> If you have the assert check here do you need to do it from the call sites
> as well?

I just tend to propagate lockdeps asserts up. It's easier to get the
idea of locking at a quick glance and it causes lockdep to splat
sooner if you have lots of conditionals (i.e. before you hit a very
specific if-else-codepath). I'll remove this redundancy when I re-spin
unless someone objects?


>> +       ASSERT_WDEV_LOCK(wdev);
>
>
> So you are changing behaviour here by requiring the wdev to be locked.

It doesn't change much in practice. Notice that prior to the patch all
switch-cases performed wdev_lock() already (either on site, or callees
did that immediately). This simply moves the locking outside the
__cfg80211_leave() so it's possible to call it from the event worker.
__cfg80211_stop_sched_scan() doesn't care about wdev lock so it's okay
I guess?


Michał

[PATCH v2 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

It doesn't make much sense to leave a cripled
interface running.

As a side effect this will unblock tx queues with
CSA reason immediately after failure instead of
until after userspace requests interface to stop.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 65768ef..64e152d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3088,7 +3088,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
@@ -3100,7 +3100,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
 	if (WARN_ON(err < 0))
-		return;
+		return err;
 
 	if (!local->use_chanctx) {
 		local->_oper_chandef = sdata->csa_chandef;
@@ -3111,10 +3111,12 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	err = ieee80211_set_after_csa_beacon(sdata, &changed);
 	if (err)
-		return;
+		return err;
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
+
+	return 0;
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3123,6 +3125,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 		container_of(work, struct ieee80211_sub_if_data,
 			     csa_finalize_work);
 	struct ieee80211_local *local = sdata->local;
+	int err;
 
 	sdata_lock(sdata);
 	mutex_lock(&local->mtx);
@@ -3134,7 +3137,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	if (!ieee80211_sdata_running(sdata))
 		goto unlock;
 
-	ieee80211_csa_finalize(sdata);
+	err = ieee80211_csa_finalize(sdata);
+	if (err) {
+		sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
+		cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev);
+		goto unlock;
+	}
+
 	if (!ieee80211_csa_needs_block_tx(local))
 		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
-- 
1.8.5.3

Re: [PATCH v2 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Luca Coelho]

On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
> It doesn't make much sense to leave a cripled
> interface running.
> 
> As a side effect this will unblock tx queues with
> CSA reason immediately after failure instead of
> until after userspace requests interface to stop.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---
>  net/mac80211/cfg.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 65768ef..64e152d 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3088,7 +3088,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
>  	return 0;
>  }
>  
> -static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
> +static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>  {
>  	struct ieee80211_local *local = sdata->local;
>  	u32 changed = 0;
> @@ -3100,7 +3100,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>  	sdata->radar_required = sdata->csa_radar_required;
>  	err = ieee80211_vif_change_channel(sdata, &changed);
>  	if (WARN_ON(err < 0))
> -		return;
> +		return err;

Now you propagate the error up, do we still need the WARN_ON here?

[...]
> @@ -3123,6 +3125,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>  		container_of(work, struct ieee80211_sub_if_data,
>  			     csa_finalize_work);
>  	struct ieee80211_local *local = sdata->local;
> +	int err;
>  
>  	sdata_lock(sdata);
>  	mutex_lock(&local->mtx);
> @@ -3134,7 +3137,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>  	if (!ieee80211_sdata_running(sdata))
>  		goto unlock;
>  
> -	ieee80211_csa_finalize(sdata);
> +	err = ieee80211_csa_finalize(sdata);
> +	if (err) {
> +		sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
> +		cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev);
> +		goto unlock;
> +	}
> +

This is neat. :) Maybe it's possible to do the same thing in mlme.c and
get rid of the connection_drop_work?

--
Luca.

Re: [PATCH v2 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

On 24 March 2014 15:55, Luca Coelho <luca@coelho.fi> wrote:
> On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
>> It doesn't make much sense to leave a cripled
>> interface running.
>>
>> As a side effect this will unblock tx queues with
>> CSA reason immediately after failure instead of
>> until after userspace requests interface to stop.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>> ---
>>  net/mac80211/cfg.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
>> index 65768ef..64e152d 100644
>> --- a/net/mac80211/cfg.c
>> +++ b/net/mac80211/cfg.c
>> @@ -3088,7 +3088,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
>>       return 0;
>>  }
>>
>> -static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>> +static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>>  {
>>       struct ieee80211_local *local = sdata->local;
>>       u32 changed = 0;
>> @@ -3100,7 +3100,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>>       sdata->radar_required = sdata->csa_radar_required;
>>       err = ieee80211_vif_change_channel(sdata, &changed);
>>       if (WARN_ON(err < 0))
>> -             return;
>> +             return err;
>
> Now you propagate the error up, do we still need the WARN_ON here?

Good point.


> [...]
>> @@ -3123,6 +3125,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>>               container_of(work, struct ieee80211_sub_if_data,
>>                            csa_finalize_work);
>>       struct ieee80211_local *local = sdata->local;
>> +     int err;
>>
>>       sdata_lock(sdata);
>>       mutex_lock(&local->mtx);
>> @@ -3134,7 +3137,13 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
>>       if (!ieee80211_sdata_running(sdata))
>>               goto unlock;
>>
>> -     ieee80211_csa_finalize(sdata);
>> +     err = ieee80211_csa_finalize(sdata);
>> +     if (err) {
>> +             sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
>> +             cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev);
>> +             goto unlock;
>> +     }
>> +
>
> This is neat. :) Maybe it's possible to do the same thing in mlme.c and
> get rid of the connection_drop_work?

I don't think it's necessary now.


Michał

Re: [PATCH v2 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Johannes Berg]

On Fri, 2014-03-21 at 14:31 +0100, Michal Kazior wrote:
> Hi,
> 
> The patchset fixes CSA tx queue locking and
> introduces interface teardown upon CSA failure
> during finalization.

Looks like you have some comments still, I'll assume you'll repost.

johannes

[PATCH v3 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Michal Kazior]

Hi,

The patchset fixes CSA tx queue locking and
introduces interface teardown upon CSA failure
during finalization.

v2:
 * get rid of idempotency

v3:
 * fixes (see each commit)

Michal Kazior (4):
  mac80211: fix CSA tx queue locking
  mac80211: split CSA finalize function
  cfg80211: export interface stopping function
  mac80211: disconnect iface if CSA unexpectedly fails

 include/net/cfg80211.h     |  15 +++++
 include/net/mac80211.h     |   4 +-
 net/mac80211/cfg.c         | 144 +++++++++++++++++++++++++++++++++------------
 net/mac80211/ieee80211_i.h |   2 +
 net/mac80211/iface.c       |   7 +++
 net/mac80211/mlme.c        |  37 +++++++++---
 net/wireless/ap.c          |   4 +-
 net/wireless/core.c        |  43 +++++++++++---
 net/wireless/core.h        |   7 +++
 net/wireless/mesh.c        |   4 +-
 net/wireless/trace.h       |  15 +++++
 net/wireless/util.c        |   3 +
 12 files changed, 228 insertions(+), 57 deletions(-)

-- 
1.8.5.3

[PATCH v3 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

It was possible for tx queues to be stuck locked
if AP CSA finalization failed. In that case
stop_ap nor do_stop woke the queues up. This means
it was impossible to perform tx at all until
driver was reloaded or a successful CSA was
performed later.

It was possible to solve this in a simpler manner
however this is more robust and future proof
(having multi-vif CSA in mind).

New sdata->csa_block_tx is introduced to keep
track of which interfaces requested tx to be
blocked for CSA. This is required because mac80211
locks all tx queues for that purpose. This means
queues must be unlocked only when last tx-blocking
CSA interface is finished.

It is still possible to have tx queues stopped
after CSA failure but as soon as offending
interfaces are stopped from userspace (stop_ap or
ifdown) tx queues are woken up properly.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * get rid of idempotency [Johannes]

v3:
 * fix wake/stop typo [Luca]
 * move queue waking to csa_finalize() [Luca]
 * tune how queues are woken for STA CSA [Luca]

 include/net/mac80211.h     |  4 ++-
 net/mac80211/cfg.c         | 77 ++++++++++++++++++++++++++++++++++++++--------
 net/mac80211/ieee80211_i.h |  2 ++
 net/mac80211/iface.c       |  7 +++++
 net/mac80211/mlme.c        | 37 ++++++++++++++++------
 5 files changed, 104 insertions(+), 23 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 604e279..ea69961 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1113,7 +1113,9 @@ enum ieee80211_vif_flags {
  * @addr: address of this interface
  * @p2p: indicates whether this AP or STA interface is a p2p
  *	interface, i.e. a GO or p2p-sta respectively
- * @csa_active: marks whether a channel switch is going on
+ * @csa_active: marks whether a channel switch is going on. Internally it is
+ *	write-protected by sdata_lock and local->mtx so holding either is fine
+ *	for read access.
  * @driver_flags: flags/capabilities the driver has for this interface,
  *	these need to be set (or cleared) when the interface is added
  *	or, if supported by the driver, the interface type is changed
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aa39381..b7cf8e4 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1084,6 +1084,31 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	lockdep_assert_held(&local->mtx);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+
+		if (!sdata->vif.csa_active)
+			continue;
+
+		if (!sdata->csa_block_tx)
+			continue;
+
+		rcu_read_unlock();
+		return true;
+	}
+	rcu_read_unlock();
+
+	return false;
+}
+
 static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -1101,7 +1126,14 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
 
 	/* abort any running channel switch */
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
+
 	kfree(sdata->u.ap.next_beacon);
 	sdata->u.ap.next_beacon = NULL;
 
@@ -3025,11 +3057,10 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	int err, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
-	mutex_lock(&local->mtx);
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
-	mutex_unlock(&local->mtx);
 	if (WARN_ON(err < 0))
 		return;
 
@@ -3070,11 +3101,12 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
+
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
-
-	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3082,8 +3114,11 @@ 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);
+	struct ieee80211_local *local = sdata->local;
 
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
+
 	/* AP might have been stopped while waiting for the lock. */
 	if (!sdata->vif.csa_active)
 		goto unlock;
@@ -3094,6 +3129,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	ieee80211_csa_finalize(sdata);
 
 unlock:
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
 }
 
@@ -3220,8 +3256,8 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-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;
@@ -3230,6 +3266,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	int err, num_chanctx, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
 	if (!list_empty(&local->roc_list) || local->scanning)
 		return -EBUSY;
@@ -3271,15 +3308,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 
 	sdata->csa_radar_required = params->radar_required;
-
-	if (params->block_tx)
-		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	sdata->csa_chandef = params->chandef;
+	sdata->csa_block_tx = params->block_tx;
 	sdata->vif.csa_active = true;
 
+	if (sdata->csa_block_tx)
+		ieee80211_stop_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+
 	if (changed) {
 		ieee80211_bss_info_change_notify(sdata, changed);
 		drv_channel_switch_beacon(sdata, &params->chandef);
@@ -3291,6 +3328,20 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+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;
+	int err;
+
+	mutex_lock(&local->mtx);
+	err = __ieee80211_channel_switch(wiphy, dev, params);
+	mutex_unlock(&local->mtx);
+
+	return err;
+}
+
 static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 			     struct cfg80211_mgmt_tx_params *params,
 			     u64 *cookie)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3b73c3e..02fe9f4 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -754,6 +754,7 @@ struct ieee80211_sub_if_data {
 	int csa_counter_offset_beacon;
 	int csa_counter_offset_presp;
 	bool csa_radar_required;
+	bool csa_block_tx; /* write-protected by sdata_lock and local->mtx */
 	struct cfg80211_chan_def csa_chandef;
 
 	/* context reservation -- protected with chanctx_mtx */
@@ -1466,6 +1467,7 @@ void ieee80211_sw_roc_work(struct work_struct *work);
 void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
 
 /* channel switch handling */
+bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local);
 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);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 359e14b..52dccd6 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -838,8 +838,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	cancel_work_sync(&sdata->recalc_smps);
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
+
 	cancel_work_sync(&sdata->csa_finalize_work);
 
 	cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index bbc2175..5a916bb 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -952,16 +952,20 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->csa_chandef.chan;
 
+	ieee80211_bss_info_change_notify(sdata, changed);
+
+	mutex_lock(&local->mtx);
+	sdata->vif.csa_active = false;
 	/* XXX: wait for a beacon first? */
-	ieee80211_wake_queues_by_reason(&local->hw,
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
-	ieee80211_bss_info_change_notify(sdata, changed);
-
- out:
-	sdata->vif.csa_active = false;
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+
+out:
 	sdata_unlock(sdata);
 }
 
@@ -1077,12 +1081,16 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	mutex_unlock(&local->chanctx_mtx);
 
 	sdata->csa_chandef = csa_ie.chandef;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = true;
+	sdata->csa_block_tx = csa_ie.mode;
 
-	if (csa_ie.mode)
+	if (sdata->csa_block_tx)
 		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
 	if (local->ops->channel_switch) {
 		/* use driver's channel switch callback */
@@ -1794,6 +1802,12 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	ifmgd->flags = 0;
 	mutex_lock(&local->mtx);
 	ieee80211_vif_release_channel(sdata);
+
+	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
 	mutex_unlock(&local->mtx);
 
 	sdata->encrypt_headroom = IEEE80211_ENCRYPT_HEADROOM;
@@ -2022,6 +2036,7 @@ EXPORT_SYMBOL(ieee80211_ap_probereq_get);
 
 static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 {
+	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
 
@@ -2035,10 +2050,14 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 			       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
 			       true, frame_buf);
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
 	cfg80211_tx_mlme_mgmt(sdata->dev, frame_buf,
 			      IEEE80211_DEAUTH_FRAME_LEN);
-- 
1.8.5.3

Re: [PATCH v3 1/4] mac80211: fix CSA tx queue locking

[Luca Coelho]

On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
> It was possible for tx queues to be stuck locked

I would still prefer to use "stopped" instead of locked here (and in the
subject).  But that's just me...


> if AP CSA finalization failed. In that case
> stop_ap nor do_stop woke the queues up.

Maybe "In that case *neither* stop_ap nor do_stop"...?

Otherwise looks good to me.

--
Luca.

Re: [PATCH v3 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

On 31 March 2014 13:06, Luca Coelho <luca@coelho.fi> wrote:
> On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
>> It was possible for tx queues to be stuck locked
>
> I would still prefer to use "stopped" instead of locked here (and in the
> subject).  But that's just me...

Ah, I totally missed this one.


>> if AP CSA finalization failed. In that case
>> stop_ap nor do_stop woke the queues up.
>
> Maybe "In that case *neither* stop_ap nor do_stop"...?

I'll respin later unless Johannes decides it's fine as it is.


Michał

Re: [PATCH v3 1/4] mac80211: fix CSA tx queue locking

[Johannes Berg]

On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
> It was possible for tx queues to be stuck locked
> if AP CSA finalization failed. In that case
> stop_ap nor do_stop woke the queues up. This means
> it was impossible to perform tx at all until
> driver was reloaded or a successful CSA was
> performed later.
> 
> It was possible to solve this in a simpler manner
> however this is more robust and future proof
> (having multi-vif CSA in mind).
> 
> New sdata->csa_block_tx is introduced to keep
> track of which interfaces requested tx to be
> blocked for CSA. This is required because mac80211
> locks all tx queues for that purpose. This means
> queues must be unlocked only when last tx-blocking
> CSA interface is finished.
> 
> It is still possible to have tx queues stopped
> after CSA failure but as soon as offending
> interfaces are stopped from userspace (stop_ap or
> ifdown) tx queues are woken up properly.

Looks fine, but I agree with Luca regarding locked -> stopped.

johannes

[PATCH v3 2/4] mac80211: split CSA finalize function

[Michal Kazior]

Improves readability and modularity.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 61 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b7cf8e4..2fd8e08 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3051,25 +3051,11 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
 }
 EXPORT_SYMBOL(ieee80211_csa_finish);
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
+					  u32 *changed)
 {
-	struct ieee80211_local *local = sdata->local;
-	int err, changed = 0;
-
-	sdata_assert_lock(sdata);
-	lockdep_assert_held(&local->mtx);
-
-	sdata->radar_required = sdata->csa_radar_required;
-	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (WARN_ON(err < 0))
-		return;
-
-	if (!local->use_chanctx) {
-		local->_oper_chandef = sdata->csa_chandef;
-		ieee80211_hw_config(local, 0);
-	}
+	int err;
 
-	sdata->vif.csa_active = false;
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
 		err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
@@ -3077,30 +3063,57 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		sdata->u.ap.next_beacon = NULL;
 
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		err = ieee80211_ibss_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
 		err = ieee80211_mesh_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #endif
 	default:
 		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 changed = 0;
+	int err;
+
+	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
+
+	sdata->radar_required = sdata->csa_radar_required;
+	err = ieee80211_vif_change_channel(sdata, &changed);
+	if (WARN_ON(err < 0))
 		return;
+
+	if (!local->use_chanctx) {
+		local->_oper_chandef = sdata->csa_chandef;
+		ieee80211_hw_config(local, 0);
 	}
 
-	ieee80211_bss_info_change_notify(sdata, changed);
+	sdata->vif.csa_active = false;
+
+	err = ieee80211_set_after_csa_beacon(sdata, &changed);
+	if (err)
+		return;
 
+	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 
 	if (!ieee80211_csa_needs_block_tx(local))
-- 
1.8.5.3

Re: [PATCH v3 2/4] mac80211: split CSA finalize function

[Luca Coelho]

On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
> Improves readability and modularity.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

Looks good to me.

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

[PATCH v3 3/4] cfg80211: export interface stopping function

[Michal Kazior]

This exports a new cfg80211_stop_iface() function.

This is intended for driver internal interface
combination management and channel switching.

Due to locking issues (it re-enters driver) the
call is asynchronous and uses cfg80211 event
list/worker.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v3:
 * improve function comment [Luca]
 * add gfp_t argument [Luca]
 * remove redundant lockdep checks [Arend]

 include/net/cfg80211.h | 15 +++++++++++++++
 net/wireless/ap.c      |  4 ++--
 net/wireless/core.c    | 43 ++++++++++++++++++++++++++++++++++++-------
 net/wireless/core.h    |  7 +++++++
 net/wireless/mesh.c    |  4 ++--
 net/wireless/trace.h   | 15 +++++++++++++++
 net/wireless/util.c    |  3 +++
 7 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fb8afce..b4ea653 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4715,6 +4715,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
 				const u8 radar_detect,
 				const int iftype_num[NUM_NL80211_IFTYPES]);
 
+/*
+ * cfg80211_stop_iface - trigger interface disconnection
+ *
+ * @wiphy: the wiphy
+ * @wdev: wireless device
+ * @gfp: context flags
+ *
+ * Trigger interface to be stopped as if AP was stopped, IBSS/mesh left, STA
+ * disconnected.
+ *
+ * Note: This doesn't need any locks and is asynchronous.
+ */
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev,
+			 gfp_t gfp);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/wireless/ap.c b/net/wireless/ap.c
index 3e02ade..bdad1f9 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -6,8 +6,8 @@
 #include "rdev-ops.h"
 
 
-static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
-			      struct net_device *dev, bool notify)
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 5fc0df0..585b131 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -792,23 +792,23 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 		rdev->num_running_monitor_ifaces += num;
 }
 
-void cfg80211_leave(struct cfg80211_registered_device *rdev,
-		    struct wireless_dev *wdev)
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev)
 {
 	struct net_device *dev = wdev->netdev;
 
 	ASSERT_RTNL();
+	ASSERT_WDEV_LOCK(wdev);
 
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
-		cfg80211_leave_ibss(rdev, dev, true);
+		__cfg80211_leave_ibss(rdev, dev, true);
 		break;
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_STATION:
 		if (rdev->sched_scan_req && dev == rdev->sched_scan_req->dev)
 			__cfg80211_stop_sched_scan(rdev, false);
 
-		wdev_lock(wdev);
 #ifdef CONFIG_CFG80211_WEXT
 		kfree(wdev->wext.ie);
 		wdev->wext.ie = NULL;
@@ -817,14 +817,13 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 #endif
 		cfg80211_disconnect(rdev, dev,
 				    WLAN_REASON_DEAUTH_LEAVING, true);
-		wdev_unlock(wdev);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:
-		cfg80211_leave_mesh(rdev, dev);
+		__cfg80211_leave_mesh(rdev, dev);
 		break;
 	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_P2P_GO:
-		cfg80211_stop_ap(rdev, dev, true);
+		__cfg80211_stop_ap(rdev, dev, true);
 		break;
 	default:
 		break;
@@ -833,6 +832,36 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 	wdev->beacon_interval = 0;
 }
 
+void cfg80211_leave(struct cfg80211_registered_device *rdev,
+		    struct wireless_dev *wdev)
+{
+	wdev_lock(wdev);
+	__cfg80211_leave(rdev, wdev);
+	wdev_unlock(wdev);
+}
+
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev,
+			 gfp_t gfp)
+{
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+	struct cfg80211_event *ev;
+	unsigned long flags;
+
+	trace_cfg80211_stop_iface(wiphy, wdev);
+
+	ev = kzalloc(sizeof(*ev), gfp);
+	if (!ev)
+		return;
+
+	ev->type = EVENT_STOPPED;
+
+	spin_lock_irqsave(&wdev->event_lock, flags);
+	list_add_tail(&ev->list, &wdev->event_list);
+	spin_unlock_irqrestore(&wdev->event_lock, flags);
+	queue_work(cfg80211_wq, &rdev->event_work);
+}
+EXPORT_SYMBOL(cfg80211_stop_iface);
+
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 					 unsigned long state, void *ptr)
 {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 5bee6cc..5876c8d 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -185,6 +185,7 @@ enum cfg80211_event_type {
 	EVENT_ROAMED,
 	EVENT_DISCONNECTED,
 	EVENT_IBSS_JOINED,
+	EVENT_STOPPED,
 };
 
 struct cfg80211_event {
@@ -281,6 +282,8 @@ int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 		       struct net_device *dev,
 		       struct mesh_setup *setup,
 		       const struct mesh_config *conf);
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev);
 int cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
 			struct net_device *dev);
 int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
@@ -288,6 +291,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 			      struct cfg80211_chan_def *chandef);
 
 /* AP */
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify);
 int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
 		     struct net_device *dev, bool notify);
 
@@ -441,6 +446,8 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 			       enum nl80211_iftype iftype, int num);
 
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev);
 void cfg80211_leave(struct cfg80211_registered_device *rdev,
 		    struct wireless_dev *wdev);
 
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index 3ddfb7c..092300b 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -238,8 +238,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 	return 0;
 }
 
-static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
-				 struct net_device *dev)
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 47b499f..1ccb2a3 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2618,6 +2618,21 @@ TRACE_EVENT(cfg80211_ft_event,
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(target_ap))
 );
 
+TRACE_EVENT(cfg80211_stop_iface,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
+	TP_ARGS(wiphy, wdev),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		WDEV_ENTRY
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		WDEV_ASSIGN;
+	),
+	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
+		  WIPHY_PR_ARG, WDEV_PR_ARG)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/net/wireless/util.c b/net/wireless/util.c
index c5d0208..31b93b9 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -823,6 +823,9 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
 			__cfg80211_ibss_joined(wdev->netdev, ev->ij.bssid,
 					       ev->ij.channel);
 			break;
+		case EVENT_STOPPED:
+			__cfg80211_leave(wiphy_to_dev(wdev->wiphy), wdev);
+			break;
 		}
 		wdev_unlock(wdev);
 
-- 
1.8.5.3

Re: [PATCH v3 3/4] cfg80211: export interface stopping function

[Luca Coelho]

On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
> This exports a new cfg80211_stop_iface() function.
> 
> This is intended for driver internal interface
> combination management and channel switching.
> 
> Due to locking issues (it re-enters driver) the
> call is asynchronous and uses cfg80211 event
> list/worker.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

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

[PATCH v3 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

It doesn't make much sense to leave a cripled
interface running.

As a side effect this will unblock tx queues with
CSA reason immediately after failure instead of
until after userspace requests interface to stop.

This also gives userspace an opportunity to
indirectly see CSA failure.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v3:
 * rebase to use gfp_t argument

 net/mac80211/cfg.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 2fd8e08..821143c 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3088,7 +3088,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
@@ -3100,7 +3100,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
 	if (WARN_ON(err < 0))
-		return;
+		return err;
 
 	if (!local->use_chanctx) {
 		local->_oper_chandef = sdata->csa_chandef;
@@ -3111,7 +3111,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	err = ieee80211_set_after_csa_beacon(sdata, &changed);
 	if (err)
-		return;
+		return err;
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
@@ -3120,6 +3120,8 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+
+	return 0;
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3128,6 +3130,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 		container_of(work, struct ieee80211_sub_if_data,
 			     csa_finalize_work);
 	struct ieee80211_local *local = sdata->local;
+	int err;
 
 	sdata_lock(sdata);
 	mutex_lock(&local->mtx);
@@ -3139,7 +3142,12 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	if (!ieee80211_sdata_running(sdata))
 		goto unlock;
 
-	ieee80211_csa_finalize(sdata);
+	err = ieee80211_csa_finalize(sdata);
+	if (err) {
+		sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
+		cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev, GFP_KERNEL);
+		goto unlock;
+	}
 
 unlock:
 	mutex_unlock(&local->mtx);
-- 
1.8.5.3

Re: [PATCH v3 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Luca Coelho]

On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
> It doesn't make much sense to leave a cripled
> interface running.
> 
> As a side effect this will unblock tx queues with
> CSA reason immediately after failure instead of
> until after userspace requests interface to stop.
> 
> This also gives userspace an opportunity to
> indirectly see CSA failure.
> 
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> ---

[...]

> @@ -3100,7 +3100,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>  	sdata->radar_required = sdata->csa_radar_required;
>  	err = ieee80211_vif_change_channel(sdata, &changed);
>  	if (WARN_ON(err < 0))
> -		return;
> +		return err;

Didn't we agree to remove the WARN_ON() from here?

--
Luca.

Re: [PATCH v3 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

On 31 March 2014 13:45, Luca Coelho <luca@coelho.fi> wrote:
> On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
>> It doesn't make much sense to leave a cripled
>> interface running.
>>
>> As a side effect this will unblock tx queues with
>> CSA reason immediately after failure instead of
>> until after userspace requests interface to stop.
>>
>> This also gives userspace an opportunity to
>> indirectly see CSA failure.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>> ---
>
> [...]
>
>> @@ -3100,7 +3100,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>>       sdata->radar_required = sdata->csa_radar_required;
>>       err = ieee80211_vif_change_channel(sdata, &changed);
>>       if (WARN_ON(err < 0))
>> -             return;
>> +             return err;
>
> Didn't we agree to remove the WARN_ON() from here?

I forgot, sorry. Now I definitely need to respin, again :)


Michał

Re: [PATCH v3 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Johannes Berg]

On Mon, 2014-03-31 at 11:57 +0200, Michal Kazior wrote:
> It doesn't make much sense to leave a cripled

typo: crippled :)

johannes

[PATCH v4 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Michal Kazior]

Hi,

The patchset fixes CSA tx queue locking and
introduces interface teardown upon CSA failure
during finalization.

v2:
 * get rid of idempotency

v3:
 * fixes (see each commit)

v4:
 * commit message fixes
 * remove WARN_ON as discussed

Michal Kazior (4):
  mac80211: fix CSA tx queue locking
  mac80211: split CSA finalize function
  cfg80211: export interface stopping function
  mac80211: disconnect iface if CSA unexpectedly fails

 include/net/cfg80211.h     |  15 +++++
 include/net/mac80211.h     |   4 +-
 net/mac80211/cfg.c         | 144 +++++++++++++++++++++++++++++++++------------
 net/mac80211/ieee80211_i.h |   2 +
 net/mac80211/iface.c       |   7 +++
 net/mac80211/mlme.c        |  37 +++++++++---
 net/wireless/ap.c          |   4 +-
 net/wireless/core.c        |  43 +++++++++++---
 net/wireless/core.h        |   7 +++
 net/wireless/mesh.c        |   4 +-
 net/wireless/trace.h       |  15 +++++
 net/wireless/util.c        |   3 +
 12 files changed, 228 insertions(+), 57 deletions(-)

-- 
1.8.5.3

[PATCH v4 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

It was possible for tx queues to be stuck stopped
if AP CSA finalization failed. In that case
neither stop_ap nor do_stop woke the queues up.
This means it was impossible to perform tx at all
until driver was reloaded or a successful CSA was
performed later.

It was possible to solve this in a simpler manner
however this is more robust and future proof
(having multi-vif CSA in mind).

New sdata->csa_block_tx is introduced to keep
track of which interfaces requested tx to be
blocked for CSA. This is required because mac80211
stops all tx queues for that purpose. This means
queues must be awoken only when last tx-blocking
CSA interface is finished.

It is still possible to have tx queues stopped
after CSA failure but as soon as offending
interfaces are stopped from userspace (stop_ap or
ifdown) tx queues are woken up properly.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * get rid of idempotency [Johannes]

v3:
 * fix wake/stop typo [Luca]
 * move queue waking to csa_finalize() [Luca]
 * tune how queues are woken for STA CSA [Luca]

v4:
 * improve commit message (locked vs stopped) [Luca]

 include/net/mac80211.h     |  4 ++-
 net/mac80211/cfg.c         | 77 ++++++++++++++++++++++++++++++++++++++--------
 net/mac80211/ieee80211_i.h |  2 ++
 net/mac80211/iface.c       |  7 +++++
 net/mac80211/mlme.c        | 37 ++++++++++++++++------
 5 files changed, 104 insertions(+), 23 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a3044e1..2cf9c01 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1113,7 +1113,9 @@ enum ieee80211_vif_flags {
  * @addr: address of this interface
  * @p2p: indicates whether this AP or STA interface is a p2p
  *	interface, i.e. a GO or p2p-sta respectively
- * @csa_active: marks whether a channel switch is going on
+ * @csa_active: marks whether a channel switch is going on. Internally it is
+ *	write-protected by sdata_lock and local->mtx so holding either is fine
+ *	for read access.
  * @driver_flags: flags/capabilities the driver has for this interface,
  *	these need to be set (or cleared) when the interface is added
  *	or, if supported by the driver, the interface type is changed
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aa39381..b7cf8e4 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1084,6 +1084,31 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	lockdep_assert_held(&local->mtx);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+
+		if (!sdata->vif.csa_active)
+			continue;
+
+		if (!sdata->csa_block_tx)
+			continue;
+
+		rcu_read_unlock();
+		return true;
+	}
+	rcu_read_unlock();
+
+	return false;
+}
+
 static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
@@ -1101,7 +1126,14 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
 	old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
 
 	/* abort any running channel switch */
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
+
 	kfree(sdata->u.ap.next_beacon);
 	sdata->u.ap.next_beacon = NULL;
 
@@ -3025,11 +3057,10 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	int err, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
-	mutex_lock(&local->mtx);
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
-	mutex_unlock(&local->mtx);
 	if (WARN_ON(err < 0))
 		return;
 
@@ -3070,11 +3101,12 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
+
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
-
-	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3082,8 +3114,11 @@ 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);
+	struct ieee80211_local *local = sdata->local;
 
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
+
 	/* AP might have been stopped while waiting for the lock. */
 	if (!sdata->vif.csa_active)
 		goto unlock;
@@ -3094,6 +3129,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	ieee80211_csa_finalize(sdata);
 
 unlock:
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
 }
 
@@ -3220,8 +3256,8 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-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;
@@ -3230,6 +3266,7 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	int err, num_chanctx, changed = 0;
 
 	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
 
 	if (!list_empty(&local->roc_list) || local->scanning)
 		return -EBUSY;
@@ -3271,15 +3308,15 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 
 	sdata->csa_radar_required = params->radar_required;
-
-	if (params->block_tx)
-		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
-
 	sdata->csa_chandef = params->chandef;
+	sdata->csa_block_tx = params->block_tx;
 	sdata->vif.csa_active = true;
 
+	if (sdata->csa_block_tx)
+		ieee80211_stop_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+
 	if (changed) {
 		ieee80211_bss_info_change_notify(sdata, changed);
 		drv_channel_switch_beacon(sdata, &params->chandef);
@@ -3291,6 +3328,20 @@ int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	return 0;
 }
 
+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;
+	int err;
+
+	mutex_lock(&local->mtx);
+	err = __ieee80211_channel_switch(wiphy, dev, params);
+	mutex_unlock(&local->mtx);
+
+	return err;
+}
+
 static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 			     struct cfg80211_mgmt_tx_params *params,
 			     u64 *cookie)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 3dd2111..6bfeb22 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -754,6 +754,7 @@ struct ieee80211_sub_if_data {
 	int csa_counter_offset_beacon;
 	int csa_counter_offset_presp;
 	bool csa_radar_required;
+	bool csa_block_tx; /* write-protected by sdata_lock and local->mtx */
 	struct cfg80211_chan_def csa_chandef;
 
 	/* context reservation -- protected with chanctx_mtx */
@@ -1466,6 +1467,7 @@ void ieee80211_sw_roc_work(struct work_struct *work);
 void ieee80211_handle_roc_started(struct ieee80211_roc_work *roc);
 
 /* channel switch handling */
+bool ieee80211_csa_needs_block_tx(struct ieee80211_local *local);
 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);
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 90b6063..d6230e2 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -838,8 +838,15 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	cancel_work_sync(&sdata->recalc_smps);
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
+
 	cancel_work_sync(&sdata->csa_finalize_work);
 
 	cancel_delayed_work_sync(&sdata->dfs_cac_timer_work);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index dee50ae..8702bc4 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -975,16 +975,20 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->csa_chandef.chan;
 
+	ieee80211_bss_info_change_notify(sdata, changed);
+
+	mutex_lock(&local->mtx);
+	sdata->vif.csa_active = false;
 	/* XXX: wait for a beacon first? */
-	ieee80211_wake_queues_by_reason(&local->hw,
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
-	ieee80211_bss_info_change_notify(sdata, changed);
-
- out:
-	sdata->vif.csa_active = false;
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+
+out:
 	sdata_unlock(sdata);
 }
 
@@ -1100,12 +1104,16 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	mutex_unlock(&local->chanctx_mtx);
 
 	sdata->csa_chandef = csa_ie.chandef;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = true;
+	sdata->csa_block_tx = csa_ie.mode;
 
-	if (csa_ie.mode)
+	if (sdata->csa_block_tx)
 		ieee80211_stop_queues_by_reason(&local->hw,
-				IEEE80211_MAX_QUEUE_MAP,
-				IEEE80211_QUEUE_STOP_REASON_CSA);
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
 	if (local->ops->channel_switch) {
 		/* use driver's channel switch callback */
@@ -1817,6 +1825,12 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	ifmgd->flags = 0;
 	mutex_lock(&local->mtx);
 	ieee80211_vif_release_channel(sdata);
+
+	sdata->vif.csa_active = false;
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CSA);
 	mutex_unlock(&local->mtx);
 
 	sdata->encrypt_headroom = IEEE80211_ENCRYPT_HEADROOM;
@@ -2045,6 +2059,7 @@ EXPORT_SYMBOL(ieee80211_ap_probereq_get);
 
 static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 {
+	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
 
@@ -2058,10 +2073,14 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 			       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
 			       true, frame_buf);
 	ifmgd->flags &= ~IEEE80211_STA_CSA_RECEIVED;
+
+	mutex_lock(&local->mtx);
 	sdata->vif.csa_active = false;
-	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+	if (!ieee80211_csa_needs_block_tx(local))
+		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+	mutex_unlock(&local->mtx);
 
 	cfg80211_tx_mlme_mgmt(sdata->dev, frame_buf,
 			      IEEE80211_DEAUTH_FRAME_LEN);
-- 
1.8.5.3

Re: [PATCH v4 1/4] mac80211: fix CSA tx queue locking

[Luca Coelho]

On Wed, 2014-04-09 at 15:10 +0200, Michal Kazior wrote:
> v4:
>  * improve commit message (locked vs stopped) [Luca]

Maybe the subject should be changed too? But I think either Johannes or
I (whoever apply the series) can easily fix it up while applying...

--
Luca.

Re: [PATCH v4 1/4] mac80211: fix CSA tx queue locking

[Michal Kazior]

On 9 April 2014 15:22, Luca Coelho <luca@coelho.fi> wrote:
> On Wed, 2014-04-09 at 15:10 +0200, Michal Kazior wrote:
>> v4:
>>  * improve commit message (locked vs stopped) [Luca]
>
> Maybe the subject should be changed too? But I think either Johannes or
> I (whoever apply the series) can easily fix it up while applying...

/me grabs my head

Please do. Thanks!


Michał

[PATCH v4 2/4] mac80211: split CSA finalize function

[Michal Kazior]

Improves readability and modularity.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/mac80211/cfg.c | 61 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b7cf8e4..2fd8e08 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3051,25 +3051,11 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif)
 }
 EXPORT_SYMBOL(ieee80211_csa_finish);
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
+					  u32 *changed)
 {
-	struct ieee80211_local *local = sdata->local;
-	int err, changed = 0;
-
-	sdata_assert_lock(sdata);
-	lockdep_assert_held(&local->mtx);
-
-	sdata->radar_required = sdata->csa_radar_required;
-	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (WARN_ON(err < 0))
-		return;
-
-	if (!local->use_chanctx) {
-		local->_oper_chandef = sdata->csa_chandef;
-		ieee80211_hw_config(local, 0);
-	}
+	int err;
 
-	sdata->vif.csa_active = false;
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_AP:
 		err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon);
@@ -3077,30 +3063,57 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		sdata->u.ap.next_beacon = NULL;
 
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 	case NL80211_IFTYPE_ADHOC:
 		err = ieee80211_ibss_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #ifdef CONFIG_MAC80211_MESH
 	case NL80211_IFTYPE_MESH_POINT:
 		err = ieee80211_mesh_finish_csa(sdata);
 		if (err < 0)
-			return;
-		changed |= err;
+			return err;
+		*changed |= err;
 		break;
 #endif
 	default:
 		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 changed = 0;
+	int err;
+
+	sdata_assert_lock(sdata);
+	lockdep_assert_held(&local->mtx);
+
+	sdata->radar_required = sdata->csa_radar_required;
+	err = ieee80211_vif_change_channel(sdata, &changed);
+	if (WARN_ON(err < 0))
 		return;
+
+	if (!local->use_chanctx) {
+		local->_oper_chandef = sdata->csa_chandef;
+		ieee80211_hw_config(local, 0);
 	}
 
-	ieee80211_bss_info_change_notify(sdata, changed);
+	sdata->vif.csa_active = false;
+
+	err = ieee80211_set_after_csa_beacon(sdata, &changed);
+	if (err)
+		return;
 
+	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
 
 	if (!ieee80211_csa_needs_block_tx(local))
-- 
1.8.5.3

[PATCH v4 3/4] cfg80211: export interface stopping function

[Michal Kazior]

This exports a new cfg80211_stop_iface() function.

This is intended for driver internal interface
combination management and channel switching.

Due to locking issues (it re-enters driver) the
call is asynchronous and uses cfg80211 event
list/worker.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v3:
 * improve function comment [Luca]
 * add gfp_t argument [Luca]
 * remove redundant lockdep checks [Arend]

 include/net/cfg80211.h | 15 +++++++++++++++
 net/wireless/ap.c      |  4 ++--
 net/wireless/core.c    | 43 ++++++++++++++++++++++++++++++++++++-------
 net/wireless/core.h    |  7 +++++++
 net/wireless/mesh.c    |  4 ++--
 net/wireless/trace.h   | 15 +++++++++++++++
 net/wireless/util.c    |  3 +++
 7 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 9496fe5..6deebc8 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4715,6 +4715,21 @@ int cfg80211_check_combinations(struct wiphy *wiphy,
 				const u8 radar_detect,
 				const int iftype_num[NUM_NL80211_IFTYPES]);
 
+/*
+ * cfg80211_stop_iface - trigger interface disconnection
+ *
+ * @wiphy: the wiphy
+ * @wdev: wireless device
+ * @gfp: context flags
+ *
+ * Trigger interface to be stopped as if AP was stopped, IBSS/mesh left, STA
+ * disconnected.
+ *
+ * Note: This doesn't need any locks and is asynchronous.
+ */
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev,
+			 gfp_t gfp);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/wireless/ap.c b/net/wireless/ap.c
index 3e02ade..bdad1f9 100644
--- a/net/wireless/ap.c
+++ b/net/wireless/ap.c
@@ -6,8 +6,8 @@
 #include "rdev-ops.h"
 
 
-static int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
-			      struct net_device *dev, bool notify)
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 33d12e2..87d9502 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -792,23 +792,23 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 		rdev->num_running_monitor_ifaces += num;
 }
 
-void cfg80211_leave(struct cfg80211_registered_device *rdev,
-		    struct wireless_dev *wdev)
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev)
 {
 	struct net_device *dev = wdev->netdev;
 
 	ASSERT_RTNL();
+	ASSERT_WDEV_LOCK(wdev);
 
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
-		cfg80211_leave_ibss(rdev, dev, true);
+		__cfg80211_leave_ibss(rdev, dev, true);
 		break;
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_STATION:
 		if (rdev->sched_scan_req && dev == rdev->sched_scan_req->dev)
 			__cfg80211_stop_sched_scan(rdev, false);
 
-		wdev_lock(wdev);
 #ifdef CONFIG_CFG80211_WEXT
 		kfree(wdev->wext.ie);
 		wdev->wext.ie = NULL;
@@ -817,20 +817,49 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 #endif
 		cfg80211_disconnect(rdev, dev,
 				    WLAN_REASON_DEAUTH_LEAVING, true);
-		wdev_unlock(wdev);
 		break;
 	case NL80211_IFTYPE_MESH_POINT:
-		cfg80211_leave_mesh(rdev, dev);
+		__cfg80211_leave_mesh(rdev, dev);
 		break;
 	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_P2P_GO:
-		cfg80211_stop_ap(rdev, dev, true);
+		__cfg80211_stop_ap(rdev, dev, true);
 		break;
 	default:
 		break;
 	}
 }
 
+void cfg80211_leave(struct cfg80211_registered_device *rdev,
+		    struct wireless_dev *wdev)
+{
+	wdev_lock(wdev);
+	__cfg80211_leave(rdev, wdev);
+	wdev_unlock(wdev);
+}
+
+void cfg80211_stop_iface(struct wiphy *wiphy, struct wireless_dev *wdev,
+			 gfp_t gfp)
+{
+	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
+	struct cfg80211_event *ev;
+	unsigned long flags;
+
+	trace_cfg80211_stop_iface(wiphy, wdev);
+
+	ev = kzalloc(sizeof(*ev), gfp);
+	if (!ev)
+		return;
+
+	ev->type = EVENT_STOPPED;
+
+	spin_lock_irqsave(&wdev->event_lock, flags);
+	list_add_tail(&ev->list, &wdev->event_list);
+	spin_unlock_irqrestore(&wdev->event_lock, flags);
+	queue_work(cfg80211_wq, &rdev->event_work);
+}
+EXPORT_SYMBOL(cfg80211_stop_iface);
+
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 					 unsigned long state, void *ptr)
 {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 5bee6cc..5876c8d 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -185,6 +185,7 @@ enum cfg80211_event_type {
 	EVENT_ROAMED,
 	EVENT_DISCONNECTED,
 	EVENT_IBSS_JOINED,
+	EVENT_STOPPED,
 };
 
 struct cfg80211_event {
@@ -281,6 +282,8 @@ int cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 		       struct net_device *dev,
 		       struct mesh_setup *setup,
 		       const struct mesh_config *conf);
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev);
 int cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
 			struct net_device *dev);
 int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
@@ -288,6 +291,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 			      struct cfg80211_chan_def *chandef);
 
 /* AP */
+int __cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
+		       struct net_device *dev, bool notify);
 int cfg80211_stop_ap(struct cfg80211_registered_device *rdev,
 		     struct net_device *dev, bool notify);
 
@@ -441,6 +446,8 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 			       enum nl80211_iftype iftype, int num);
 
+void __cfg80211_leave(struct cfg80211_registered_device *rdev,
+		      struct wireless_dev *wdev);
 void cfg80211_leave(struct cfg80211_registered_device *rdev,
 		    struct wireless_dev *wdev);
 
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index 3ddfb7c..092300b 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -238,8 +238,8 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 	return 0;
 }
 
-static int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
-				 struct net_device *dev)
+int __cfg80211_leave_mesh(struct cfg80211_registered_device *rdev,
+			  struct net_device *dev)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	int err;
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 47b499f..1ccb2a3 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2618,6 +2618,21 @@ TRACE_EVENT(cfg80211_ft_event,
 		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(target_ap))
 );
 
+TRACE_EVENT(cfg80211_stop_iface,
+	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
+	TP_ARGS(wiphy, wdev),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		WDEV_ENTRY
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		WDEV_ASSIGN;
+	),
+	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
+		  WIPHY_PR_ARG, WDEV_PR_ARG)
+);
+
 #endif /* !__RDEV_OPS_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/net/wireless/util.c b/net/wireless/util.c
index d032a31..ea4e28d 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -839,6 +839,9 @@ void cfg80211_process_wdev_events(struct wireless_dev *wdev)
 			__cfg80211_ibss_joined(wdev->netdev, ev->ij.bssid,
 					       ev->ij.channel);
 			break;
+		case EVENT_STOPPED:
+			__cfg80211_leave(wiphy_to_dev(wdev->wiphy), wdev);
+			break;
 		}
 		wdev_unlock(wdev);
 
-- 
1.8.5.3

[PATCH v4 4/4] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

It doesn't make much sense to leave a crippled
interface running.

As a side effect this will unblock tx queues with
CSA reason immediately after failure instead of
until after userspace requests interface to stop.

This also gives userspace an opportunity to
indirectly see CSA failure.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v3:
 * rebase to use gfp_t argument

v4:
 * remove WARN_ON from ieee80211_vif_change_channel() error checking [Luca]
 * fix typo: s/cripled/crippled/ [Johannes]

 net/mac80211/cfg.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 2fd8e08..5bd4a81 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3088,7 +3088,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
@@ -3099,8 +3099,8 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (WARN_ON(err < 0))
-		return;
+	if (err < 0)
+		return err;
 
 	if (!local->use_chanctx) {
 		local->_oper_chandef = sdata->csa_chandef;
@@ -3111,7 +3111,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	err = ieee80211_set_after_csa_beacon(sdata, &changed);
 	if (err)
-		return;
+		return err;
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
@@ -3120,6 +3120,8 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+
+	return 0;
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
@@ -3128,6 +3130,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 		container_of(work, struct ieee80211_sub_if_data,
 			     csa_finalize_work);
 	struct ieee80211_local *local = sdata->local;
+	int err;
 
 	sdata_lock(sdata);
 	mutex_lock(&local->mtx);
@@ -3139,7 +3142,12 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	if (!ieee80211_sdata_running(sdata))
 		goto unlock;
 
-	ieee80211_csa_finalize(sdata);
+	err = ieee80211_csa_finalize(sdata);
+	if (err) {
+		sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
+		cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev, GFP_KERNEL);
+		goto unlock;
+	}
 
 unlock:
 	mutex_unlock(&local->mtx);
-- 
1.8.5.3

[PATCH v5] mac80211: disconnect iface if CSA unexpectedly fails

[Michal Kazior]

It doesn't make much sense to leave a crippled
interface running.

As a side effect this will unblock tx queues with
CSA reason immediately after failure instead of
until after userspace requests interface to stop.

This also gives userspace an opportunity to
indirectly see CSA failure.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
This was originally a 4/4 of csa fixes patchset.

Changelog:

v3:
 * rebase to use gfp_t argument

v4:
 * remove WARN_ON from ieee80211_vif_change_channel() error checking [Luca]
 * fix typo: s/cripled/crippled/ [Johannes]

v5:
 * fix csa failure for immediate csa.
   ieee80211_csa_finalize() wasn't error checked
   in __ieee80211_channel_switch.
   __ieee80211_csa_finalize() now does what
   ieee80211_csa_finalize() originally did, while
   the latter now performs the error check to avoid
   code duplication [Johannes]

 net/mac80211/cfg.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 0c87c8c..e028a2b 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3090,7 +3090,7 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata,
 	return 0;
 }
 
-static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_local *local = sdata->local;
 	u32 changed = 0;
@@ -3101,8 +3101,8 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	sdata->radar_required = sdata->csa_radar_required;
 	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (WARN_ON(err < 0))
-		return;
+	if (err < 0)
+		return err;
 
 	if (!local->use_chanctx) {
 		local->_oper_chandef = sdata->csa_chandef;
@@ -3113,7 +3113,7 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	err = ieee80211_set_after_csa_beacon(sdata, &changed);
 	if (err)
-		return;
+		return err;
 
 	ieee80211_bss_info_change_notify(sdata, changed);
 	cfg80211_ch_switch_notify(sdata->dev, &sdata->csa_chandef);
@@ -3122,6 +3122,21 @@ static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 		ieee80211_wake_queues_by_reason(&local->hw,
 					IEEE80211_MAX_QUEUE_MAP,
 					IEEE80211_QUEUE_STOP_REASON_CSA);
+
+	return 0;
+}
+
+static void ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
+{
+	int err;
+
+	err = __ieee80211_csa_finalize(sdata);
+	if (err) {
+		sdata_info(sdata, "failed to finalize CSA, disconnecting\n");
+		cfg80211_stop_iface(sdata->local->hw.wiphy, &sdata->wdev,
+				    GFP_KERNEL);
+		return;
+	}
 }
 
 void ieee80211_csa_finalize_work(struct work_struct *work)
-- 
1.8.5.3

Re: [PATCH v5] mac80211: disconnect iface if CSA unexpectedly fails

[Johannes Berg]

On Thu, 2014-05-08 at 09:10 +0200, Michal Kazior wrote:
> It doesn't make much sense to leave a crippled
> interface running.
> 
> As a side effect this will unblock tx queues with
> CSA reason immediately after failure instead of
> until after userspace requests interface to stop.
> 
> This also gives userspace an opportunity to
> indirectly see CSA failure.

Applied, I did a small cleanup to the new function.

johannes

Re: [PATCH v4 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Johannes Berg]

On Wed, 2014-04-09 at 15:10 +0200, Michal Kazior wrote:
> Hi,
> 
> The patchset fixes CSA tx queue locking and
> introduces interface teardown upon CSA failure
> during finalization.

Applied (with the rename Luca suggested), thanks.

However, there's one more thing - one caller of ieee80211_csa_finalize()
doesn't check the return value now, I think it probably should. Mind
taking a look at that? It should *probably* be the same as the other
caller (and not propagate the error), I'd say, so maybe that should be
rolled into the function?

Thanks,
johannes

Re: [PATCH v4 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Michal Kazior]

On 6 May 2014 15:15, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2014-04-09 at 15:10 +0200, Michal Kazior wrote:
>> Hi,
>>
>> The patchset fixes CSA tx queue locking and
>> introduces interface teardown upon CSA failure
>> during finalization.
>
> Applied (with the rename Luca suggested), thanks.
>
> However, there's one more thing - one caller of ieee80211_csa_finalize()
> doesn't check the return value now, I think it probably should. Mind
> taking a look at that? It should *probably* be the same as the other
> caller (and not propagate the error), I'd say, so maybe that should be
> rolled into the function?

Right - the return value of ieee80211_csa_finalize() in
__ieee80211_channel_switch() should be checked against. I think it
should be like:

 err = ieee80211_csa_finalize(sdata);
 if (err) {
  sdata_info(sdata, "failed to finalize immediate CSA, disconnecting\n");
  cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev, GFP_KERNEL);

  /* don't propagate the error here - stop the iface instead since
there's no way to revert CS now */
  err = 0;
 }

Should I re-post the patch(set?), post a follow up, or are you going
to update the patch yourself?

I'm not sure what you mean by rolling "that" into the function. You
want to put the stop_iface() in csa_finalize()? I think it's going to
look a bit ugly that way.


Michał

Re: [PATCH v4 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Johannes Berg]

On Wed, 2014-05-07 at 09:04 +0200, Michal Kazior wrote:

> > However, there's one more thing - one caller of ieee80211_csa_finalize()
> > doesn't check the return value now, I think it probably should. Mind
> > taking a look at that? It should *probably* be the same as the other
> > caller (and not propagate the error), I'd say, so maybe that should be
> > rolled into the function?
> 
> Right - the return value of ieee80211_csa_finalize() in
> __ieee80211_channel_switch() should be checked against. I think it
> should be like:
> 
>  err = ieee80211_csa_finalize(sdata);
>  if (err) {
>   sdata_info(sdata, "failed to finalize immediate CSA, disconnecting\n");
>   cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev, GFP_KERNEL);
> 
>   /* don't propagate the error here - stop the iface instead since
> there's no way to revert CS now */
>   err = 0;
>  }
> 
> Should I re-post the patch(set?), post a follow up, or are you going
> to update the patch yourself?

Can you post a separate patch on top? I already have it in my tree, but
I can squash it I guess if I really want to.

> I'm not sure what you mean by rolling "that" into the function. You
> want to put the stop_iface() in csa_finalize()? I think it's going to
> look a bit ugly that way.

Well, the only other caller of it right now does the same thing:

       err = ieee80211_csa_finalize(sdata);
       if (err) {
               sdata_info(sdata, "failed to finalize CSA, disconnecting
\n");
               cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev,
GFP_KERNEL);
               goto unlock;
       }

so it seems it would make sense to have that as common code, maybe in
the ieee80211_csa_finalize() function or maybe by changing
ieee80211_csa_finalize() to be __ieee80211_csa_finalize() and making a
new ieee80211_csa_finalize() that does this step?

In fact, then we could get rid of the int return value again, so maybe
better post a new version of this particular patch after all.

johannes

Re: [PATCH v4 0/4] mac80211/cfg80211: CSA fixes/cleanups

[Michal Kazior]

On 7 May 2014 10:59, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2014-05-07 at 09:04 +0200, Michal Kazior wrote:
>
>> > However, there's one more thing - one caller of ieee80211_csa_finalize()
>> > doesn't check the return value now, I think it probably should. Mind
>> > taking a look at that? It should *probably* be the same as the other
>> > caller (and not propagate the error), I'd say, so maybe that should be
>> > rolled into the function?
>>
>> Right - the return value of ieee80211_csa_finalize() in
>> __ieee80211_channel_switch() should be checked against. I think it
>> should be like:
>>
>>  err = ieee80211_csa_finalize(sdata);
>>  if (err) {
>>   sdata_info(sdata, "failed to finalize immediate CSA, disconnecting\n");
>>   cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev, GFP_KERNEL);
>>
>>   /* don't propagate the error here - stop the iface instead since
>> there's no way to revert CS now */
>>   err = 0;
>>  }
>>
>> Should I re-post the patch(set?), post a follow up, or are you going
>> to update the patch yourself?
>
> Can you post a separate patch on top? I already have it in my tree, but
> I can squash it I guess if I really want to.
>
>> I'm not sure what you mean by rolling "that" into the function. You
>> want to put the stop_iface() in csa_finalize()? I think it's going to
>> look a bit ugly that way.
>
> Well, the only other caller of it right now does the same thing:
>
>        err = ieee80211_csa_finalize(sdata);
>        if (err) {
>                sdata_info(sdata, "failed to finalize CSA, disconnecting
> \n");
>                cfg80211_stop_iface(local->hw.wiphy, &sdata->wdev,
> GFP_KERNEL);
>                goto unlock;
>        }
>
> so it seems it would make sense to have that as common code, maybe in
> the ieee80211_csa_finalize() function or maybe by changing
> ieee80211_csa_finalize() to be __ieee80211_csa_finalize() and making a
> new ieee80211_csa_finalize() that does this step?

Yeah, this sounds reasonable.

At times I'm just out of ideas on function naming for CSA. There's
like 5 functions that perform various phases of CSA finalization :-)


> In fact, then we could get rid of the int return value again, so maybe
> better post a new version of this particular patch after all.

I'll re-spin the patch later then.


Michał