[PATCH v6 0/6] cfg/mac80211: implement multi-vif csa

[PATCH v6 0/6] cfg/mac80211: implement multi-vif csa

[Michal Kazior]

Hi,

I've added a switch_vif_chanctx patch as discussed
earlier and refactored the multi-vif reservations.

I've merged that with my other multi-vif csa
patches as to not post hundreds of patchsets
separately.

This is based on my csa counter fixes. Otherwise
there are 2 or 3 conflicts.


Michal Kazior (6):
  mac80211: introduce switch_vif_chanctx op
  mac80211: implement multi-vif in-place reservations
  mac80211: make check_combinations() aware of chanctx reservation
  mac80211: use chanctx reservation for AP CSA
  mac80211: use chanctx reservation for STA CSA
  cfg80211: remove channel_switch combination check

 include/net/mac80211.h     |  36 +++-
 net/mac80211/cfg.c         |  82 ++++++---
 net/mac80211/chan.c        | 407 ++++++++++++++++++++++++++++++---------------
 net/mac80211/driver-ops.h  |  36 ++++
 net/mac80211/ieee80211_i.h |   9 +-
 net/mac80211/main.c        |   2 +
 net/mac80211/mlme.c        |  94 +++++++----
 net/mac80211/trace.h       | 111 +++++++++++++
 net/mac80211/util.c        |  39 ++++-
 net/wireless/nl80211.c     |  11 --
 10 files changed, 602 insertions(+), 225 deletions(-)

-- 
1.8.5.3

[PATCH v6 1/6] mac80211: introduce switch_vif_chanctx op

[Michal Kazior]

This new device driver operation will be used for
channel context reservation and switching.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 include/net/mac80211.h    |  28 ++++++++++++
 net/mac80211/driver-ops.h |  36 +++++++++++++++
 net/mac80211/main.c       |   2 +
 net/mac80211/trace.h      | 111 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 177 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a34f26a..831607e 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -161,6 +161,23 @@ enum ieee80211_chanctx_change {
 };
 
 /**
+ * enum ieee80211_chanctx_swmode - channel switch mode
+ * @IEEE80211_CHANCTX_SWMODE_REASSIGN: simply re-assign chanctx to/from vifs.
+ *	Both contexts in old_ctx and new_ctx are already known to device driver.
+ * @IEEE80211_CHANCTX_SWMODE_SWAP: contexts in old_ctx are known to device
+ *	driver and are supposed to be treated as if remove_chanctx() was called for
+ *	each on success. Contexts in new_ctx aren't known to device driver at
+ *	call time and are supposed to be treated as if add_chanctx() was called
+ *	for each on success. This is used for channel switching when there are
+ *	no channel context allocations possible and contexts must be swapped
+ *	"in place".
+ */
+enum ieee80211_chanctx_swmode {
+	IEEE80211_CHANCTX_SWMODE_REASSIGN,
+	IEEE80211_CHANCTX_SWMODE_SWAP,
+};
+
+/**
  * struct ieee80211_chanctx_conf - channel context that vifs may be tuned to
  *
  * This is the driver-visible part. The ieee80211_chanctx
@@ -2736,6 +2753,11 @@ enum ieee80211_roc_type {
  *	to vif. Possible use is for hw queue remapping.
  * @unassign_vif_chanctx: Notifies device driver about channel context being
  *	unbound from vif.
+ * @switch_vif_chanctx: Requests driver to perform a channel switch. It passes
+ *	a list of triplets (vif, oldctx, newctx). Driver should try to restore
+ *	pre-call state if it fails.
+ *	The callback is optional and can sleep.
+ *
  * @start_ap: Start operation on the AP interface, this is called after all the
  *	information in bss_conf is set and beacon can be retrieved. A channel
  *	context is bound before this is called. Note that if the driver uses
@@ -2948,6 +2970,12 @@ struct ieee80211_ops {
 	void (*unassign_vif_chanctx)(struct ieee80211_hw *hw,
 				     struct ieee80211_vif *vif,
 				     struct ieee80211_chanctx_conf *ctx);
+	int (*switch_vif_chanctx)(struct ieee80211_hw *hw,
+				  struct ieee80211_vif **vifs,
+				  struct ieee80211_chanctx_conf **old_ctx,
+				  struct ieee80211_chanctx_conf **new_ctx,
+				  int n_vifs,
+				  enum ieee80211_chanctx_swmode swmode);
 
 	void (*restart_complete)(struct ieee80211_hw *hw);
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index df1d502..8e8c290 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1048,6 +1048,42 @@ static inline void drv_unassign_vif_chanctx(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
+#define IEEE80211_MAX_NUM_SWITCH_VIFS 8
+
+static inline int drv_switch_vif_chanctx(struct ieee80211_local *local,
+					 struct ieee80211_sub_if_data **sdata,
+					 struct ieee80211_chanctx **old_ctx,
+					 struct ieee80211_chanctx **new_ctx,
+					 int n_vifs,
+					 enum ieee80211_chanctx_swmode swmode)
+{
+	struct ieee80211_vif *vifs[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
+	struct ieee80211_chanctx_conf *old_conf[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
+	struct ieee80211_chanctx_conf *new_conf[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
+	int i, ret = 0;
+
+	if (local->ops->switch_vif_chanctx)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < n_vifs; i++)
+		if (!check_sdata_in_driver(sdata[i]))
+			return -EIO;
+
+	for (i = 0; i < n_vifs; i++) {
+		trace_drv_switch_vif_chanctx(local, sdata[i], old_ctx[i],
+					     new_ctx[i], n_vifs, swmode, i);
+
+		vifs[i] = &sdata[i]->vif;
+		old_conf[i] = &old_ctx[i]->conf;
+		new_conf[i] = &new_ctx[i]->conf;
+	}
+
+	ret = local->ops->switch_vif_chanctx(&local->hw, vifs, old_conf,
+					     new_conf, n_vifs, swmode);
+	trace_drv_return_int(local, ret);
+	return ret;
+}
+
 static inline int drv_start_ap(struct ieee80211_local *local,
 			       struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 767335f..acdb4b5 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -502,6 +502,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 	if (WARN_ON(i != 0 && i != 5))
 		return NULL;
 	use_chanctx = i == 5;
+	if (WARN_ON(!use_chanctx && ops->switch_vif_chanctx))
+		return NULL;
 
 	/* Ensure 32-byte alignment of our private data and hw private data.
 	 * We use the wiphy priv data for both our ieee80211_local and for
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index a0b0aea..c9bb678 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1414,6 +1414,117 @@ DEFINE_EVENT(local_sdata_chanctx, drv_unassign_vif_chanctx,
 	TP_ARGS(local, sdata, ctx)
 );
 
+TRACE_EVENT(drv_switch_vif_chanctx,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_sub_if_data *sdata,
+		 struct ieee80211_chanctx *old_ctx,
+		 struct ieee80211_chanctx *new_ctx,
+		 int n_vifs,
+		 enum ieee80211_chanctx_swmode swmode,
+		 int i),
+
+	TP_ARGS(local, sdata, old_ctx, new_ctx, n_vifs, swmode, i),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		VIF_ENTRY
+
+		__field(u32, old_control_freq)
+		__field(u32, old_chan_width)
+		__field(u32, old_center_freq1)
+		__field(u32, old_center_freq2)
+		__field(u32, old_min_control_freq)
+		__field(u32, old_min_chan_width)
+		__field(u32, old_min_center_freq1)
+		__field(u32, old_min_center_freq2)
+		__field(u8, old_rx_chains_static)
+		__field(u8, old_rx_chains_dynamic)
+
+		__field(u32, new_control_freq)
+		__field(u32, new_chan_width)
+		__field(u32, new_center_freq1)
+		__field(u32, new_center_freq2)
+		__field(u32, new_min_control_freq)
+		__field(u32, new_min_chan_width)
+		__field(u32, new_min_center_freq1)
+		__field(u32, new_min_center_freq2)
+		__field(u8, new_rx_chains_static)
+		__field(u8, new_rx_chains_dynamic)
+
+		__field(u32, n_vifs)
+		__field(u32, swmode)
+		__field(u32, i)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		VIF_ASSIGN;
+
+		__entry->old_control_freq = old_ctx->conf.def.chan ? old_ctx->conf.def.chan->center_freq : 0;
+		__entry->old_chan_width = old_ctx->conf.def.width;
+		__entry->old_center_freq1 = old_ctx->conf.def.center_freq1;
+		__entry->old_center_freq2 = old_ctx->conf.def.center_freq2;
+		__entry->old_min_control_freq = old_ctx->conf.min_def.chan ? old_ctx->conf.min_def.chan->center_freq : 0;
+		__entry->old_min_chan_width = old_ctx->conf.min_def.width;
+		__entry->old_min_center_freq1 = old_ctx->conf.min_def.center_freq1;
+		__entry->old_min_center_freq2 = old_ctx->conf.min_def.center_freq2;
+		__entry->old_rx_chains_static = old_ctx->conf.rx_chains_static;
+		__entry->old_rx_chains_dynamic = old_ctx->conf.rx_chains_dynamic;
+
+		__entry->new_control_freq = new_ctx->conf.def.chan ? new_ctx->conf.def.chan->center_freq : 0;
+		__entry->new_chan_width = new_ctx->conf.def.width;
+		__entry->new_center_freq1 = new_ctx->conf.def.center_freq1;
+		__entry->new_center_freq2 = new_ctx->conf.def.center_freq2;
+		__entry->new_min_control_freq = new_ctx->conf.min_def.chan ? new_ctx->conf.min_def.chan->center_freq : 0;
+		__entry->new_min_chan_width = new_ctx->conf.min_def.width;
+		__entry->new_min_center_freq1 = new_ctx->conf.min_def.center_freq1;
+		__entry->new_min_center_freq2 = new_ctx->conf.min_def.center_freq2;
+		__entry->new_rx_chains_static = new_ctx->conf.rx_chains_static;
+		__entry->new_rx_chains_dynamic = new_ctx->conf.rx_chains_dynamic;
+
+		__entry->n_vifs = n_vifs;
+		__entry->swmode = swmode;
+		__entry->i = i;
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT VIF_PR_FMT
+		" old_ctx:"
+		" control:%d MHz width:%d center: %d/%d MHz"
+		" min_control:%d MHz min_width:%d min_center: %d/%d MHz"
+		" chains:%d/%d"
+		" new_ctx:"
+		" control:%d MHz width:%d center: %d/%d MHz"
+		" min_control:%d MHz min_width:%d min_center: %d/%d MHz"
+		" chains:%d/%d"
+		" n_vifs:%d swmode:%d i:%d",
+		LOCAL_PR_ARG, VIF_PR_ARG,
+		__entry->old_control_freq,
+		__entry->old_chan_width,
+		__entry->old_center_freq1,
+		__entry->old_center_freq2,
+		__entry->old_min_control_freq,
+		__entry->old_min_chan_width,
+		__entry->old_min_center_freq1,
+		__entry->old_min_center_freq2,
+		__entry->old_rx_chains_static,
+		__entry->old_rx_chains_dynamic,
+		__entry->new_control_freq,
+		__entry->new_chan_width,
+		__entry->new_center_freq1,
+		__entry->new_center_freq2,
+		__entry->new_min_control_freq,
+		__entry->new_min_chan_width,
+		__entry->new_min_center_freq1,
+		__entry->new_min_center_freq2,
+		__entry->new_rx_chains_static,
+		__entry->new_rx_chains_dynamic,
+		__entry->n_vifs,
+		__entry->swmode,
+		__entry->i
+	)
+);
+
 TRACE_EVENT(drv_start_ap,
 	TP_PROTO(struct ieee80211_local *local,
 		 struct ieee80211_sub_if_data *sdata,
-- 
1.8.5.3

Re: [PATCH v6 1/6] mac80211: introduce switch_vif_chanctx op

[Johannes Berg]

On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
> This new device driver operation will be used for
> channel context reservation and switching.

Heh. Luca just had some very similar code. But that's not necessarily a
bad thing - we can compare :)

> +	int (*switch_vif_chanctx)(struct ieee80211_hw *hw,
> +				  struct ieee80211_vif **vifs,
> +				  struct ieee80211_chanctx_conf **old_ctx,
> +				  struct ieee80211_chanctx_conf **new_ctx,
> +				  int n_vifs,
> +				  enum ieee80211_chanctx_swmode swmode);

Luca had a struct here with (vif, old, new), I think that makes sense.

> +#define IEEE80211_MAX_NUM_SWITCH_VIFS 8

:-)

That seems artificial though - why not dynamically allocate?

> +static inline int drv_switch_vif_chanctx(struct ieee80211_local *local,
> +					 struct ieee80211_sub_if_data **sdata,
> +					 struct ieee80211_chanctx **old_ctx,
> +					 struct ieee80211_chanctx **new_ctx,
> +					 int n_vifs,
> +					 enum ieee80211_chanctx_swmode swmode)
> +{
> +	struct ieee80211_vif *vifs[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
> +	struct ieee80211_chanctx_conf *old_conf[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
> +	struct ieee80211_chanctx_conf *new_conf[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
> +	int i, ret = 0;

That's a little big for an inline?

> +	if (local->ops->switch_vif_chanctx)
> +		return -EOPNOTSUPP;
> +
> +	for (i = 0; i < n_vifs; i++)
> +		if (!check_sdata_in_driver(sdata[i]))
> +			return -EIO;
> +
> +	for (i = 0; i < n_vifs; i++) {
> +		trace_drv_switch_vif_chanctx(local, sdata[i], old_ctx[i],
> +					     new_ctx[i], n_vifs, swmode, i);

Hmm. This is somewhat ugly since the loop always runs. In theory it's
possible to do this all with dynamic_array() and code in the assign path
of the tracepoint, I think that'd be better. Or even for now just leave
the tracing to have just a subset or something (e.g. at most 2
interfaces)

> +++ b/net/mac80211/main.c
> @@ -502,6 +502,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
>  	if (WARN_ON(i != 0 && i != 5))
>  		return NULL;
>  	use_chanctx = i == 5;
> +	if (WARN_ON(!use_chanctx && ops->switch_vif_chanctx))
> +		return NULL;

I don't think this makes sense - we perfectly handle the case right now
by disconnecting (and not advertising switch to userspace, I guess? if
not we should)

Requiring drivers to implement this just makes things more difficult,
and the channel switch isn't really mandatory spec-wise.

> +		__field(u32, old_control_freq)

I believe there's a macro for a chandef?

johannes

[PATCH] mac80211: add a single-transaction driver op to switch contexts

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

In some cases, when the driver is already using all the channel
contexts it can handle at once, we have to do an in-place switch
(ie. we cannot afford using an extra context temporarily for the
transaction).  But some drivers may not support switching the channel
context assigned to a vif on the fly (ie. without unassigning and
assigning it) while others may only work if the context is changed on
the fly, without unassigning it first.

To allow these different scenarios, add a new driver operation that
let's the driver decide how to handle an in-place switch.

Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
since we never change a running context directly anymore.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 include/net/mac80211.h    | 53 ++++++++++++++++++++++++++----
 net/mac80211/chan.c       | 78 +++++++++++++++++++++++++++++++++++++------
 net/mac80211/driver-ops.h | 40 ++++++++++++++++++++++
 net/mac80211/trace.h      | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 239 insertions(+), 16 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2c78997..38c10ea 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -189,6 +189,43 @@ struct ieee80211_chanctx_conf {
 };
 
 /**
+ * enum ieee80211_chanctx_switch_mode - channel context switch mode
+ * @CHANCTX_SWMODE_REASSIGN_VIF: Both old and new contexts already
+ *	exist (and will continue to exist), but the virtual interface
+ *	needs to be switched from one to the other.
+ * @CHANCTX_SWMODE_SWAP_CONTEXTS: The old context exists but will stop
+ *      to exist with this call, the new context doesn't exist but
+ *      will be active after this call, the virtual interface switches
+ *      from the old to the new (note that the driver may of course
+ *      implement this as an on-the-fly chandef switch of the existing
+ *      hardware context, but the mac80211 pointer for the old context
+ *      will cease to exist and only the new one will later be used
+ *      for changes/removal.)
+ */
+enum ieee80211_chanctx_switch_mode {
+	CHANCTX_SWMODE_REASSIGN_VIF,
+	CHANCTX_SWMODE_SWAP_CONTEXTS,
+};
+
+/**
+ * struct ieee80211_vif_chanctx_switch - vif chanctx switch information
+ *
+ * This is structure is used to pass information about a vif that
+ * needs to switch from one chanctx to another.  The
+ * &ieee80211_chanctx_switch_mode defines how the switch should be
+ * done.
+ *
+ * @vif: the vif that should be switched from old_ctx to new_ctx
+ * @old_ctx: the old context to which the vif was assigned
+ * @new_ctx: the new context to which the vif must be assigned
+ */
+struct ieee80211_vif_chanctx_switch {
+	struct ieee80211_vif *vif;
+	struct ieee80211_chanctx_conf *old_ctx;
+	struct ieee80211_chanctx_conf *new_ctx;
+};
+
+/**
  * enum ieee80211_bss_change - BSS change notification flags
  *
  * These flags are used with the bss_info_changed() callback
@@ -1564,11 +1601,6 @@ struct ieee80211_tx_control {
  *	is not enabled the default action is to disconnect when getting the
  *	CSA frame.
  *
- * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a
- *	channel context on-the-fly.  This is needed for channel switch
- *	on single-channel hardware.  It can also be used as an
- *	optimization in certain channel switch cases with
- *	multi-channel.
  */
 enum ieee80211_hw_flags {
 	IEEE80211_HW_HAS_RATE_CONTROL			= 1<<0,
@@ -1600,7 +1632,6 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_TIMING_BEACON_ONLY			= 1<<26,
 	IEEE80211_HW_SUPPORTS_HT_CCK_RATES		= 1<<27,
 	IEEE80211_HW_CHANCTX_STA_CSA			= 1<<28,
-	IEEE80211_HW_CHANGE_RUNNING_CHANCTX		= 1<<29,
 };
 
 /**
@@ -2736,6 +2767,12 @@ enum ieee80211_roc_type {
  *	to vif. Possible use is for hw queue remapping.
  * @unassign_vif_chanctx: Notifies device driver about channel context being
  *	unbound from vif.
+
+ * @switch_vif_chanctx: switch a number of vifs from one chanctx to
+ *	another, as specified in the list of
+ *	@ieee80211_vif_chanctx_switch passed to the driver, according
+ *	to the mode defined in &ieee80211_chanctx_switch_mode.
+ *
  * @start_ap: Start operation on the AP interface, this is called after all the
  *	information in bss_conf is set and beacon can be retrieved. A channel
  *	context is bound before this is called. Note that if the driver uses
@@ -2952,6 +2989,10 @@ struct ieee80211_ops {
 	void (*unassign_vif_chanctx)(struct ieee80211_hw *hw,
 				     struct ieee80211_vif *vif,
 				     struct ieee80211_chanctx_conf *ctx);
+	int (*switch_vif_chanctx)(struct ieee80211_hw *hw,
+				  struct ieee80211_vif_chanctx_switch *vifs,
+				  int n_vifs,
+				  enum ieee80211_chanctx_switch_mode mode);
 
 	void (*restart_complete)(struct ieee80211_hw *hw);
 
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 3702d64..20defee 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -928,10 +928,10 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 	new_ctx = ieee80211_find_reservation_chanctx(local, chandef, mode);
 	if (!new_ctx) {
 		if (ieee80211_chanctx_refcount(local, curr_ctx) == 1 &&
-		    (local->hw.flags & IEEE80211_HW_CHANGE_RUNNING_CHANCTX)) {
+		    local->ops->switch_vif_chanctx) {
 			/* if we're the only users of the chanctx and
-			 * the driver supports changing a running
-			 * context, reserve our current context
+			 * the driver supports chanctx switches
+			 * reserve our current context.
 			 */
 			new_ctx = curr_ctx;
 		} else if (ieee80211_can_create_new_chanctx(local)) {
@@ -956,6 +956,65 @@ out:
 	return ret;
 }
 
+static int
+ieee80211_vif_change_reserved_context(struct ieee80211_sub_if_data *sdata,
+				      struct ieee80211_chanctx *old_ctx)
+{
+	struct ieee80211_local *local = sdata->local;
+	const struct cfg80211_chan_def *chandef = &sdata->reserved_chandef;
+	struct ieee80211_chanctx *new_ctx;
+	struct ieee80211_vif_chanctx_switch vifs[1];
+	int err, changed;
+
+	lockdep_assert_held(&local->mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	new_ctx = ieee80211_alloc_chanctx(local, chandef, old_ctx->mode);
+	if (!new_ctx) {
+		err = -ENOMEM;
+		goto err;
+	}
+
+	vifs[0].vif = &sdata->vif;
+	vifs[0].old_ctx = &old_ctx->conf;
+	vifs[0].new_ctx = &new_ctx->conf;
+
+	/* turn idle off *before* setting channel -- some drivers need that */
+	changed = ieee80211_idle_off(local);
+	ieee80211_hw_config(local, changed);
+
+	err = drv_switch_vif_chanctx(local, vifs, 1,
+				     CHANCTX_SWMODE_SWAP_CONTEXTS);
+	if (err)
+		goto err_idle;
+
+	rcu_assign_pointer(sdata->vif.chanctx_conf, &new_ctx->conf);
+
+	list_del_rcu(&old_ctx->list);
+
+	if (sdata->vif.type == NL80211_IFTYPE_AP)
+		__ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
+
+	list_add_rcu(&new_ctx->list, &local->chanctx_list);
+	kfree_rcu(old_ctx, rcu_head);
+
+	ieee80211_recalc_txpower(sdata);
+	ieee80211_recalc_chanctx_chantype(local, new_ctx);
+	ieee80211_recalc_smps_chanctx(local, new_ctx);
+	ieee80211_recalc_radar_chanctx(local, new_ctx);
+	ieee80211_recalc_chanctx_min_def(local, new_ctx);
+	ieee80211_recalc_idle(local);
+
+	return 0;
+
+err_idle:
+	ieee80211_recalc_idle(local);
+	kfree_rcu(new_ctx, rcu_head);
+err:
+	return err;
+
+}
+
 int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 				       u32 *changed)
 {
@@ -999,8 +1058,7 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 
 	if (old_ctx == ctx) {
 		/* This is our own context, just change it */
-		ret = __ieee80211_vif_change_channel(sdata, old_ctx,
-						     &tmp_changed);
+		ret = ieee80211_vif_change_reserved_context(sdata, old_ctx);
 		if (ret)
 			goto out;
 	} else {
@@ -1016,14 +1074,14 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 
 		if (sdata->vif.type == NL80211_IFTYPE_AP)
 			__ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
+
+		ieee80211_recalc_chanctx_chantype(local, ctx);
+		ieee80211_recalc_smps_chanctx(local, ctx);
+		ieee80211_recalc_radar_chanctx(local, ctx);
+		ieee80211_recalc_chanctx_min_def(local, ctx);
 	}
 
 	*changed = tmp_changed;
-
-	ieee80211_recalc_chanctx_chantype(local, ctx);
-	ieee80211_recalc_smps_chanctx(local, ctx);
-	ieee80211_recalc_radar_chanctx(local, ctx);
-	ieee80211_recalc_chanctx_min_def(local, ctx);
 out:
 	mutex_unlock(&local->chanctx_mtx);
 	return ret;
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 696ef78..58feb29 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1048,6 +1048,46 @@ static inline void drv_unassign_vif_chanctx(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
+static inline int
+drv_switch_vif_chanctx(struct ieee80211_local *local,
+		       struct ieee80211_vif_chanctx_switch *vifs,
+		       int n_vifs,
+		       enum ieee80211_chanctx_switch_mode mode)
+{
+	int ret = 0;
+	int i;
+
+	if (!local->ops->switch_vif_chanctx)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < n_vifs; i++) {
+		struct ieee80211_chanctx *old_ctx =
+			container_of(vifs[i].old_ctx,
+				     struct ieee80211_chanctx,
+				     conf);
+
+		WARN_ON_ONCE(!old_ctx->driver_present);
+	}
+
+	trace_drv_switch_vif_chanctx(local, vifs, n_vifs, mode);
+	ret = local->ops->switch_vif_chanctx(&local->hw,
+					     vifs, n_vifs, mode);
+	trace_drv_return_int(local, ret);
+
+	if (!ret && mode == CHANCTX_SWMODE_SWAP_CONTEXTS) {
+		for (i = 0; i < n_vifs; i++) {
+			struct ieee80211_chanctx *new_ctx =
+				container_of(vifs[i].new_ctx,
+					     struct ieee80211_chanctx,
+					     conf);
+
+			new_ctx->driver_present = true;
+		}
+	}
+
+	return ret;
+}
+
 static inline int drv_start_ap(struct ieee80211_local *local,
 			       struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 942f64b..272f51f 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1389,6 +1389,90 @@ TRACE_EVENT(drv_change_chanctx,
 	)
 );
 
+#if !defined(__TRACE_VIF_ENTRY)
+#define __TRACE_VIF_ENTRY
+struct trace_vif_entry {
+	enum nl80211_iftype vif_type;
+	bool p2p;
+	char vif_name[8];
+};
+
+struct trace_chandef_entry {
+	u32 control_freq;
+	u32 chan_width;
+	u32 center_freq1;
+	u32 center_freq2;
+};
+
+struct trace_switch_entry {
+	struct trace_vif_entry vif;
+	struct trace_chandef_entry old_chandef;
+	struct trace_chandef_entry new_chandef;
+};
+
+#define SWITCH_ENTRY_ASSIGN(to, from) local_vifs[i].to = vifs[i].from
+#endif
+
+TRACE_EVENT(drv_switch_vif_chanctx,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_vif_chanctx_switch *vifs,
+		 int n_vifs, enum ieee80211_chanctx_switch_mode mode),
+	    TP_ARGS(local, vifs, n_vifs, mode),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		__field(int, n_vifs)
+		__field(u32, mode)
+		__dynamic_array(u8, vifs,
+				sizeof(struct trace_switch_entry) * n_vifs)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		__entry->n_vifs = n_vifs;
+		__entry->mode = mode;
+		{
+			struct trace_switch_entry *local_vifs =
+				__get_dynamic_array(vifs);
+			int i;
+
+			for (i = 0; i < n_vifs; i++) {
+				struct ieee80211_sub_if_data *sdata;
+
+				sdata = container_of(vifs[i].vif,
+						struct ieee80211_sub_if_data,
+						vif);
+
+				SWITCH_ENTRY_ASSIGN(vif.vif_type, vif->type);
+				SWITCH_ENTRY_ASSIGN(vif.p2p, vif->p2p);
+				strncpy(local_vifs[i].vif.vif_name,
+					sdata->name, 8);
+				SWITCH_ENTRY_ASSIGN(old_chandef.control_freq,
+						old_ctx->def.chan->center_freq);
+				SWITCH_ENTRY_ASSIGN(old_chandef.chan_width,
+						    old_ctx->def.width);
+				SWITCH_ENTRY_ASSIGN(old_chandef.center_freq1,
+						    old_ctx->def.center_freq1);
+				SWITCH_ENTRY_ASSIGN(old_chandef.center_freq2,
+						    old_ctx->def.center_freq2);
+				SWITCH_ENTRY_ASSIGN(new_chandef.control_freq,
+						new_ctx->def.chan->center_freq);
+				SWITCH_ENTRY_ASSIGN(new_chandef.chan_width,
+						    new_ctx->def.width);
+				SWITCH_ENTRY_ASSIGN(new_chandef.center_freq1,
+						    new_ctx->def.center_freq1);
+				SWITCH_ENTRY_ASSIGN(new_chandef.center_freq2,
+						    new_ctx->def.center_freq2);
+			}
+		}
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT " n_vifs:%d mode:%d",
+		LOCAL_PR_ARG, __entry->n_vifs, __entry->mode
+	)
+);
+
 DECLARE_EVENT_CLASS(local_sdata_chanctx,
 	TP_PROTO(struct ieee80211_local *local,
 		 struct ieee80211_sub_if_data *sdata,
-- 
2.0.0.rc0

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Luca Coelho]

On Fri, 2014-05-23 at 05:38 +0300, Luca Coelho wrote:
> From: Luciano Coelho <luciano.coelho@intel.com>
> 
> In some cases, when the driver is already using all the channel
> contexts it can handle at once, we have to do an in-place switch
> (ie. we cannot afford using an extra context temporarily for the
> transaction).  But some drivers may not support switching the channel
> context assigned to a vif on the fly (ie. without unassigning and
> assigning it) while others may only work if the context is changed on
> the fly, without unassigning it first.
> 
> To allow these different scenarios, add a new driver operation that
> let's the driver decide how to handle an in-place switch.
> 
> Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
> since we never change a running context directly anymore.
> 
> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
> ---

So, this is what I had cooked up.  Sorry Michal, I should have synced
with you.  Actually I tried to ping you on IRC, but you were not there,
and then I forgot to sync by email.

In any case, our patches were very similar, since the whole API
discussion had already been done before.  Some details are different
though, and my version only supports a single vif (obviously).  Also, I
already added a usage of the new op in a new
ieee80211_vif_change_reserved_context() function (which is similar to
Michal's "change incopat" function).

--
Luca.

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Michal Kazior]

On 23 May 2014 04:49, Luca Coelho <luca@coelho.fi> wrote:
> On Fri, 2014-05-23 at 05:38 +0300, Luca Coelho wrote:
>> From: Luciano Coelho <luciano.coelho@intel.com>
>>
>> In some cases, when the driver is already using all the channel
>> contexts it can handle at once, we have to do an in-place switch
>> (ie. we cannot afford using an extra context temporarily for the
>> transaction).  But some drivers may not support switching the channel
>> context assigned to a vif on the fly (ie. without unassigning and
>> assigning it) while others may only work if the context is changed on
>> the fly, without unassigning it first.
>>
>> To allow these different scenarios, add a new driver operation that
>> let's the driver decide how to handle an in-place switch.
>>
>> Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
>> since we never change a running context directly anymore.
>>
>> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
>> ---
>
> So, this is what I had cooked up.  Sorry Michal, I should have synced
> with you.  Actually I tried to ping you on IRC, but you were not there,
> and then I forgot to sync by email.

Thanks for explaining. That's not much of a problem.


Michał

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Michal Kazior]

On 23 May 2014 04:38, Luca Coelho <luca@coelho.fi> wrote:
> From: Luciano Coelho <luciano.coelho@intel.com>
>
> In some cases, when the driver is already using all the channel
> contexts it can handle at once, we have to do an in-place switch
> (ie. we cannot afford using an extra context temporarily for the
> transaction).  But some drivers may not support switching the channel
> context assigned to a vif on the fly (ie. without unassigning and
> assigning it) while others may only work if the context is changed on
> the fly, without unassigning it first.
>
> To allow these different scenarios, add a new driver operation that
> let's the driver decide how to handle an in-place switch.
>
> Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
> since we never change a running context directly anymore.
>
> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
> ---

[...]

> +static int
> +ieee80211_vif_change_reserved_context(struct ieee80211_sub_if_data *sdata,
> +                                     struct ieee80211_chanctx *old_ctx)
> +{
> +       struct ieee80211_local *local = sdata->local;
> +       const struct cfg80211_chan_def *chandef = &sdata->reserved_chandef;
> +       struct ieee80211_chanctx *new_ctx;
> +       struct ieee80211_vif_chanctx_switch vifs[1];
> +       int err, changed;
> +
> +       lockdep_assert_held(&local->mtx);
> +       lockdep_assert_held(&local->chanctx_mtx);
> +
> +       new_ctx = ieee80211_alloc_chanctx(local, chandef, old_ctx->mode);
> +       if (!new_ctx) {
> +               err = -ENOMEM;
> +               goto err;
> +       }
> +
> +       vifs[0].vif = &sdata->vif;
> +       vifs[0].old_ctx = &old_ctx->conf;
> +       vifs[0].new_ctx = &new_ctx->conf;
> +
> +       /* turn idle off *before* setting channel -- some drivers need that */
> +       changed = ieee80211_idle_off(local);

Oh. My code doesn't have that.


> +       ieee80211_hw_config(local, changed);
> +
> +       err = drv_switch_vif_chanctx(local, vifs, 1,
> +                                    CHANCTX_SWMODE_SWAP_CONTEXTS);
> +       if (err)
> +               goto err_idle;
> +
> +       rcu_assign_pointer(sdata->vif.chanctx_conf, &new_ctx->conf);
> +
> +       list_del_rcu(&old_ctx->list);
> +
> +       if (sdata->vif.type == NL80211_IFTYPE_AP)
> +               __ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
> +
> +       list_add_rcu(&new_ctx->list, &local->chanctx_list);
> +       kfree_rcu(old_ctx, rcu_head);
> +
> +       ieee80211_recalc_txpower(sdata);
> +       ieee80211_recalc_chanctx_chantype(local, new_ctx);
> +       ieee80211_recalc_smps_chanctx(local, new_ctx);
> +       ieee80211_recalc_radar_chanctx(local, new_ctx);
> +       ieee80211_recalc_chanctx_min_def(local, new_ctx);
> +       ieee80211_recalc_idle(local);
> +
> +       return 0;
> +
> +err_idle:
> +       ieee80211_recalc_idle(local);
> +       kfree_rcu(new_ctx, rcu_head);
> +err:
> +       return err;
> +

Spurious empty line?

> +}
> +

[...]

> +static inline int
> +drv_switch_vif_chanctx(struct ieee80211_local *local,
> +                      struct ieee80211_vif_chanctx_switch *vifs,
> +                      int n_vifs,
> +                      enum ieee80211_chanctx_switch_mode mode)
> +{
> +       int ret = 0;
> +       int i;
> +
> +       if (!local->ops->switch_vif_chanctx)
> +               return -EOPNOTSUPP;
> +
> +       for (i = 0; i < n_vifs; i++) {
> +               struct ieee80211_chanctx *old_ctx =
> +                       container_of(vifs[i].old_ctx,
> +                                    struct ieee80211_chanctx,
> +                                    conf);
> +
> +               WARN_ON_ONCE(!old_ctx->driver_present);

I guess it's good to do this too:

 WARN_ON_ONCE(mode == SWAP && new_ctx->driver_present);

If you swap then new_ctx must not be in driver yet. It wouldn't make
much sense, would it?


> +       }
> +
> +       trace_drv_switch_vif_chanctx(local, vifs, n_vifs, mode);
> +       ret = local->ops->switch_vif_chanctx(&local->hw,
> +                                            vifs, n_vifs, mode);
> +       trace_drv_return_int(local, ret);
> +
> +       if (!ret && mode == CHANCTX_SWMODE_SWAP_CONTEXTS) {
> +               for (i = 0; i < n_vifs; i++) {
> +                       struct ieee80211_chanctx *new_ctx =
> +                               container_of(vifs[i].new_ctx,
> +                                            struct ieee80211_chanctx,
> +                                            conf);
> +
> +                       new_ctx->driver_present = true;

Oh! I totally forgot about driver_present in my patch :-)

But while at it I'd also do `old_ctx->driver_present = false` even if
it isn't strictly necessary. It just makes it more obvious what
switch_vif_chanctx expects driver to think of new_ctx/old_ctx.


So.. which patch are we going forward with? Luca's or mine? Either way
is fine with me as long as we reach a conclusion :-)


Michał

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Johannes Berg]

On Fri, 2014-05-23 at 10:01 +0200, Michal Kazior wrote:

> So.. which patch are we going forward with? Luca's or mine? Either way
> is fine with me as long as we reach a conclusion :-)

Looks like both are missing some things? Luca's doesn't have tracing,
and supports just a single interface (which you'll not like :) ) but
personally I liked the struct API a bit better than multiple double
pointer arrays you had.

johannes

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Luca Coelho]

On Fri, 2014-05-23 at 10:58 +0200, Johannes Berg wrote:
> On Fri, 2014-05-23 at 10:01 +0200, Michal Kazior wrote:
> 
> > So.. which patch are we going forward with? Luca's or mine? Either way
> > is fine with me as long as we reach a conclusion :-)
> 
> Looks like both are missing some things? Luca's doesn't have tracing,
> and supports just a single interface (which you'll not like :) ) but
> personally I liked the struct API a bit better than multiple double
> pointer arrays you had.

What do you mean mine doesn't support tracing? Check the bottom of the
patch again. :) I don't think it's the most beautiful code, but it seems
to work. :)

Also, my patch *does* support multiple vifs.  But the code we have
doesn't support it, so I call the new op with a single-vif only.
Michal's patch doesn't call the new op (only on a later patch), so it's
pretty much the same.

--
Luca.

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Johannes Berg]

On Fri, 2014-05-23 at 12:14 +0300, Luca Coelho wrote:

> What do you mean mine doesn't support tracing? Check the bottom of the
> patch again. :) I don't think it's the most beautiful code, but it seems
> to work. :)

Oh, I was going by the old version I guess! :)

That tracing code looks OK to me, though I think that the vif name field
is too short?

> Also, my patch *does* support multiple vifs.  But the code we have
> doesn't support it, so I call the new op with a single-vif only.
> Michal's patch doesn't call the new op (only on a later patch), so it's
> pretty much the same.

Right, ok. So it's a wash. Flip a coin? :)

johannes

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Johannes Berg]

On Fri, 2014-05-23 at 11:30 +0200, Johannes Berg wrote:

> > Also, my patch *does* support multiple vifs.  But the code we have
> > doesn't support it, so I call the new op with a single-vif only.
> > Michal's patch doesn't call the new op (only on a later patch), so it's
> > pretty much the same.
> 
> Right, ok. So it's a wash. Flip a coin? :)

Looks like Luca's patch needs less work, so let's go with that?

johannes

[PATCH 1/2] mac80211: add a single-transaction driver op to switch contexts

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

In some cases, when the driver is already using all the channel
contexts it can handle at once, we have to do an in-place switch
(ie. we cannot afford using an extra context temporarily for the
transaction).  But some drivers may not support switching the channel
context assigned to a vif on the fly (ie. without unassigning and
assigning it) while others may only work if the context is changed on
the fly, without unassigning it first.

To allow these different scenarios, add a new driver operation that
let's the driver decide how to handle an in-place switch.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In v2:
   * moved the change_reserved_context part to a separate patch [Michal];
   * removed spurious line [Michal];
   * added WARN if new_ctx->driver_present is already set [Michal];
   * explicitly set old_ctx->driver_present to false [Michal];
---
 include/net/mac80211.h    | 47 ++++++++++++++++++++++++++
 net/mac80211/driver-ops.h | 51 ++++++++++++++++++++++++++++
 net/mac80211/trace.h      | 84 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 2c78997..236c5c3 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -189,6 +189,43 @@ struct ieee80211_chanctx_conf {
 };
 
 /**
+ * enum ieee80211_chanctx_switch_mode - channel context switch mode
+ * @CHANCTX_SWMODE_REASSIGN_VIF: Both old and new contexts already
+ *	exist (and will continue to exist), but the virtual interface
+ *	needs to be switched from one to the other.
+ * @CHANCTX_SWMODE_SWAP_CONTEXTS: The old context exists but will stop
+ *      to exist with this call, the new context doesn't exist but
+ *      will be active after this call, the virtual interface switches
+ *      from the old to the new (note that the driver may of course
+ *      implement this as an on-the-fly chandef switch of the existing
+ *      hardware context, but the mac80211 pointer for the old context
+ *      will cease to exist and only the new one will later be used
+ *      for changes/removal.)
+ */
+enum ieee80211_chanctx_switch_mode {
+	CHANCTX_SWMODE_REASSIGN_VIF,
+	CHANCTX_SWMODE_SWAP_CONTEXTS,
+};
+
+/**
+ * struct ieee80211_vif_chanctx_switch - vif chanctx switch information
+ *
+ * This is structure is used to pass information about a vif that
+ * needs to switch from one chanctx to another.  The
+ * &ieee80211_chanctx_switch_mode defines how the switch should be
+ * done.
+ *
+ * @vif: the vif that should be switched from old_ctx to new_ctx
+ * @old_ctx: the old context to which the vif was assigned
+ * @new_ctx: the new context to which the vif must be assigned
+ */
+struct ieee80211_vif_chanctx_switch {
+	struct ieee80211_vif *vif;
+	struct ieee80211_chanctx_conf *old_ctx;
+	struct ieee80211_chanctx_conf *new_ctx;
+};
+
+/**
  * enum ieee80211_bss_change - BSS change notification flags
  *
  * These flags are used with the bss_info_changed() callback
@@ -2736,6 +2773,12 @@ enum ieee80211_roc_type {
  *	to vif. Possible use is for hw queue remapping.
  * @unassign_vif_chanctx: Notifies device driver about channel context being
  *	unbound from vif.
+
+ * @switch_vif_chanctx: switch a number of vifs from one chanctx to
+ *	another, as specified in the list of
+ *	@ieee80211_vif_chanctx_switch passed to the driver, according
+ *	to the mode defined in &ieee80211_chanctx_switch_mode.
+ *
  * @start_ap: Start operation on the AP interface, this is called after all the
  *	information in bss_conf is set and beacon can be retrieved. A channel
  *	context is bound before this is called. Note that if the driver uses
@@ -2952,6 +2995,10 @@ struct ieee80211_ops {
 	void (*unassign_vif_chanctx)(struct ieee80211_hw *hw,
 				     struct ieee80211_vif *vif,
 				     struct ieee80211_chanctx_conf *ctx);
+	int (*switch_vif_chanctx)(struct ieee80211_hw *hw,
+				  struct ieee80211_vif_chanctx_switch *vifs,
+				  int n_vifs,
+				  enum ieee80211_chanctx_switch_mode mode);
 
 	void (*restart_complete)(struct ieee80211_hw *hw);
 
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 696ef78..c5a6bd1 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1048,6 +1048,57 @@ static inline void drv_unassign_vif_chanctx(struct ieee80211_local *local,
 	trace_drv_return_void(local);
 }
 
+static inline int
+drv_switch_vif_chanctx(struct ieee80211_local *local,
+		       struct ieee80211_vif_chanctx_switch *vifs,
+		       int n_vifs,
+		       enum ieee80211_chanctx_switch_mode mode)
+{
+	int ret = 0;
+	int i;
+
+	if (!local->ops->switch_vif_chanctx)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < n_vifs; i++) {
+		struct ieee80211_chanctx *new_ctx =
+			container_of(vifs[i].new_ctx,
+				     struct ieee80211_chanctx,
+				     conf);
+		struct ieee80211_chanctx *old_ctx =
+			container_of(vifs[i].old_ctx,
+				     struct ieee80211_chanctx,
+				     conf);
+
+		WARN_ON_ONCE(!old_ctx->driver_present);
+		WARN_ON_ONCE(mode == CHANCTX_SWMODE_SWAP_CONTEXTS &&
+			     new_ctx->driver_present);
+	}
+
+	trace_drv_switch_vif_chanctx(local, vifs, n_vifs, mode);
+	ret = local->ops->switch_vif_chanctx(&local->hw,
+					     vifs, n_vifs, mode);
+	trace_drv_return_int(local, ret);
+
+	if (!ret && mode == CHANCTX_SWMODE_SWAP_CONTEXTS) {
+		for (i = 0; i < n_vifs; i++) {
+			struct ieee80211_chanctx *new_ctx =
+				container_of(vifs[i].new_ctx,
+					     struct ieee80211_chanctx,
+					     conf);
+			struct ieee80211_chanctx *old_ctx =
+				container_of(vifs[i].old_ctx,
+					     struct ieee80211_chanctx,
+					     conf);
+
+			new_ctx->driver_present = true;
+			old_ctx->driver_present = false;
+		}
+	}
+
+	return ret;
+}
+
 static inline int drv_start_ap(struct ieee80211_local *local,
 			       struct ieee80211_sub_if_data *sdata)
 {
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 942f64b..272f51f 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -1389,6 +1389,90 @@ TRACE_EVENT(drv_change_chanctx,
 	)
 );
 
+#if !defined(__TRACE_VIF_ENTRY)
+#define __TRACE_VIF_ENTRY
+struct trace_vif_entry {
+	enum nl80211_iftype vif_type;
+	bool p2p;
+	char vif_name[8];
+};
+
+struct trace_chandef_entry {
+	u32 control_freq;
+	u32 chan_width;
+	u32 center_freq1;
+	u32 center_freq2;
+};
+
+struct trace_switch_entry {
+	struct trace_vif_entry vif;
+	struct trace_chandef_entry old_chandef;
+	struct trace_chandef_entry new_chandef;
+};
+
+#define SWITCH_ENTRY_ASSIGN(to, from) local_vifs[i].to = vifs[i].from
+#endif
+
+TRACE_EVENT(drv_switch_vif_chanctx,
+	TP_PROTO(struct ieee80211_local *local,
+		 struct ieee80211_vif_chanctx_switch *vifs,
+		 int n_vifs, enum ieee80211_chanctx_switch_mode mode),
+	    TP_ARGS(local, vifs, n_vifs, mode),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		__field(int, n_vifs)
+		__field(u32, mode)
+		__dynamic_array(u8, vifs,
+				sizeof(struct trace_switch_entry) * n_vifs)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		__entry->n_vifs = n_vifs;
+		__entry->mode = mode;
+		{
+			struct trace_switch_entry *local_vifs =
+				__get_dynamic_array(vifs);
+			int i;
+
+			for (i = 0; i < n_vifs; i++) {
+				struct ieee80211_sub_if_data *sdata;
+
+				sdata = container_of(vifs[i].vif,
+						struct ieee80211_sub_if_data,
+						vif);
+
+				SWITCH_ENTRY_ASSIGN(vif.vif_type, vif->type);
+				SWITCH_ENTRY_ASSIGN(vif.p2p, vif->p2p);
+				strncpy(local_vifs[i].vif.vif_name,
+					sdata->name, 8);
+				SWITCH_ENTRY_ASSIGN(old_chandef.control_freq,
+						old_ctx->def.chan->center_freq);
+				SWITCH_ENTRY_ASSIGN(old_chandef.chan_width,
+						    old_ctx->def.width);
+				SWITCH_ENTRY_ASSIGN(old_chandef.center_freq1,
+						    old_ctx->def.center_freq1);
+				SWITCH_ENTRY_ASSIGN(old_chandef.center_freq2,
+						    old_ctx->def.center_freq2);
+				SWITCH_ENTRY_ASSIGN(new_chandef.control_freq,
+						new_ctx->def.chan->center_freq);
+				SWITCH_ENTRY_ASSIGN(new_chandef.chan_width,
+						    new_ctx->def.width);
+				SWITCH_ENTRY_ASSIGN(new_chandef.center_freq1,
+						    new_ctx->def.center_freq1);
+				SWITCH_ENTRY_ASSIGN(new_chandef.center_freq2,
+						    new_ctx->def.center_freq2);
+			}
+		}
+	),
+
+	TP_printk(
+		LOCAL_PR_FMT " n_vifs:%d mode:%d",
+		LOCAL_PR_ARG, __entry->n_vifs, __entry->mode
+	)
+);
+
 DECLARE_EVENT_CLASS(local_sdata_chanctx,
 	TP_PROTO(struct ieee80211_local *local,
 		 struct ieee80211_sub_if_data *sdata,
-- 
2.0.0.rc0

[PATCH 2/2] mac80211: use switch_vif_chanctx to change a running chanctx

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

Instead of hammering a new channel into the running context, make use
of the new switch_vif_chanctx to let the driver decide how to perform
the switch.

Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
since we never change a running context directly anymore.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 include/net/mac80211.h |  6 ----
 net/mac80211/chan.c    | 77 +++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 236c5c3..38c10ea 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1601,11 +1601,6 @@ struct ieee80211_tx_control {
  *	is not enabled the default action is to disconnect when getting the
  *	CSA frame.
  *
- * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a
- *	channel context on-the-fly.  This is needed for channel switch
- *	on single-channel hardware.  It can also be used as an
- *	optimization in certain channel switch cases with
- *	multi-channel.
  */
 enum ieee80211_hw_flags {
 	IEEE80211_HW_HAS_RATE_CONTROL			= 1<<0,
@@ -1637,7 +1632,6 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_TIMING_BEACON_ONLY			= 1<<26,
 	IEEE80211_HW_SUPPORTS_HT_CCK_RATES		= 1<<27,
 	IEEE80211_HW_CHANCTX_STA_CSA			= 1<<28,
-	IEEE80211_HW_CHANGE_RUNNING_CHANCTX		= 1<<29,
 };
 
 /**
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 3702d64..7084c2b 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -928,10 +928,10 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 	new_ctx = ieee80211_find_reservation_chanctx(local, chandef, mode);
 	if (!new_ctx) {
 		if (ieee80211_chanctx_refcount(local, curr_ctx) == 1 &&
-		    (local->hw.flags & IEEE80211_HW_CHANGE_RUNNING_CHANCTX)) {
+		    local->ops->switch_vif_chanctx) {
 			/* if we're the only users of the chanctx and
-			 * the driver supports changing a running
-			 * context, reserve our current context
+			 * the driver supports chanctx switches
+			 * reserve our current context.
 			 */
 			new_ctx = curr_ctx;
 		} else if (ieee80211_can_create_new_chanctx(local)) {
@@ -956,6 +956,64 @@ out:
 	return ret;
 }
 
+static int
+ieee80211_vif_change_reserved_context(struct ieee80211_sub_if_data *sdata,
+				      struct ieee80211_chanctx *old_ctx)
+{
+	struct ieee80211_local *local = sdata->local;
+	const struct cfg80211_chan_def *chandef = &sdata->reserved_chandef;
+	struct ieee80211_chanctx *new_ctx;
+	struct ieee80211_vif_chanctx_switch vifs[1];
+	int err, changed;
+
+	lockdep_assert_held(&local->mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	new_ctx = ieee80211_alloc_chanctx(local, chandef, old_ctx->mode);
+	if (!new_ctx) {
+		err = -ENOMEM;
+		goto err;
+	}
+
+	vifs[0].vif = &sdata->vif;
+	vifs[0].old_ctx = &old_ctx->conf;
+	vifs[0].new_ctx = &new_ctx->conf;
+
+	/* turn idle off *before* setting channel -- some drivers need that */
+	changed = ieee80211_idle_off(local);
+	ieee80211_hw_config(local, changed);
+
+	err = drv_switch_vif_chanctx(local, vifs, 1,
+				     CHANCTX_SWMODE_SWAP_CONTEXTS);
+	if (err)
+		goto err_idle;
+
+	rcu_assign_pointer(sdata->vif.chanctx_conf, &new_ctx->conf);
+
+	list_del_rcu(&old_ctx->list);
+
+	if (sdata->vif.type == NL80211_IFTYPE_AP)
+		__ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
+
+	list_add_rcu(&new_ctx->list, &local->chanctx_list);
+	kfree_rcu(old_ctx, rcu_head);
+
+	ieee80211_recalc_txpower(sdata);
+	ieee80211_recalc_chanctx_chantype(local, new_ctx);
+	ieee80211_recalc_smps_chanctx(local, new_ctx);
+	ieee80211_recalc_radar_chanctx(local, new_ctx);
+	ieee80211_recalc_chanctx_min_def(local, new_ctx);
+	ieee80211_recalc_idle(local);
+
+	return 0;
+
+err_idle:
+	ieee80211_recalc_idle(local);
+	kfree_rcu(new_ctx, rcu_head);
+err:
+	return err;
+}
+
 int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 				       u32 *changed)
 {
@@ -999,8 +1057,7 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 
 	if (old_ctx == ctx) {
 		/* This is our own context, just change it */
-		ret = __ieee80211_vif_change_channel(sdata, old_ctx,
-						     &tmp_changed);
+		ret = ieee80211_vif_change_reserved_context(sdata, old_ctx);
 		if (ret)
 			goto out;
 	} else {
@@ -1016,14 +1073,14 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 
 		if (sdata->vif.type == NL80211_IFTYPE_AP)
 			__ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
+
+		ieee80211_recalc_chanctx_chantype(local, ctx);
+		ieee80211_recalc_smps_chanctx(local, ctx);
+		ieee80211_recalc_radar_chanctx(local, ctx);
+		ieee80211_recalc_chanctx_min_def(local, ctx);
 	}
 
 	*changed = tmp_changed;
-
-	ieee80211_recalc_chanctx_chantype(local, ctx);
-	ieee80211_recalc_smps_chanctx(local, ctx);
-	ieee80211_recalc_radar_chanctx(local, ctx);
-	ieee80211_recalc_chanctx_min_def(local, ctx);
 out:
 	mutex_unlock(&local->chanctx_mtx);
 	return ret;
-- 
2.0.0.rc0

Re: [PATCH 2/2] mac80211: use switch_vif_chanctx to change a running chanctx

[Luca Coelho]

On Fri, 2014-05-23 at 14:33 +0300, Luca Coelho wrote:
> From: Luciano Coelho <luciano.coelho@intel.com>
> 
> Instead of hammering a new channel into the running context, make use
> of the new switch_vif_chanctx to let the driver decide how to perform
> the switch.
> 
> Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
> since we never change a running context directly anymore.
> 
> Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
> ---

This is optional, you don't have to apply it if you're going to take
Michal's patches (since they will undo this change anyway), as we
discussed on IRC.

So, this is mostly just for the record.

--
Luca.

Re: [PATCH 2/2] mac80211: use switch_vif_chanctx to change a running chanctx

[Johannes Berg]

On Fri, 2014-05-23 at 14:33 +0300, Luca Coelho wrote:
> From: Luciano Coelho <luciano.coelho@intel.com>
> 
> Instead of hammering a new channel into the running context, make use
> of the new switch_vif_chanctx to let the driver decide how to perform
> the switch.
> 
> Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
> since we never change a running context directly anymore.

This seems totally reasonable to me, but I'll wait to see what Michal
does :)

johannes

Re: [PATCH 2/2] mac80211: use switch_vif_chanctx to change a running chanctx

[Luca Coelho]

On Fri, 2014-05-23 at 15:28 +0200, Johannes Berg wrote:
> On Fri, 2014-05-23 at 14:33 +0300, Luca Coelho wrote:
> > From: Luciano Coelho <luciano.coelho@intel.com>
> > 
> > Instead of hammering a new channel into the running context, make use
> > of the new switch_vif_chanctx to let the driver decide how to perform
> > the switch.
> > 
> > Additionally, remove the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag,
> > since we never change a running context directly anymore.
> 
> This seems totally reasonable to me, but I'll wait to see what Michal
> does :)

Yeah, I think so too, but if he specifically add us not to do this, it's
probably because he would have trouble rebasing or something. :)

--
Luca.

Re: [PATCH 1/2] mac80211: add a single-transaction driver op to switch contexts

[Johannes Berg]

On Fri, 2014-05-23 at 14:33 +0300, Luca Coelho wrote:
> From: Luciano Coelho <luciano.coelho@intel.com>
> 
> In some cases, when the driver is already using all the channel
> contexts it can handle at once, we have to do an in-place switch
> (ie. we cannot afford using an extra context temporarily for the
> transaction).  But some drivers may not support switching the channel
> context assigned to a vif on the fly (ie. without unassigning and
> assigning it) while others may only work if the context is changed on
> the fly, without unassigning it first.
> 
> To allow these different scenarios, add a new driver operation that
> let's the driver decide how to handle an in-place switch.

Ok, I've applied this. I extended the warning for the new context (to
check that it exists in the reassign case) and changed the tracing
structs slightly (fixing the ifname length and marking the structs
packed to avoid alignment issues)

johannes

Re: [PATCH 1/2] mac80211: add a single-transaction driver op to switch contexts

[Luca Coelho]

On Fri, 2014-05-23 at 15:24 +0200, Johannes Berg wrote:
> On Fri, 2014-05-23 at 14:33 +0300, Luca Coelho wrote:
> > From: Luciano Coelho <luciano.coelho@intel.com>
> > 
> > In some cases, when the driver is already using all the channel
> > contexts it can handle at once, we have to do an in-place switch
> > (ie. we cannot afford using an extra context temporarily for the
> > transaction).  But some drivers may not support switching the channel
> > context assigned to a vif on the fly (ie. without unassigning and
> > assigning it) while others may only work if the context is changed on
> > the fly, without unassigning it first.
> > 
> > To allow these different scenarios, add a new driver operation that
> > let's the driver decide how to handle an in-place switch.
> 
> Ok, I've applied this. I extended the warning for the new context (to
> check that it exists in the reassign case) and changed the tracing
> structs slightly (fixing the ifname length and marking the structs
> packed to avoid alignment issues)

Great, thanks!

I was just adding the __packed in the struct, because I am implementing
the parser for trace-cmd and noticed we could have trouble. :)

--
Luca.

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Michal Kazior]

On 23 May 2014 10:58, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2014-05-23 at 10:01 +0200, Michal Kazior wrote:
>
>> So.. which patch are we going forward with? Luca's or mine? Either way
>> is fine with me as long as we reach a conclusion :-)
>
> Looks like both are missing some things? Luca's doesn't have tracing,
> and supports just a single interface (which you'll not like :) ) but
> personally I liked the struct API a bit better than multiple double
> pointer arrays you had.

Well, the 'single interface' problem isn't even a thing. Luca just
happens to rework ieee80211_vif_use_reserved_
context() in one go. My patch simply adds the new op since the
old/single-vif channel csa code gets thrown out later anyway.

I suppose we can just split Luca's patch and drop the
vif_use_reserved_context() part (as well as
IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag removal) as it will get
overwritten by my patches later anyway?


Michał

Re: [PATCH] mac80211: add a single-transaction driver op to switch contexts

[Johannes Berg]

On Fri, 2014-05-23 at 11:16 +0200, Michal Kazior wrote:
> On 23 May 2014 10:58, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Fri, 2014-05-23 at 10:01 +0200, Michal Kazior wrote:
> >
> >> So.. which patch are we going forward with? Luca's or mine? Either way
> >> is fine with me as long as we reach a conclusion :-)
> >
> > Looks like both are missing some things? Luca's doesn't have tracing,
> > and supports just a single interface (which you'll not like :) ) but
> > personally I liked the struct API a bit better than multiple double
> > pointer arrays you had.
> 
> Well, the 'single interface' problem isn't even a thing. Luca just
> happens to rework ieee80211_vif_use_reserved_
> context() in one go. My patch simply adds the new op since the
> old/single-vif channel csa code gets thrown out later anyway.
> 
> I suppose we can just split Luca's patch and drop the
> vif_use_reserved_context() part (as well as
> IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag removal) as it will get
> overwritten by my patches later anyway?

Oh ok. Can't say I really care :)

johannes

Re: [PATCH v6 1/6] mac80211: introduce switch_vif_chanctx op

[Michal Kazior]

On 22 May 2014 16:45, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
>> This new device driver operation will be used for
>> channel context reservation and switching.
>
> Heh. Luca just had some very similar code. But that's not necessarily a
> bad thing - we can compare :)
>
>> +     int (*switch_vif_chanctx)(struct ieee80211_hw *hw,
>> +                               struct ieee80211_vif **vifs,
>> +                               struct ieee80211_chanctx_conf **old_ctx,
>> +                               struct ieee80211_chanctx_conf **new_ctx,
>> +                               int n_vifs,
>> +                               enum ieee80211_chanctx_swmode swmode);
>
> Luca had a struct here with (vif, old, new), I think that makes sense.
>
>> +#define IEEE80211_MAX_NUM_SWITCH_VIFS 8
>
> :-)
>
> That seems artificial though - why not dynamically allocate?

I tend to avoid dynamic allocations whenever I can. I can make it
dynamic if you want.


>> +static inline int drv_switch_vif_chanctx(struct ieee80211_local *local,
>> +                                      struct ieee80211_sub_if_data **sdata,
>> +                                      struct ieee80211_chanctx **old_ctx,
>> +                                      struct ieee80211_chanctx **new_ctx,
>> +                                      int n_vifs,
>> +                                      enum ieee80211_chanctx_swmode swmode)
>> +{
>> +     struct ieee80211_vif *vifs[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
>> +     struct ieee80211_chanctx_conf *old_conf[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
>> +     struct ieee80211_chanctx_conf *new_conf[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
>> +     int i, ret = 0;
>
> That's a little big for an inline?

Maybe just a bit..


>> +     if (local->ops->switch_vif_chanctx)
>> +             return -EOPNOTSUPP;
>> +
>> +     for (i = 0; i < n_vifs; i++)
>> +             if (!check_sdata_in_driver(sdata[i]))
>> +                     return -EIO;
>> +
>> +     for (i = 0; i < n_vifs; i++) {
>> +             trace_drv_switch_vif_chanctx(local, sdata[i], old_ctx[i],
>> +                                          new_ctx[i], n_vifs, swmode, i);
>
> Hmm. This is somewhat ugly since the loop always runs. In theory it's
> possible to do this all with dynamic_array() and code in the assign path
> of the tracepoint, I think that'd be better. Or even for now just leave
> the tracing to have just a subset or something (e.g. at most 2
> interfaces)

I think I've had the same problem when I was trying to make a
single-call multi-vif csa tracing. Is using dynamic_array for this
really doable? I haven't found any code in the kernel using
__dynamic_array for anything but simple scalars and buffers.


>> +++ b/net/mac80211/main.c
>> @@ -502,6 +502,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
>>       if (WARN_ON(i != 0 && i != 5))
>>               return NULL;
>>       use_chanctx = i == 5;
>> +     if (WARN_ON(!use_chanctx && ops->switch_vif_chanctx))
>> +             return NULL;
>
> I don't think this makes sense - we perfectly handle the case right now
> by disconnecting (and not advertising switch to userspace, I guess? if
> not we should)
>
> Requiring drivers to implement this just makes things more difficult,
> and the channel switch isn't really mandatory spec-wise.

This is supposed to prevent non-chanctx drivers from accidentally
implementing switch_vif_chanctx. It doesn't require chanctx drivers to
implement switch_vif_chanctx.


>
>> +             __field(u32, old_control_freq)
>
> I believe there's a macro for a chandef?

Yes there is, but it assumes you have a `control_freq`. Since there
are old_ and new_ prefixes in my tracing I can't really use that
define, can I? I'd have to modify the define (and all callsites) to
take an argument with a prefix and concatenate symbols with ##.


Michał

Re: [PATCH v6 1/6] mac80211: introduce switch_vif_chanctx op

[Johannes Berg]

On Fri, 2014-05-23 at 08:09 +0200, Michal Kazior wrote:

> >> +#define IEEE80211_MAX_NUM_SWITCH_VIFS 8
> >
> > :-)
> >
> > That seems artificial though - why not dynamically allocate?
> 
> I tend to avoid dynamic allocations whenever I can. I can make it
> dynamic if you want.

I'm just thinking that Ben Greear will invariably complain about this
limit ;-)

> I think I've had the same problem when I was trying to make a
> single-call multi-vif csa tracing. Is using dynamic_array for this
> really doable? I haven't found any code in the kernel using
> __dynamic_array for anything but simple scalars and buffers.

You could either use multiple arrays, or I guess you could even pack a
struct into the array, no?

> >> +     if (WARN_ON(!use_chanctx && ops->switch_vif_chanctx))
> >> +             return NULL;
> >
> > I don't think this makes sense - we perfectly handle the case right now
> > by disconnecting (and not advertising switch to userspace, I guess? if
> > not we should)
> >
> > Requiring drivers to implement this just makes things more difficult,
> > and the channel switch isn't really mandatory spec-wise.
> 
> This is supposed to prevent non-chanctx drivers from accidentally
> implementing switch_vif_chanctx. It doesn't require chanctx drivers to
> implement switch_vif_chanctx.

Err, yeah, I misread the code - sorry.

> >> +             __field(u32, old_control_freq)
> >
> > I believe there's a macro for a chandef?
> 
> Yes there is, but it assumes you have a `control_freq`. Since there
> are old_ and new_ prefixes in my tracing I can't really use that
> define, can I? I'd have to modify the define (and all callsites) to
> take an argument with a prefix and concatenate symbols with ##.

Oh, you're right, somehow I thought we'd done something like that
already.

Probably not worth it then.

johannes

Re: [PATCH v6 1/6] mac80211: introduce switch_vif_chanctx op

[Michal Kazior]

On 23 May 2014 10:51, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2014-05-23 at 08:09 +0200, Michal Kazior wrote:
>
>> >> +#define IEEE80211_MAX_NUM_SWITCH_VIFS 8
>> >
>> > :-)
>> >
>> > That seems artificial though - why not dynamically allocate?
>>
>> I tend to avoid dynamic allocations whenever I can. I can make it
>> dynamic if you want.
>
> I'm just thinking that Ben Greear will invariably complain about this
> limit ;-)

I see your point now.. :D


>> I think I've had the same problem when I was trying to make a
>> single-call multi-vif csa tracing. Is using dynamic_array for this
>> really doable? I haven't found any code in the kernel using
>> __dynamic_array for anything but simple scalars and buffers.
>
> You could either use multiple arrays, or I guess you could even pack a
> struct into the array, no?

Packing is not a problem - reading/using is. How do you print an array
of structs in a trace? I think I've played around this a little but
gave up for some reason I don't remember anymore.


Michał

Re: [PATCH v6 1/6] mac80211: introduce switch_vif_chanctx op

[Johannes Berg]

On Fri, 2014-05-23 at 11:10 +0200, Michal Kazior wrote:

> > You could either use multiple arrays, or I guess you could even pack a
> > struct into the array, no?
> 
> Packing is not a problem - reading/using is. How do you print an array
> of structs in a trace? I think I've played around this a little but
> gave up for some reason I don't remember anymore.

Oh, you can't print it, you'd have to have a userspace plugin for
trace-cmd to do that.

johannes

[PATCH v6 2/6] mac80211: implement multi-vif in-place reservations

[Michal Kazior]

Multi-vif in-place reservations happen when
it's impossible to allocate more chanctx as per
driver combinations.

Such reservations aren't finalized until last
reservation interface calls in to use the
reservation.

This introduces a special hook
ieee80211_vif_chanctx_reservation_complete(). This
is currently an empty stub and will be filled in
by AP/STA CSA code. This is required to implement
2-step CSA finalization.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

Notes:
    v2:
     * use new_ctx instead of ctx [Eliad]
     * move recalcs after bss_conf is updated [Eliad]
    
    v4:
     * move recalc-radar before vif-chanctx-assign [Eliad]
     * move radar_required swapping before initial add_chanctx() [Eliad]
    
    v5:
     * move kfree_rcu() [Zhao Gang]
    
    v6:
     * use switch_vif_chanctx for incompat case with chanctx drivers [Johannes]
     * fix vlan chanctx copying

 include/net/mac80211.h     |  10 +-
 net/mac80211/chan.c        | 330 +++++++++++++++++++++++++++++++++++----------
 net/mac80211/ieee80211_i.h |   4 +-
 3 files changed, 260 insertions(+), 84 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 831607e..e3cc825 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1580,12 +1580,6 @@ struct ieee80211_tx_control {
  *	for a single active channel while using channel contexts. When support
  *	is not enabled the default action is to disconnect when getting the
  *	CSA frame.
- *
- * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a
- *	channel context on-the-fly.  This is needed for channel switch
- *	on single-channel hardware.  It can also be used as an
- *	optimization in certain channel switch cases with
- *	multi-channel.
  */
 enum ieee80211_hw_flags {
 	IEEE80211_HW_HAS_RATE_CONTROL			= 1<<0,
@@ -1617,7 +1611,6 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_TIMING_BEACON_ONLY			= 1<<26,
 	IEEE80211_HW_SUPPORTS_HT_CCK_RATES		= 1<<27,
 	IEEE80211_HW_CHANCTX_STA_CSA			= 1<<28,
-	IEEE80211_HW_CHANGE_RUNNING_CHANCTX		= 1<<29,
 };
 
 /**
@@ -2756,7 +2749,8 @@ enum ieee80211_roc_type {
  * @switch_vif_chanctx: Requests driver to perform a channel switch. It passes
  *	a list of triplets (vif, oldctx, newctx). Driver should try to restore
  *	pre-call state if it fails.
- *	The callback is optional and can sleep.
+ *	The callback is optional for channel context based drivers but is
+ *	required to support channel switching. The callback and can sleep.
  *
  * @start_ap: Start operation on the AP interface, this is called after all the
  *	information in bss_conf is set and beacon can be retrieved. A channel
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 3702d64..3965470 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -173,6 +173,24 @@ ieee80211_find_reservation_chanctx(struct ieee80211_local *local,
 	return NULL;
 }
 
+static bool
+ieee80211_chanctx_all_reserved_vifs_ready(struct ieee80211_local *local,
+					  struct ieee80211_chanctx *ctx)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	list_for_each_entry(sdata, &ctx->reserved_vifs, reserved_chanctx_list) {
+		if (!sdata->reserved_chanctx)
+			continue;
+		if (!sdata->reserved_ready)
+			return false;
+	}
+
+	return true;
+}
+
 static enum nl80211_chan_width ieee80211_get_sta_bw(struct ieee80211_sta *sta)
 {
 	switch (sta->bandwidth) {
@@ -912,38 +930,27 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_chanctx_conf *conf;
 	struct ieee80211_chanctx *new_ctx, *curr_ctx;
-	int ret = 0;
 
-	mutex_lock(&local->chanctx_mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	if (local->use_chanctx && !local->ops->switch_vif_chanctx)
+		return -ENOTSUPP;
 
 	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
 					 lockdep_is_held(&local->chanctx_mtx));
-	if (!conf) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (!conf)
+		return -EINVAL;
 
 	curr_ctx = container_of(conf, struct ieee80211_chanctx, conf);
 
 	new_ctx = ieee80211_find_reservation_chanctx(local, chandef, mode);
 	if (!new_ctx) {
-		if (ieee80211_chanctx_refcount(local, curr_ctx) == 1 &&
-		    (local->hw.flags & IEEE80211_HW_CHANGE_RUNNING_CHANCTX)) {
-			/* if we're the only users of the chanctx and
-			 * the driver supports changing a running
-			 * context, reserve our current context
-			 */
-			new_ctx = curr_ctx;
-		} else if (ieee80211_can_create_new_chanctx(local)) {
-			/* create a new context and reserve it */
+		if (ieee80211_can_create_new_chanctx(local)) {
 			new_ctx = ieee80211_new_chanctx(local, chandef, mode);
-			if (IS_ERR(new_ctx)) {
-				ret = PTR_ERR(new_ctx);
-				goto out;
-			}
+			if (IS_ERR(new_ctx))
+				return PTR_ERR(new_ctx);
 		} else {
-			ret = -EBUSY;
-			goto out;
+			new_ctx = curr_ctx;
 		}
 	}
 
@@ -951,82 +958,257 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 	sdata->reserved_chanctx = new_ctx;
 	sdata->reserved_chandef = *chandef;
 	sdata->reserved_radar_required = radar_required;
-out:
-	mutex_unlock(&local->chanctx_mtx);
-	return ret;
+	sdata->reserved_ready = false;
+
+	return 0;
 }
 
-int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
-				       u32 *changed)
+static void
+ieee80211_vif_chanctx_reservation_complete(struct ieee80211_sub_if_data *sdata)
 {
-	struct ieee80211_local *local = sdata->local;
-	struct ieee80211_chanctx *ctx;
-	struct ieee80211_chanctx *old_ctx;
-	struct ieee80211_chanctx_conf *conf;
-	int ret;
-	u32 tmp_changed = *changed;
+	/* stub */
+}
 
-	/* TODO: need to recheck if the chandef is usable etc.? */
+static int
+ieee80211_vif_use_reserved_incompat_nonctx(struct ieee80211_local *local,
+					   struct ieee80211_chanctx *old_ctx,
+					   struct ieee80211_chanctx *new_ctx,
+					   const struct cfg80211_chan_def *chandef)
+{
+	int err;
 
-	lockdep_assert_held(&local->mtx);
+	ieee80211_del_chanctx(local, old_ctx);
 
-	mutex_lock(&local->chanctx_mtx);
+	err = ieee80211_add_chanctx(local, new_ctx);
+	if (err) {
+		WARN_ON(ieee80211_add_chanctx(local, old_ctx));
+		return err;
+	}
 
-	ctx = sdata->reserved_chanctx;
-	if (WARN_ON(!ctx)) {
-		ret = -EINVAL;
-		goto out;
+	return 0;
+}
+
+static int
+ieee80211_vif_use_reserved_incompat_ctx(struct ieee80211_local *local,
+					struct ieee80211_chanctx *old_ctx,
+					struct ieee80211_chanctx *new_ctx,
+					const struct cfg80211_chan_def *chandef)
+{
+	struct ieee80211_sub_if_data *sdata_list[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
+	struct ieee80211_chanctx *old_ctx_list[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
+	struct ieee80211_chanctx *new_ctx_list[IEEE80211_MAX_NUM_SWITCH_VIFS] = {};
+	struct ieee80211_sub_if_data *sdata;
+	int n_vifs = 0, err;
+
+	list_for_each_entry(sdata, &old_ctx->reserved_vifs, reserved_chanctx_list) {
+		if (n_vifs > IEEE80211_MAX_NUM_SWITCH_VIFS)
+			return -EBUSY;
+
+		sdata_list[n_vifs] = sdata;
+		old_ctx_list[n_vifs] = old_ctx;
+		new_ctx_list[n_vifs] = new_ctx;
+
+		n_vifs++;
 	}
 
-	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
-					 lockdep_is_held(&local->chanctx_mtx));
-	if (!conf) {
-		ret = -EINVAL;
-		goto out;
+	err = drv_switch_vif_chanctx(local, sdata_list, old_ctx_list,
+				     new_ctx_list, n_vifs,
+				     IEEE80211_CHANCTX_SWMODE_SWAP);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int
+ieee80211_vif_use_reserved_incompat(struct ieee80211_local *local,
+				    struct ieee80211_chanctx *ctx,
+				    const struct cfg80211_chan_def *chandef)
+{
+	struct ieee80211_sub_if_data *sdata, *tmp;
+	struct ieee80211_chanctx *new_ctx;
+	u32 changed = 0;
+	int err;
+
+	lockdep_assert_held(&local->mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	if (!ieee80211_chanctx_all_reserved_vifs_ready(local, ctx))
+		return 0;
+
+	if (ieee80211_chanctx_num_assigned(local, ctx) !=
+	    ieee80211_chanctx_num_reserved(local, ctx)) {
+		wiphy_info(local->hw.wiphy,
+			   "channel context reservation cannot be finalized because some interfaces aren't switching\n");
+		err = -EBUSY;
+		goto err;
 	}
 
-	old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+	new_ctx = ieee80211_alloc_chanctx(local, chandef, ctx->mode);
+	if (!new_ctx) {
+		err = -ENOMEM;
+		goto err;
+	}
 
-	if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
-		tmp_changed |= BSS_CHANGED_BANDWIDTH;
+	list_del_rcu(&ctx->list);
 
-	sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+	list_for_each_entry(sdata, &ctx->reserved_vifs, reserved_chanctx_list) {
+		bool tmp = sdata->radar_required;
+		sdata->radar_required = sdata->reserved_radar_required;
+		sdata->reserved_radar_required = tmp;
 
-	/* unref our reservation */
-	sdata->reserved_chanctx = NULL;
-	sdata->radar_required = sdata->reserved_radar_required;
-	list_del(&sdata->reserved_chanctx_list);
+		if (sdata->radar_required)
+			new_ctx->conf.radar_enabled = true;
+	}
 
-	if (old_ctx == ctx) {
-		/* This is our own context, just change it */
-		ret = __ieee80211_vif_change_channel(sdata, old_ctx,
-						     &tmp_changed);
-		if (ret)
-			goto out;
+	if (local->use_chanctx) {
+		err = ieee80211_vif_use_reserved_incompat_ctx(local, ctx,
+							      new_ctx,
+							      chandef);
+		if (err)
+			goto err_revert;
 	} else {
-		ret = ieee80211_assign_vif_chanctx(sdata, ctx);
-		if (ieee80211_chanctx_refcount(local, old_ctx) == 0)
-			ieee80211_free_chanctx(local, old_ctx);
-		if (ret) {
-			/* if assign fails refcount stays the same */
-			if (ieee80211_chanctx_refcount(local, ctx) == 0)
-				ieee80211_free_chanctx(local, ctx);
-			goto out;
-		}
+		err = ieee80211_vif_use_reserved_incompat_nonctx(local, ctx,
+								 new_ctx,
+								 chandef);
+		if (err)
+			goto err_revert;
+	}
+
+	list_add_rcu(&new_ctx->list, &local->chanctx_list);
+
+	list_for_each_entry(sdata, &ctx->reserved_vifs, reserved_chanctx_list) {
+		rcu_assign_pointer(sdata->vif.chanctx_conf, &new_ctx->conf);
 
 		if (sdata->vif.type == NL80211_IFTYPE_AP)
 			__ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
 	}
 
-	*changed = tmp_changed;
+	list_for_each_entry(sdata, &ctx->reserved_vifs,
+			    reserved_chanctx_list) {
+		changed = 0;
+		if (sdata->vif.bss_conf.chandef.width !=
+		    sdata->reserved_chandef.width)
+			changed = BSS_CHANGED_BANDWIDTH;
+
+		sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+		if (changed)
+			ieee80211_bss_info_change_notify(sdata, changed);
+
+		ieee80211_recalc_txpower(sdata);
+	}
+
+	ieee80211_recalc_chanctx_chantype(local, new_ctx);
+	ieee80211_recalc_smps_chanctx(local, new_ctx);
+	ieee80211_recalc_chanctx_min_def(local, new_ctx);
+
+	list_for_each_entry_safe(sdata, tmp, &ctx->reserved_vifs,
+				 reserved_chanctx_list) {
+		list_del(&sdata->reserved_chanctx_list);
+		list_move(&sdata->assigned_chanctx_list,
+			  &new_ctx->assigned_vifs);
+		sdata->reserved_chanctx = NULL;
+
+		ieee80211_vif_chanctx_reservation_complete(sdata);
+	}
+
+	kfree_rcu(ctx, rcu_head);
+	return 0;
+
+err_revert:
+	kfree_rcu(new_ctx, rcu_head);
+	list_add_rcu(&ctx->list, &local->chanctx_list);
+	list_for_each_entry(sdata, &ctx->reserved_vifs, reserved_chanctx_list) {
+		bool tmp = sdata->radar_required;
+		sdata->radar_required = sdata->reserved_radar_required;
+		sdata->reserved_radar_required = tmp;
+	}
+err:
+	list_for_each_entry_safe(sdata, tmp, &ctx->reserved_vifs,
+				 reserved_chanctx_list) {
+		list_del(&sdata->reserved_chanctx_list);
+		sdata->reserved_chanctx = NULL;
+		ieee80211_vif_chanctx_reservation_complete(sdata);
+	}
+	return err;
+}
+
+static int
+ieee80211_vif_use_reserved_compat(struct ieee80211_sub_if_data *sdata,
+				  struct ieee80211_chanctx *old_ctx,
+				  struct ieee80211_chanctx *new_ctx)
+{
+	struct ieee80211_local *local = sdata->local;
+	u32 changed = 0;
+	int err;
+
+	lockdep_assert_held(&local->mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	list_del(&sdata->reserved_chanctx_list);
+	sdata->reserved_chanctx = NULL;
+
+	err = ieee80211_assign_vif_chanctx(sdata, new_ctx);
+	if (ieee80211_chanctx_refcount(local, old_ctx) == 0)
+		ieee80211_free_chanctx(local, old_ctx);
+	if (err) {
+		/* if assign fails refcount stays the same */
+		if (ieee80211_chanctx_refcount(local, new_ctx) == 0)
+			ieee80211_free_chanctx(local, new_ctx);
+		goto out;
+	}
+
+	if (sdata->vif.type == NL80211_IFTYPE_AP)
+		__ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
+
+	if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
+		changed = BSS_CHANGED_BANDWIDTH;
+
+	sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+
+	if (changed)
+		ieee80211_bss_info_change_notify(sdata, changed);
 
-	ieee80211_recalc_chanctx_chantype(local, ctx);
-	ieee80211_recalc_smps_chanctx(local, ctx);
-	ieee80211_recalc_radar_chanctx(local, ctx);
-	ieee80211_recalc_chanctx_min_def(local, ctx);
 out:
-	mutex_unlock(&local->chanctx_mtx);
-	return ret;
+	ieee80211_vif_chanctx_reservation_complete(sdata);
+	return err;
+}
+
+int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx *ctx;
+	struct ieee80211_chanctx *old_ctx;
+	struct ieee80211_chanctx_conf *conf;
+	const struct cfg80211_chan_def *chandef;
+
+	lockdep_assert_held(&local->mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	ctx = sdata->reserved_chanctx;
+	if (WARN_ON(!ctx))
+		return -EINVAL;
+
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf)
+		return -EINVAL;
+
+	old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+	if (WARN_ON(sdata->reserved_ready))
+		return -EINVAL;
+
+	chandef = ieee80211_chanctx_reserved_chandef(local, ctx, NULL);
+	if (WARN_ON(!chandef))
+		return -EINVAL;
+
+	sdata->reserved_ready = true;
+
+	if (cfg80211_chandef_compatible(&ctx->conf.def, chandef))
+		return ieee80211_vif_use_reserved_compat(sdata, old_ctx, ctx);
+	else
+		return ieee80211_vif_use_reserved_incompat(local, ctx, chandef);
 }
 
 int ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 69da2d6..340b2ac 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -767,6 +767,7 @@ struct ieee80211_sub_if_data {
 	struct ieee80211_chanctx *reserved_chanctx;
 	struct cfg80211_chan_def reserved_chandef;
 	bool reserved_radar_required;
+	bool reserved_ready;
 
 	/* used to reconfigure hardware SM PS */
 	struct work_struct recalc_smps;
@@ -1790,8 +1791,7 @@ ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 			      enum ieee80211_chanctx_mode mode,
 			      bool radar_required);
 int __must_check
-ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
-				   u32 *changed);
+ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata);
 int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata);
 
 int __must_check
-- 
1.8.5.3

Re: [PATCH v6 2/6] mac80211: implement multi-vif in-place reservations

[Johannes Berg]

On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:

> This introduces a special hook
> ieee80211_vif_chanctx_reservation_complete(). This
> is currently an empty stub and will be filled in
> by AP/STA CSA code. This is required to implement
> 2-step CSA finalization.

I really don't see any value in this - just put it into the patch
implementing it?

> + *	The callback is optional for channel context based drivers but is
> + *	required to support channel switching. The callback and can sleep.

That probably belongs to the previous patch, but isn't even true there
or here afaict.

> +static int
> +ieee80211_vif_use_reserved_incompat(struct ieee80211_local *local,
> +				    struct ieee80211_chanctx *ctx,
> +				    const struct cfg80211_chan_def *chandef)
> +{
> +	struct ieee80211_sub_if_data *sdata, *tmp;
> +	struct ieee80211_chanctx *new_ctx;
> +	u32 changed = 0;
> +	int err;
> +
> +	lockdep_assert_held(&local->mtx);
> +	lockdep_assert_held(&local->chanctx_mtx);
> +
> +	if (!ieee80211_chanctx_all_reserved_vifs_ready(local, ctx))
> +		return 0;
> +
> +	if (ieee80211_chanctx_num_assigned(local, ctx) !=
> +	    ieee80211_chanctx_num_reserved(local, ctx)) {
> +		wiphy_info(local->hw.wiphy,
> +			   "channel context reservation cannot be finalized because some interfaces aren't switching\n");
> +		err = -EBUSY;
> +		goto err;
>  	}

I don't think I understand that condition, it's it possible to switch
from two vifs using two channels to both using a single third one?

> +	ieee80211_recalc_chanctx_chantype(local, new_ctx);
> +	ieee80211_recalc_smps_chanctx(local, new_ctx);
> +	ieee80211_recalc_chanctx_min_def(local, new_ctx);

vs.

> -	ieee80211_recalc_chanctx_chantype(local, ctx);
> -	ieee80211_recalc_smps_chanctx(local, ctx);
> -	ieee80211_recalc_radar_chanctx(local, ctx);
> -	ieee80211_recalc_chanctx_min_def(local, ctx);

Did I miss something? Maybe it would be good to squeeze in that patch
that made a recalc_all() function to call all of these.

johannes

Re: [PATCH v6 2/6] mac80211: implement multi-vif in-place reservations

[Michal Kazior]

On 22 May 2014 16:51, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
>
>> This introduces a special hook
>> ieee80211_vif_chanctx_reservation_complete(). This
>> is currently an empty stub and will be filled in
>> by AP/STA CSA code. This is required to implement
>> 2-step CSA finalization.
>
> I really don't see any value in this - just put it into the patch
> implementing it?

Okay, I'll move it to the AP CSA patch then.


>> + *   The callback is optional for channel context based drivers but is
>> + *   required to support channel switching. The callback and can sleep.
>
> That probably belongs to the previous patch, but isn't even true there
> or here afaict.

Ah, rebasing..


>> +static int
>> +ieee80211_vif_use_reserved_incompat(struct ieee80211_local *local,
>> +                                 struct ieee80211_chanctx *ctx,
>> +                                 const struct cfg80211_chan_def *chandef)
>> +{
>> +     struct ieee80211_sub_if_data *sdata, *tmp;
>> +     struct ieee80211_chanctx *new_ctx;
>> +     u32 changed = 0;
>> +     int err;
>> +
>> +     lockdep_assert_held(&local->mtx);
>> +     lockdep_assert_held(&local->chanctx_mtx);
>> +
>> +     if (!ieee80211_chanctx_all_reserved_vifs_ready(local, ctx))
>> +             return 0;
>> +
>> +     if (ieee80211_chanctx_num_assigned(local, ctx) !=
>> +         ieee80211_chanctx_num_reserved(local, ctx)) {
>> +             wiphy_info(local->hw.wiphy,
>> +                        "channel context reservation cannot be finalized because some interfaces aren't switching\n");
>> +             err = -EBUSY;
>> +             goto err;
>>       }
>
> I don't think I understand that condition, it's it possible to switch
> from two vifs using two channels to both using a single third one?

I assume you have a case where max number of different channels is
degraded (e.g. non-radar -> more restrictive radar combination) in
mind, right? In that case this won't work with this code. Now that I
think about it it also won't work with cross-swapping (2 chanctx are
being swapped and some vifs try to interchange between).

I'll have to re-think this a bit more.


>> +     ieee80211_recalc_chanctx_chantype(local, new_ctx);
>> +     ieee80211_recalc_smps_chanctx(local, new_ctx);
>> +     ieee80211_recalc_chanctx_min_def(local, new_ctx);
>
> vs.
>
>> -     ieee80211_recalc_chanctx_chantype(local, ctx);
>> -     ieee80211_recalc_smps_chanctx(local, ctx);
>> -     ieee80211_recalc_radar_chanctx(local, ctx);
>> -     ieee80211_recalc_chanctx_min_def(local, ctx);
>
> Did I miss something? Maybe it would be good to squeeze in that patch
> that made a recalc_all() function to call all of these.

The radar is now calculated in the _incompat manually because it
should be saying "radar" before you call into
switch_vif_chanctx/create new chanctx. I mean, you could initially
bind a chanctx with ch52 without radar flag and then update it right
away but it didn't sit right with me.


Michał

Re: [PATCH v6 2/6] mac80211: implement multi-vif in-place reservations

[Michal Kazior]

On 23 May 2014 08:16, Michal Kazior <michal.kazior@tieto.com> wrote:
> On 22 May 2014 16:51, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
>>> +static int
>>> +ieee80211_vif_use_reserved_incompat(struct ieee80211_local *local,
>>> +                                 struct ieee80211_chanctx *ctx,
>>> +                                 const struct cfg80211_chan_def *chandef)
>>> +{
>>> +     struct ieee80211_sub_if_data *sdata, *tmp;
>>> +     struct ieee80211_chanctx *new_ctx;
>>> +     u32 changed = 0;
>>> +     int err;
>>> +
>>> +     lockdep_assert_held(&local->mtx);
>>> +     lockdep_assert_held(&local->chanctx_mtx);
>>> +
>>> +     if (!ieee80211_chanctx_all_reserved_vifs_ready(local, ctx))
>>> +             return 0;
>>> +
>>> +     if (ieee80211_chanctx_num_assigned(local, ctx) !=
>>> +         ieee80211_chanctx_num_reserved(local, ctx)) {
>>> +             wiphy_info(local->hw.wiphy,
>>> +                        "channel context reservation cannot be finalized because some interfaces aren't switching\n");
>>> +             err = -EBUSY;
>>> +             goto err;
>>>       }
>>
>> I don't think I understand that condition, it's it possible to switch
>> from two vifs using two channels to both using a single third one?
>
> I assume you have a case where max number of different channels is
> degraded (e.g. non-radar -> more restrictive radar combination) in
> mind, right? In that case this won't work with this code. Now that I
> think about it it also won't work with cross-swapping (2 chanctx are
> being swapped and some vifs try to interchange between).
>
> I'll have to re-think this a bit more.

Actually I think I've just spotted a little problem. This is also
related to the `[PATCH v6 6/6] cfg80211: remove channel_switch
combination check` thread.

Currently mac80211 verifies interface combinations in channel_switch.
It has an implicit assumption that channel switch does not change the
number of different channels (or at least it doesn't decrease it).

We were discussing degradation of number of different channels
earlier, e.g. due to non-radar -> radar change (going from max 2
channels to 1). With how things are now it's impossible to perform
such a channel switch.

Basically if we want to support this kind of channel switch we must
remove interface combination checks from ieee80211_channel_switch
because it's simply impossible to know the future (userspace may or
may not submit subsequent requests to match an existing interface
combination).

So while I still intend to rework the multi-vif reservation a little
(to support cross-swapping at least) I'd like to know if I should
invest my time implementing degradation of number of channels in
multi-vif reservation or not.


Michał

Re: [PATCH v6 2/6] mac80211: implement multi-vif in-place reservations

[Johannes Berg]

On Fri, 2014-05-23 at 14:23 +0200, Michal Kazior wrote:

> Actually I think I've just spotted a little problem. This is also
> related to the `[PATCH v6 6/6] cfg80211: remove channel_switch
> combination check` thread.
> 
> Currently mac80211 verifies interface combinations in channel_switch.
> It has an implicit assumption that channel switch does not change the
> number of different channels (or at least it doesn't decrease it).
> 
> We were discussing degradation of number of different channels
> earlier, e.g. due to non-radar -> radar change (going from max 2
> channels to 1). With how things are now it's impossible to perform
> such a channel switch.

Good point.

> Basically if we want to support this kind of channel switch we must
> remove interface combination checks from ieee80211_channel_switch
> because it's simply impossible to know the future (userspace may or
> may not submit subsequent requests to match an existing interface
> combination).

I guess this is a side effect of not having all the channel switches in
the API in a single call, which I think you had originally? :-)

> So while I still intend to rework the multi-vif reservation a little
> (to support cross-swapping at least) I'd like to know if I should
> invest my time implementing degradation of number of channels in
> multi-vif reservation or not.

It doesn't matter to me. If you don't need the feature now, don't
implement it now. It's not like it restricts the API (much) making
future implementation difficult.

johannes

[PATCH v6 3/6] mac80211: make check_combinations() aware of chanctx reservation

[Michal Kazior]

The ieee80211_check_combinations() computes
radar_detect accordingly depending on chanctx
reservation status.

This makes it possible to use the function for
channel_switch validation.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

Notes:
    v3:
     * fix typo in comment [Johannes]
     * fix comment style [Johannes]

 net/mac80211/util.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 7e0dd4b..419bf55 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2835,6 +2835,42 @@ void ieee80211_recalc_dtim(struct ieee80211_local *local,
 	ps->dtim_count = dtim_count;
 }
 
+static u8 ieee80211_chanctx_radar_detect(struct ieee80211_local *local,
+					 struct ieee80211_chanctx *ctx)
+{
+	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_chanctx_conf *conf;
+	u8 radar_detect = 0;
+	bool in_place = false;
+
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	list_for_each_entry(sdata, &ctx->reserved_vifs, reserved_chanctx_list) {
+		if (sdata->reserved_radar_required)
+			radar_detect |= BIT(sdata->reserved_chandef.width);
+
+		conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+				lockdep_is_held(&local->chanctx_mtx));
+		if (conf == &ctx->conf)
+			in_place = true;
+	}
+
+	/*
+	 * if chanctx has in-place reservation then consider only the future
+	 * radar_detect. multi-vif reservation is deferred so ignore assigned
+	 * vifs. it is impossible for new vifs to be bound to an in-place
+	 * reserved chanctx so consistency is guaranteed
+	 */
+	if (in_place)
+		return radar_detect;
+
+	list_for_each_entry(sdata, &ctx->assigned_vifs, assigned_chanctx_list)
+		if (sdata->radar_required)
+			radar_detect |= BIT(sdata->vif.bss_conf.chandef.width);
+
+	return radar_detect;
+}
+
 int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 				 const struct cfg80211_chan_def *chandef,
 				 enum ieee80211_chanctx_mode chanmode,
@@ -2876,8 +2912,7 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 		num[iftype] = 1;
 
 	list_for_each_entry(ctx, &local->chanctx_list, list) {
-		if (ctx->conf.radar_enabled)
-			radar_detect |= BIT(ctx->conf.def.width);
+		radar_detect |= ieee80211_chanctx_radar_detect(local, ctx);
 		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) {
 			num_different_channels++;
 			continue;
-- 
1.8.5.3

[PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Michal Kazior]

Channel switch finalization is now 2-step. First
step is when driver calls csa_finish(), the other
is when reservation is actually finalized (which
be defered for in-place reservation).

It is now safe to call ieee80211_csa_finish() more
then once.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

Notes:
    v3:
     * fix lockdep typo s/mtx/chanctx_mtx/ [Johannes]
     * fix comment style [Johannes]
     * use goto for cleaner unlocking/returning [Johannes]
     * squash with ieee80211_vif_change_channel() removal patch [Johannes]
     * fix commit message [Johannes]
     * add resilience for multiple ieee80211_csa_finish() calls
    
    v4:
     * split removal of ieee80211_vif_change_channel()

 net/mac80211/cfg.c  | 82 +++++++++++++++++++++++++++++++++++------------------
 net/mac80211/chan.c | 11 ++++++-
 2 files changed, 65 insertions(+), 28 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aab258b..b75014e 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3114,17 +3114,35 @@ static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 
 	sdata_assert_lock(sdata);
 	lockdep_assert_held(&local->mtx);
+	lockdep_assert_held(&local->chanctx_mtx);
 
-	sdata->radar_required = sdata->csa_radar_required;
-	err = ieee80211_vif_change_channel(sdata, &changed);
-	if (err < 0)
-		return err;
+	/*
+	 * using reservation isn't immediate as it may be deferred until later
+	 * with multi-vif. once reservation is complete it will re-schedule the
+	 * work with no reserved_chanctx so verify chandef to check if it
+	 * completed successfully
+	 */
 
-	if (!local->use_chanctx) {
-		local->_oper_chandef = sdata->csa_chandef;
-		ieee80211_hw_config(local, 0);
+	if (sdata->reserved_chanctx) {
+		/*
+		 * with multi-vif csa driver may call ieee80211_csa_finish()
+		 * many times while waiting for other interfaces to use their
+		 * reservations
+		 */
+		if (sdata->reserved_ready)
+			return 0;
+
+		err = ieee80211_vif_use_reserved_context(sdata);
+		if (err)
+			return err;
+
+		return 0;
 	}
 
+	if (!cfg80211_chandef_identical(&sdata->vif.bss_conf.chandef,
+					&sdata->csa_chandef))
+		return -EINVAL;
+
 	sdata->vif.csa_active = false;
 
 	err = ieee80211_set_after_csa_beacon(sdata, &changed);
@@ -3160,6 +3178,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 
 	sdata_lock(sdata);
 	mutex_lock(&local->mtx);
+	mutex_lock(&local->chanctx_mtx);
 
 	/* AP might have been stopped while waiting for the lock. */
 	if (!sdata->vif.csa_active)
@@ -3171,6 +3190,7 @@ void ieee80211_csa_finalize_work(struct work_struct *work)
 	ieee80211_csa_finalize(sdata);
 
 unlock:
+	mutex_unlock(&local->chanctx_mtx);
 	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
 }
@@ -3314,7 +3334,7 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_chanctx_conf *conf;
 	struct ieee80211_chanctx *chanctx;
-	int err, num_chanctx, changed = 0;
+	int err, changed = 0;
 
 	sdata_assert_lock(sdata);
 	lockdep_assert_held(&local->mtx);
@@ -3329,37 +3349,43 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 				       &sdata->vif.bss_conf.chandef))
 		return -EINVAL;
 
+	/* don't allow another channel switch if one is already active. */
+	if (sdata->vif.csa_active)
+		return -EBUSY;
+
 	mutex_lock(&local->chanctx_mtx);
 	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
 					 lockdep_is_held(&local->chanctx_mtx));
 	if (!conf) {
-		mutex_unlock(&local->chanctx_mtx);
-		return -EBUSY;
+		err = -EBUSY;
+		goto out;
 	}
 
-	/* don't handle for multi-VIF cases */
 	chanctx = container_of(conf, struct ieee80211_chanctx, conf);
-	if (ieee80211_chanctx_refcount(local, chanctx) > 1) {
-		mutex_unlock(&local->chanctx_mtx);
-		return -EBUSY;
+	if (!chanctx) {
+		err = -EBUSY;
+		goto out;
 	}
-	num_chanctx = 0;
-	list_for_each_entry_rcu(chanctx, &local->chanctx_list, list)
-		num_chanctx++;
-	mutex_unlock(&local->chanctx_mtx);
 
-	if (num_chanctx > 1)
-		return -EBUSY;
+	err = ieee80211_vif_reserve_chanctx(sdata, &params->chandef,
+					    chanctx->mode,
+					    params->radar_required);
+	if (err)
+		goto out;
 
-	/* don't allow another channel switch if one is already active. */
-	if (sdata->vif.csa_active)
-		return -EBUSY;
+	/* if reservation is invalid then this will fail */
+	err = ieee80211_check_combinations(sdata, NULL, chanctx->mode, 0);
+	if (err) {
+		ieee80211_vif_unreserve_chanctx(sdata);
+		goto out;
+	}
 
 	err = ieee80211_set_csa_beacon(sdata, params, &changed);
-	if (err)
-		return err;
+	if (err) {
+		ieee80211_vif_unreserve_chanctx(sdata);
+		goto out;
+	}
 
-	sdata->csa_radar_required = params->radar_required;
 	sdata->csa_chandef = params->chandef;
 	sdata->csa_block_tx = params->block_tx;
 	sdata->vif.csa_active = true;
@@ -3377,7 +3403,9 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
 		ieee80211_csa_finalize(sdata);
 	}
 
-	return 0;
+out:
+	mutex_unlock(&local->chanctx_mtx);
+	return err;
 }
 
 int ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev,
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 3965470..6ca007f 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -966,7 +966,16 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 static void
 ieee80211_vif_chanctx_reservation_complete(struct ieee80211_sub_if_data *sdata)
 {
-	/* stub */
+	switch (sdata->vif.type) {
+	case NL80211_IFTYPE_AP:
+	case NL80211_IFTYPE_ADHOC:
+	case NL80211_IFTYPE_MESH_POINT:
+		ieee80211_queue_work(&sdata->local->hw,
+				     &sdata->csa_finalize_work);
+		break;
+	default:
+		break;
+	}
 }
 
 static int
-- 
1.8.5.3

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Johannes Berg]

On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
> Channel switch finalization is now 2-step. First
> step is when driver calls csa_finish(), the other
> is when reservation is actually finalized (which
> be defered for in-place reservation).
> 
> It is now safe to call ieee80211_csa_finish() more
> then once.

But you'll WARN_ON() if they're actually not at the same time and you
grab a beacon (or for the template case, call csa_update) in the
meantime, right? I'd really like to have all those driver requirements
(e.g. to stop beaconing) better documented.

johannes

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Michal Kazior]

On 22 May 2014 16:54, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
>> Channel switch finalization is now 2-step. First
>> step is when driver calls csa_finish(), the other
>> is when reservation is actually finalized (which
>> be defered for in-place reservation).
>>
>> It is now safe to call ieee80211_csa_finish() more
>> then once.
>
> But you'll WARN_ON() if they're actually not at the same time and you
> grab a beacon (or for the template case, call csa_update) in the
> meantime, right? I'd really like to have all those driver requirements
> (e.g. to stop beaconing) better documented.

Good point. I suppose it should be stated in the docs that once you
reach ieee80211_csa_is_complete() being true you must not call
ieee80211_beacon_get() nor ieee80211_csa_update_counter(). ath9k and
ath10k conform to this.

I wonder what driver should be supposed to look at before starting to
beacon again? csa_active isn't well protected to be depended upon. If
we should create a ieee80211_csa_is_active() that just checks if
beacon->csa_counter_offset[0] != 0 (assuming my other csa counter
patches are applied) then it's still racy:
 a) rcu_dereference() across ieee80211_csa_is_active(), _is_complete()
and _beacon_get() can yield different beacon pointers
 b) cs_count <= 1 yields no beacon update (thus no counters/offsets,
meaning both _csa_is_complete and _csa_is_active() are `false` thus
suggesting driver can beacon as if nothing happened)

We could fix (b) by simply not treating cs_count <= 1 so special and
update the beacon anyway. For (a) to work we'd need either make a
single-call do-all function or introduce an additional call and a
generic pointer/structure to be passed to other functions so that a
beacon pointer is consistent across calls.


Michał

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Johannes Berg]

On Fri, 2014-05-23 at 08:49 +0200, Michal Kazior wrote:

> > But you'll WARN_ON() if they're actually not at the same time and you
> > grab a beacon (or for the template case, call csa_update) in the
> > meantime, right? I'd really like to have all those driver requirements
> > (e.g. to stop beaconing) better documented.
> 
> Good point. I suppose it should be stated in the docs that once you
> reach ieee80211_csa_is_complete() being true you must not call
> ieee80211_beacon_get() nor ieee80211_csa_update_counter(). ath9k and
> ath10k conform to this.

Right, it's still a bit strange that you just have to stop beaconing.
Dunno. I guess I don't care all that much since our driver won't support
multiple AP interfaces anyway :-)

> I wonder what driver should be supposed to look at before starting to
> beacon again? csa_active isn't well protected to be depended upon. If
> we should create a ieee80211_csa_is_active() that just checks if
> beacon->csa_counter_offset[0] != 0 (assuming my other csa counter
> patches are applied) then it's still racy:
>  a) rcu_dereference() across ieee80211_csa_is_active(), _is_complete()
> and _beacon_get() can yield different beacon pointers
>  b) cs_count <= 1 yields no beacon update (thus no counters/offsets,
> meaning both _csa_is_complete and _csa_is_active() are `false` thus
> suggesting driver can beacon as if nothing happened)
> 
> We could fix (b) by simply not treating cs_count <= 1 so special and
> update the beacon anyway. For (a) to work we'd need either make a
> single-call do-all function or introduce an additional call and a
> generic pointer/structure to be passed to other functions so that a
> beacon pointer is consistent across calls.

You could technically just return NULL in this period, but I don't know
how well drivers would cope with that (though technically they have to
cope with it due to memory allocation failures anyway)

johannes

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Michal Kazior]

On 23 May 2014 10:44, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2014-05-23 at 08:49 +0200, Michal Kazior wrote:
>
>> > But you'll WARN_ON() if they're actually not at the same time and you
>> > grab a beacon (or for the template case, call csa_update) in the
>> > meantime, right? I'd really like to have all those driver requirements
>> > (e.g. to stop beaconing) better documented.
>>
>> Good point. I suppose it should be stated in the docs that once you
>> reach ieee80211_csa_is_complete() being true you must not call
>> ieee80211_beacon_get() nor ieee80211_csa_update_counter(). ath9k and
>> ath10k conform to this.
>
> Right, it's still a bit strange that you just have to stop beaconing.
> Dunno. I guess I don't care all that much since our driver won't support
> multiple AP interfaces anyway :-)

If you keep on beaconing for too long on a DFS channel after detecting
a radar you risk breaking local law (ETSI and FCC specify quiescing
periods).


>> I wonder what driver should be supposed to look at before starting to
>> beacon again? csa_active isn't well protected to be depended upon. If
>> we should create a ieee80211_csa_is_active() that just checks if
>> beacon->csa_counter_offset[0] != 0 (assuming my other csa counter
>> patches are applied) then it's still racy:
>>  a) rcu_dereference() across ieee80211_csa_is_active(), _is_complete()
>> and _beacon_get() can yield different beacon pointers
>>  b) cs_count <= 1 yields no beacon update (thus no counters/offsets,
>> meaning both _csa_is_complete and _csa_is_active() are `false` thus
>> suggesting driver can beacon as if nothing happened)
>>
>> We could fix (b) by simply not treating cs_count <= 1 so special and
>> update the beacon anyway. For (a) to work we'd need either make a
>> single-call do-all function or introduce an additional call and a
>> generic pointer/structure to be passed to other functions so that a
>> beacon pointer is consistent across calls.
>
> You could technically just return NULL in this period, but I don't know
> how well drivers would cope with that (though technically they have to
> cope with it due to memory allocation failures anyway)

Sounds good to me.

I guess you'd use ieee80211_csa_is_complete (or
ieee80211_csa_update_counter) for template-based drivers to stop
beaconing.


Michał

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Johannes Berg]

On Fri, 2014-05-23 at 10:58 +0200, Michal Kazior wrote:

> If you keep on beaconing for too long on a DFS channel after detecting
> a radar you risk breaking local law (ETSI and FCC specify quiescing
> periods).

Yes, this is true.

> > You could technically just return NULL in this period, but I don't know
> > how well drivers would cope with that (though technically they have to
> > cope with it due to memory allocation failures anyway)
> 
> Sounds good to me.
> 
> I guess you'd use ieee80211_csa_is_complete (or
> ieee80211_csa_update_counter) for template-based drivers to stop
> beaconing.

The thing is, I don't see that template based drivers would be able to
stop beaconing :-)

This whole design that requires it seems a bit fishy to start with, so I
can't really imagine anyone writing firmware that supports it. That
said, ours only supports a single beacon at a time anyway, so we'll
switch immediately and don't have to do that anyway.

johannes

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Michal Kazior]

On 23 May 2014 11:07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2014-05-23 at 10:58 +0200, Michal Kazior wrote:
>
>> If you keep on beaconing for too long on a DFS channel after detecting
>> a radar you risk breaking local law (ETSI and FCC specify quiescing
>> periods).
>
> Yes, this is true.
>
>> > You could technically just return NULL in this period, but I don't know
>> > how well drivers would cope with that (though technically they have to
>> > cope with it due to memory allocation failures anyway)
>>
>> Sounds good to me.
>>
>> I guess you'd use ieee80211_csa_is_complete (or
>> ieee80211_csa_update_counter) for template-based drivers to stop
>> beaconing.
>
> The thing is, I don't see that template based drivers would be able to
> stop beaconing :-)
>
> This whole design that requires it seems a bit fishy to start with, so I
> can't really imagine anyone writing firmware that supports it. That
> said, ours only supports a single beacon at a time anyway, so we'll
> switch immediately and don't have to do that anyway.

What if you have AP+STA and STA switches too? It might delay your AP
interface switch, no? We could assume userspace always requests us to
do The Right Thing, but should we trust it so much?


Michał

Re: [PATCH v6 4/6] mac80211: use chanctx reservation for AP CSA

[Johannes Berg]

On Fri, 2014-05-23 at 11:35 +0200, Michal Kazior wrote:

> > The thing is, I don't see that template based drivers would be able to
> > stop beaconing :-)
> >
> > This whole design that requires it seems a bit fishy to start with, so I
> > can't really imagine anyone writing firmware that supports it. That
> > said, ours only supports a single beacon at a time anyway, so we'll
> > switch immediately and don't have to do that anyway.
> 
> What if you have AP+STA and STA switches too? It might delay your AP
> interface switch, no? We could assume userspace always requests us to
> do The Right Thing, but should we trust it so much?

Most of the time we'd probably have a spare channel context anyway, so
it's not that big of a deal.

If we don't, well, I guess we might have to disconnect the STA or
something.

johannes

[PATCH v6 5/6] mac80211: use chanctx reservation for STA CSA

[Michal Kazior]

Channel switch finalization is now 2-step. First
step is when driver calls chswitch_done(), the
other is when reservation is actually finalized
(which be defered for in-place reservation).

It is now safe to call ieee80211_chswitch_done()
more than once.

Also remove the ieee80211_vif_change_channel()
because it is no longer used.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

Notes:
    v3:
     * fix comment style [Johannes]
     * add resilience for multiple ieee80211_chswitch_done() calls
    
    v3:
     * fix lockdep typo s/mtx/chanctx_mtx/ [Johannes]
     * fix comment style [Johannes]
     * use goto for cleaner unlocking/returning [Johannes]
     * squash with ieee80211_vif_change_channel() removal patch [Johannes]
     * fix commit message [Johannes]
     * add resilience for multiple ieee80211_csa_finish() calls
    
    v4:
     * squash with removal of ieee80211_vif_change_channel()

 net/mac80211/chan.c        | 68 ++-------------------------------
 net/mac80211/ieee80211_i.h |  5 ---
 net/mac80211/mlme.c        | 94 ++++++++++++++++++++++++++++++----------------
 3 files changed, 65 insertions(+), 102 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 6ca007f..52d79f2 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -798,70 +798,6 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 	return ret;
 }
 
-static int __ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
-					  struct ieee80211_chanctx *ctx,
-					  u32 *changed)
-{
-	struct ieee80211_local *local = sdata->local;
-	const struct cfg80211_chan_def *chandef = &sdata->csa_chandef;
-	u32 chanctx_changed = 0;
-
-	if (!cfg80211_chandef_usable(sdata->local->hw.wiphy, chandef,
-				     IEEE80211_CHAN_DISABLED))
-		return -EINVAL;
-
-	if (ieee80211_chanctx_refcount(local, ctx) != 1)
-		return -EINVAL;
-
-	if (sdata->vif.bss_conf.chandef.width != chandef->width) {
-		chanctx_changed = IEEE80211_CHANCTX_CHANGE_WIDTH;
-		*changed |= BSS_CHANGED_BANDWIDTH;
-	}
-
-	sdata->vif.bss_conf.chandef = *chandef;
-	ctx->conf.def = *chandef;
-
-	chanctx_changed |= IEEE80211_CHANCTX_CHANGE_CHANNEL;
-	drv_change_chanctx(local, ctx, chanctx_changed);
-
-	ieee80211_recalc_chanctx_chantype(local, ctx);
-	ieee80211_recalc_smps_chanctx(local, ctx);
-	ieee80211_recalc_radar_chanctx(local, ctx);
-	ieee80211_recalc_chanctx_min_def(local, ctx);
-
-	return 0;
-}
-
-int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
-				 u32 *changed)
-{
-	struct ieee80211_local *local = sdata->local;
-	struct ieee80211_chanctx_conf *conf;
-	struct ieee80211_chanctx *ctx;
-	int ret;
-
-	lockdep_assert_held(&local->mtx);
-
-	/* should never be called if not performing a channel switch. */
-	if (WARN_ON(!sdata->vif.csa_active))
-		return -EINVAL;
-
-	mutex_lock(&local->chanctx_mtx);
-	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
-					 lockdep_is_held(&local->chanctx_mtx));
-	if (!conf) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	ctx = container_of(conf, struct ieee80211_chanctx, conf);
-
-	ret = __ieee80211_vif_change_channel(sdata, ctx, changed);
- out:
-	mutex_unlock(&local->chanctx_mtx);
-	return ret;
-}
-
 static void
 __ieee80211_vif_copy_chanctx_to_vlans(struct ieee80211_sub_if_data *sdata,
 				      bool clear)
@@ -973,6 +909,10 @@ ieee80211_vif_chanctx_reservation_complete(struct ieee80211_sub_if_data *sdata)
 		ieee80211_queue_work(&sdata->local->hw,
 				     &sdata->csa_finalize_work);
 		break;
+	case NL80211_IFTYPE_STATION:
+		ieee80211_queue_work(&sdata->local->hw,
+				     &sdata->u.mgd.chswitch_work);
+		break;
 	default:
 		break;
 	}
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 340b2ac..06ab5a4 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -756,7 +756,6 @@ struct ieee80211_sub_if_data {
 	struct mac80211_qos_map __rcu *qos_map;
 
 	struct work_struct csa_finalize_work;
-	bool csa_radar_required;
 	bool csa_block_tx; /* write-protected by sdata_lock and local->mtx */
 	struct cfg80211_chan_def csa_chandef;
 
@@ -1798,10 +1797,6 @@ int __must_check
 ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
 			       const struct cfg80211_chan_def *chandef,
 			       u32 *changed);
-/* NOTE: only use ieee80211_vif_change_channel() for channel switch */
-int __must_check
-ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
-			     u32 *changed);
 void ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata);
 void ieee80211_vif_vlan_copy_chanctx(struct ieee80211_sub_if_data *sdata);
 void ieee80211_vif_copy_chanctx_to_vlans(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 7f073ef..dc4b575 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -940,51 +940,69 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 		container_of(work, struct ieee80211_sub_if_data, u.mgd.chswitch_work);
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
-	u32 changed = 0;
 	int ret;
 
 	if (!ieee80211_sdata_running(sdata))
 		return;
 
 	sdata_lock(sdata);
+	mutex_lock(&local->mtx);
+	mutex_lock(&local->chanctx_mtx);
+
 	if (!ifmgd->associated)
 		goto out;
 
-	mutex_lock(&local->mtx);
-	ret = ieee80211_vif_change_channel(sdata, &changed);
-	mutex_unlock(&local->mtx);
-	if (ret) {
+	if (!sdata->vif.csa_active)
+		goto out;
+
+	/*
+	 * using reservation isn't immediate as it may be deferred until later
+	 * with multi-vif. once reservation is complete it will re-schedule the
+	 * work with no reserved_chanctx so verify chandef to check if it
+	 * completed successfully
+	 */
+
+	if (sdata->reserved_chanctx) {
+		/*
+		 * with multi-vif csa driver may call ieee80211_csa_finish()
+		 * many times while waiting for other interfaces to use their
+		 * reservations
+		 */
+		if (sdata->reserved_ready)
+			goto out;
+
+		ret = ieee80211_vif_use_reserved_context(sdata);
+		if (ret) {
+			sdata_info(sdata,
+				   "failed to use reserved channel context, disconnecting (err=%d)\n",
+				   ret);
+			ieee80211_queue_work(&sdata->local->hw,
+					     &ifmgd->csa_connection_drop_work);
+			goto out;
+		}
+
+		goto out;
+	}
+
+	if (!cfg80211_chandef_identical(&sdata->vif.bss_conf.chandef,
+					&sdata->csa_chandef)) {
 		sdata_info(sdata,
-			   "vif channel switch failed, disconnecting\n");
+			   "failed to finalize channel switch, disconnecting\n");
 		ieee80211_queue_work(&sdata->local->hw,
 				     &ifmgd->csa_connection_drop_work);
 		goto out;
 	}
 
-	if (!local->use_chanctx) {
-		local->_oper_chandef = sdata->csa_chandef;
-		/* Call "hw_config" only if doing sw channel switch.
-		 * Otherwise update the channel directly
-		 */
-		if (!local->ops->channel_switch)
-			ieee80211_hw_config(local, 0);
-		else
-			local->hw.conf.chandef = local->_oper_chandef;
-	}
-
 	/* 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? */
 	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;
 
@@ -992,6 +1010,8 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	ieee80211_sta_reset_conn_monitor(sdata);
 
 out:
+	mutex_unlock(&local->chanctx_mtx);
+	mutex_unlock(&local->mtx);
 	sdata_unlock(sdata);
 }
 
@@ -1028,6 +1048,7 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	struct cfg80211_bss *cbss = ifmgd->associated;
+	struct ieee80211_chanctx_conf *conf;
 	struct ieee80211_chanctx *chanctx;
 	enum ieee80211_band current_band;
 	struct ieee80211_csa_ie csa_ie;
@@ -1072,6 +1093,19 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
 
 	mutex_lock(&local->chanctx_mtx);
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf) {
+		sdata_info(sdata,
+			   "no channel context assigned to vif?, disconnecting\n");
+		ieee80211_queue_work(&local->hw,
+				     &ifmgd->csa_connection_drop_work);
+		mutex_unlock(&local->chanctx_mtx);
+		return;
+	}
+
+	chanctx = container_of(conf, struct ieee80211_chanctx, conf);
+
 	if (local->use_chanctx) {
 		u32 num_chanctx = 0;
 		list_for_each_entry(chanctx, &local->chanctx_list, list)
@@ -1088,17 +1122,12 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	if (WARN_ON(!rcu_access_pointer(sdata->vif.chanctx_conf))) {
-		ieee80211_queue_work(&local->hw,
-				     &ifmgd->csa_connection_drop_work);
-		mutex_unlock(&local->chanctx_mtx);
-		return;
-	}
-	chanctx = container_of(rcu_access_pointer(sdata->vif.chanctx_conf),
-			       struct ieee80211_chanctx, conf);
-	if (ieee80211_chanctx_refcount(local, chanctx) > 1) {
+	res = ieee80211_vif_reserve_chanctx(sdata, &csa_ie.chandef,
+					    chanctx->mode, false);
+	if (res) {
 		sdata_info(sdata,
-			   "channel switch with multiple interfaces on the same channel, disconnecting\n");
+			   "failed to reserve channel context for channel switch, disconnecting (err=%d)\n",
+			   res);
 		ieee80211_queue_work(&local->hw,
 				     &ifmgd->csa_connection_drop_work);
 		mutex_unlock(&local->chanctx_mtx);
@@ -1106,10 +1135,9 @@ 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_chandef = csa_ie.chandef;
 	sdata->csa_block_tx = csa_ie.mode;
 
 	if (sdata->csa_block_tx)
-- 
1.8.5.3

[PATCH v6 6/6] cfg80211: remove channel_switch combination check

[Michal Kazior]

Make the driver responsible for making sure it is
capable of performing the switch. It might as well
accept a request but then disconnect an interface
if some requirements are not met.

In that case userspace should be prepared for an
appropriate event (AP/IBSS/mesh being stopped/left).

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 net/wireless/nl80211.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 49adf58..d290afd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6001,17 +6001,6 @@ skip_beacons:
 		params.radar_required = true;
 	}
 
-	/* TODO: I left this here for now.  With channel switch, the
-	 * verification is a bit more complicated, because we only do
-	 * it later when the channel switch really happens.
-	 */
-	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-					   params.chandef.chan,
-					   CHAN_MODE_SHARED,
-					   radar_detect_width);
-	if (err)
-		return err;
-
 	if (info->attrs[NL80211_ATTR_CH_SWITCH_BLOCK_TX])
 		params.block_tx = true;
 
-- 
1.8.5.3

Re: [PATCH v6 6/6] cfg80211: remove channel_switch combination check

[Johannes Berg]

On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
> Make the driver responsible for making sure it is
> capable of performing the switch. It might as well
> accept a request but then disconnect an interface
> if some requirements are not met.

Care to elaborate? I'd really like to avoid this case as much as
possible, so just mentioning here that it would be valid seems like a
bad idea.

johannes

Re: [PATCH v6 6/6] cfg80211: remove channel_switch combination check

[Michal Kazior]

On 22 May 2014 16:57, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
>> Make the driver responsible for making sure it is
>> capable of performing the switch. It might as well
>> accept a request but then disconnect an interface
>> if some requirements are not met.
>
> Care to elaborate? I'd really like to avoid this case as much as
> possible, so just mentioning here that it would be valid seems like a
> bad idea.

Well, CSA isn't really visible to cfg80211 so you can't enforce anything now.

Also since CSA requests are submitted one-by-one you already break
interface combinations and hope:
 a) userspace sends more CSA requests soon enough to make future
interface combination valid
 b) trust drivers deal with it either way

So why bother?

The most you can probably do is to prevent CSA requests from switching
to too many different channels but you can easily guarantee this in a
driver.


Michal

Re: [PATCH v6 6/6] cfg80211: remove channel_switch combination check

[Johannes Berg]

On Fri, 2014-05-23 at 09:04 +0200, Michal Kazior wrote:
> On 22 May 2014 16:57, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
> >> Make the driver responsible for making sure it is
> >> capable of performing the switch. It might as well
> >> accept a request but then disconnect an interface
> >> if some requirements are not met.
> >
> > Care to elaborate? I'd really like to avoid this case as much as
> > possible, so just mentioning here that it would be valid seems like a
> > bad idea.
> 
> Well, CSA isn't really visible to cfg80211 so you can't enforce anything now.
> 
> Also since CSA requests are submitted one-by-one you already break
> interface combinations and hope:
>  a) userspace sends more CSA requests soon enough to make future
> interface combination valid
>  b) trust drivers deal with it either way
> 
> So why bother?
> 
> The most you can probably do is to prevent CSA requests from switching
> to too many different channels but you can easily guarantee this in a
> driver.

Yeah, absolutely - it's just the fact that you're saying that it might
accept but then disconnect ... that will make people feel OK about that,
when it's really not all that desirable. Better to state that more
explicitly that it should check before :)

johannes

Re: [PATCH v6 6/6] cfg80211: remove channel_switch combination check

[Michal Kazior]

On 23 May 2014 10:53, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2014-05-23 at 09:04 +0200, Michal Kazior wrote:
>> On 22 May 2014 16:57, Johannes Berg <johannes@sipsolutions.net> wrote:
>> > On Thu, 2014-05-22 at 16:07 +0200, Michal Kazior wrote:
>> >> Make the driver responsible for making sure it is
>> >> capable of performing the switch. It might as well
>> >> accept a request but then disconnect an interface
>> >> if some requirements are not met.
>> >
>> > Care to elaborate? I'd really like to avoid this case as much as
>> > possible, so just mentioning here that it would be valid seems like a
>> > bad idea.
>>
>> Well, CSA isn't really visible to cfg80211 so you can't enforce anything now.
>>
>> Also since CSA requests are submitted one-by-one you already break
>> interface combinations and hope:
>>  a) userspace sends more CSA requests soon enough to make future
>> interface combination valid
>>  b) trust drivers deal with it either way
>>
>> So why bother?
>>
>> The most you can probably do is to prevent CSA requests from switching
>> to too many different channels but you can easily guarantee this in a
>> driver.
>
> Yeah, absolutely - it's just the fact that you're saying that it might
> accept but then disconnect ... that will make people feel OK about that,
> when it's really not all that desirable. Better to state that more
> explicitly that it should check before :)

I understand now. I'll try to emphasize this when I respin.


Michał