[PATCH v7 0/4] mac802111: channel context reservation

[PATCH v7 0/4] mac802111: channel context reservation

[Luciano Coelho]

Hi,

New version with some comments from an offline review with Johannes
plus a few comments by Michal and Eliad.  I removed the history from
here, since it's all in the patches anyway.

There's a new patch here to combine the assign and unassign functions
to avoid having problems with a NULL conf while the vif is still
running.

--
Luca.

Luciano Coelho (4):
  mac80211: split ieee80211_vif_change_channel in two
  mac80211: refactor ieee80211_assign/unassign_vif_chanctx into one
  mac80211: implement chanctx reservation
  mac80211: allow reservation of a running chanctx

 include/net/mac80211.h     |   7 ++
 net/mac80211/chan.c        | 298 +++++++++++++++++++++++++++++++++++----------
 net/mac80211/ieee80211_i.h |  13 ++
 3 files changed, 256 insertions(+), 62 deletions(-)

-- 
1.9.0

[PATCH v7 1/4] mac80211: split ieee80211_vif_change_channel in two

[Luciano Coelho]

ieee80211_vif_change_channel() locks chanctx_mtx.  When implementing
channel reservation for CS, we will need to call the function to
change channel when the lock is already held, so split the part that
requires the lock out and leave the locking in the original function.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 net/mac80211/chan.c | 58 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index fdbb932..ef77703 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -547,39 +547,20 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 	return ret;
 }
 
-int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
-				 u32 *changed)
+static int __ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
+					  struct ieee80211_chanctx *ctx,
+					  u32 *changed)
 {
 	struct ieee80211_local *local = sdata->local;
-	struct ieee80211_chanctx_conf *conf;
-	struct ieee80211_chanctx *ctx;
 	const struct cfg80211_chan_def *chandef = &sdata->csa_chandef;
-	int ret;
 	u32 chanctx_changed = 0;
 
-	lockdep_assert_held(&local->mtx);
-
-	/* should never be called if not performing a channel switch. */
-	if (WARN_ON(!sdata->vif.csa_active))
-		return -EINVAL;
-
 	if (!cfg80211_chandef_usable(sdata->local->hw.wiphy, chandef,
 				     IEEE80211_CHAN_DISABLED))
 		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);
-	if (ctx->refcount != 1) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (ctx->refcount != 1)
+		return -EINVAL;
 
 	if (sdata->vif.bss_conf.chandef.width != chandef->width) {
 		chanctx_changed = IEEE80211_CHANCTX_CHANGE_WIDTH;
@@ -597,7 +578,34 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
 	ieee80211_recalc_radar_chanctx(local, ctx);
 	ieee80211_recalc_chanctx_min_def(local, ctx);
 
-	ret = 0;
+	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;
-- 
1.9.0

[PATCH v7 2/4] mac80211: refactor ieee80211_assign/unassign_vif_chanctx into one

[Luciano Coelho]

Combine the functions into one, so that we can switch from one context
to the other without having to unassign and assign separately.  This
is needed by the channel reservation functionality because otherwise
we have a small period of time when the chanctx is set to NULL, which
can cause problems if someone else is trying to dereference it.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 net/mac80211/chan.c | 85 +++++++++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 42 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index ef77703..5fa12f1 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -294,32 +294,6 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
 	ieee80211_recalc_idle(local);
 }
 
-static int ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
-					struct ieee80211_chanctx *ctx)
-{
-	struct ieee80211_local *local = sdata->local;
-	int ret;
-
-	lockdep_assert_held(&local->chanctx_mtx);
-
-	ret = drv_assign_vif_chanctx(local, sdata, ctx);
-	if (ret)
-		return ret;
-
-	rcu_assign_pointer(sdata->vif.chanctx_conf, &ctx->conf);
-	ctx->refcount++;
-
-	ieee80211_recalc_txpower(sdata);
-	ieee80211_recalc_chanctx_min_def(local, ctx);
-	sdata->vif.bss_conf.idle = false;
-
-	if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE &&
-	    sdata->vif.type != NL80211_IFTYPE_MONITOR)
-		ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_IDLE);
-
-	return 0;
-}
-
 static void ieee80211_recalc_chanctx_chantype(struct ieee80211_local *local,
 					      struct ieee80211_chanctx *ctx)
 {
@@ -378,30 +352,57 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
 	drv_change_chanctx(local, chanctx, IEEE80211_CHANCTX_CHANGE_RADAR);
 }
 
-static void ieee80211_unassign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
-					   struct ieee80211_chanctx *ctx)
+static int ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
+					struct ieee80211_chanctx *new_ctx)
 {
 	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx_conf *conf;
+	struct ieee80211_chanctx *curr_ctx = NULL;
+	int ret = 0;
 
-	lockdep_assert_held(&local->chanctx_mtx);
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
 
-	ctx->refcount--;
-	rcu_assign_pointer(sdata->vif.chanctx_conf, NULL);
+	if (conf) {
+		curr_ctx = container_of(conf, struct ieee80211_chanctx, conf);
 
-	sdata->vif.bss_conf.idle = true;
+		curr_ctx->refcount--;
+		drv_unassign_vif_chanctx(local, sdata, curr_ctx);
+		conf = NULL;
+	}
 
-	if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE &&
-	    sdata->vif.type != NL80211_IFTYPE_MONITOR)
-		ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_IDLE);
+	if (new_ctx) {
+		ret = drv_assign_vif_chanctx(local, sdata, new_ctx);
+		if (ret)
+			goto out;
+
+		new_ctx->refcount++;
+		conf = &new_ctx->conf;
+	}
 
-	drv_unassign_vif_chanctx(local, sdata, ctx);
+out:
+	rcu_assign_pointer(sdata->vif.chanctx_conf, conf);
+
+	sdata->vif.bss_conf.idle = !conf;
 
-	if (ctx->refcount > 0) {
-		ieee80211_recalc_chanctx_chantype(sdata->local, ctx);
-		ieee80211_recalc_smps_chanctx(local, ctx);
-		ieee80211_recalc_radar_chanctx(local, ctx);
-		ieee80211_recalc_chanctx_min_def(local, ctx);
+	if (curr_ctx && curr_ctx->refcount > 0) {
+		ieee80211_recalc_chanctx_chantype(local, curr_ctx);
+		ieee80211_recalc_smps_chanctx(local, curr_ctx);
+		ieee80211_recalc_radar_chanctx(local, curr_ctx);
+		ieee80211_recalc_chanctx_min_def(local, curr_ctx);
 	}
+
+	if (new_ctx && new_ctx->refcount > 0) {
+		ieee80211_recalc_txpower(sdata);
+		ieee80211_recalc_chanctx_min_def(local, new_ctx);
+	}
+
+	if (sdata->vif.type != NL80211_IFTYPE_P2P_DEVICE &&
+	    sdata->vif.type != NL80211_IFTYPE_MONITOR)
+		ieee80211_bss_info_change_notify(sdata,
+						 BSS_CHANGED_IDLE);
+
+	return ret;
 }
 
 static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
@@ -419,7 +420,7 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
 
 	ctx = container_of(conf, struct ieee80211_chanctx, conf);
 
-	ieee80211_unassign_vif_chanctx(sdata, ctx);
+	ieee80211_assign_vif_chanctx(sdata, NULL);
 	if (ctx->refcount == 0)
 		ieee80211_free_chanctx(local, ctx);
 }
-- 
1.9.0

[PATCH v7 3/4] mac80211: implement chanctx reservation

[Luciano Coelho]

In order to support channel switch with multiple vifs and multiple
contexts, we implement a concept of channel context reservation.  This
allows us to reserve a channel context to be used later.

The reservation functionality is not tied directly to channel switch
and may be used in other situations (eg. reserving a channel context
during IBSS join).

We first check if an existing compatible context exists and if it
does, we reserve it.  If there is no compatible context we create a
new one and reserve it.

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

   * reformulated the patch to make channel reservation generic and
     not only for channel switch, as we may find other uses for it in
     the future;
   * added ieee80211_vif_unreserve_chanctx();
   * removed reservation of the current chanctx from this patch;
   * added a stop_queue_reason for chanctx reservation;
   * added a few TODOs according to Michal's comments to be handled in
     future patch series;
   * set sdata->csa_chandef to the received chandef in all cases (it
     was only happening in the alone-in-the-vif case;

In v3:

   * removed stray empty line;
   * reworded the commit message;
   * added reserved_chandef;
   * added comment about reserved_chanctx and reserved_chandef protection;
   * wake the queues if ieee80211_assign_vif_chanctx() fails;

In v4:

   * add chanctx mode parameter to ieee80211_vif_reserve_chanctx();
   * compare the vif's previous BSS width with the reserved new width
     instead of comparing it to the chanctx's combined width;

In v5:

   * removed a comment that was being removed, mistakenly, on the next
     patch;

In v7:

   * renamed local_changed variable to tmp_changed to avoid confusion;
   * call ieee80211_recalc_chanctx_min_def() in
     ieee80211_vif_use_reserved_chanctx();
   * use new combined ieee80211_assign_vif_chanctx() when using
     reserved channel;
   * don't stop queues before switching context;
   * call ieee80211_vif_copy_chanctx_to_vlans() in use_reserved();
---
 net/mac80211/chan.c        | 140 +++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  13 +++++
 2 files changed, 153 insertions(+)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 5fa12f1..9a4b0fb 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -420,6 +420,9 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
 
 	ctx = container_of(conf, struct ieee80211_chanctx, conf);
 
+	if (sdata->reserved_chanctx)
+		ieee80211_vif_unreserve_chanctx(sdata);
+
 	ieee80211_assign_vif_chanctx(sdata, NULL);
 	if (ctx->refcount == 0)
 		ieee80211_free_chanctx(local, ctx);
@@ -612,6 +615,143 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
 	return ret;
 }
 
+int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata)
+{
+	lockdep_assert_held(&sdata->local->chanctx_mtx);
+
+	if (WARN_ON(!sdata->reserved_chanctx))
+		return -EINVAL;
+
+	if (--sdata->reserved_chanctx->refcount == 0)
+		ieee80211_free_chanctx(sdata->local, sdata->reserved_chanctx);
+
+	sdata->reserved_chanctx = NULL;
+
+	return 0;
+}
+
+int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
+				  const struct cfg80211_chan_def *chandef,
+				  enum ieee80211_chanctx_mode mode)
+{
+	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);
+
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	curr_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+	/* try to find another context with the chandef we want */
+	new_ctx = ieee80211_find_chanctx(local, chandef, mode);
+	if (!new_ctx) {
+		/* create a new context */
+		new_ctx = ieee80211_new_chanctx(local, chandef, mode);
+		if (IS_ERR(new_ctx)) {
+			ret = PTR_ERR(new_ctx);
+			goto out;
+		}
+	}
+
+	sdata->reserved_chanctx = new_ctx;
+	new_ctx->refcount++;
+	sdata->reserved_chandef = *chandef;
+out:
+	mutex_unlock(&local->chanctx_mtx);
+	return ret;
+}
+
+int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
+				       u32 *changed)
+{
+	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;
+
+	/* TODO: need to recheck if the chandef is usable etc.? */
+
+	lockdep_assert_held(&local->mtx);
+
+	mutex_lock(&local->chanctx_mtx);
+
+	ctx = sdata->reserved_chanctx;
+	if (WARN_ON(!ctx)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+	if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
+		tmp_changed |= BSS_CHANGED_BANDWIDTH;
+
+	sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+
+	/* unref our reservation before assigning */
+	ctx->refcount--;
+	sdata->reserved_chanctx = NULL;
+
+	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;
+	} else {
+		if (sdata->vif.bss_conf.chandef.width !=
+		    sdata->reserved_chandef.width)
+			tmp_changed |= BSS_CHANGED_BANDWIDTH;
+
+		sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+
+		ret = ieee80211_assign_vif_chanctx(sdata, ctx);
+		if (old_ctx->refcount == 0)
+			ieee80211_free_chanctx(local, old_ctx);
+		if (ret) {
+			/* if assign fails refcount stays the same */
+			if (ctx->refcount == 0)
+				ieee80211_free_chanctx(local, ctx);
+			goto out;
+		}
+
+		*changed = tmp_changed;
+
+		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;
+}
+
 int ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
 				   const struct cfg80211_chan_def *chandef,
 				   u32 *changed)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2f662ca..6949099 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -756,6 +756,10 @@ struct ieee80211_sub_if_data {
 	bool csa_radar_required;
 	struct cfg80211_chan_def csa_chandef;
 
+	/* context reservation -- protected with chanctx_mtx */
+	struct ieee80211_chanctx *reserved_chanctx;
+	struct cfg80211_chan_def reserved_chandef;
+
 	/* used to reconfigure hardware SM PS */
 	struct work_struct recalc_smps;
 
@@ -1776,6 +1780,15 @@ ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 			  const struct cfg80211_chan_def *chandef,
 			  enum ieee80211_chanctx_mode mode);
 int __must_check
+ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
+			      const struct cfg80211_chan_def *chandef,
+			      enum ieee80211_chanctx_mode mode);
+int __must_check
+ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
+				   u32 *changed);
+int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata);
+
+int __must_check
 ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
 			       const struct cfg80211_chan_def *chandef,
 			       u32 *changed);
-- 
1.9.0

Re: [PATCH v7 3/4] mac80211: implement chanctx reservation

[Michal Kazior]

On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:

[...]

> +       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;
> +       } else {
> +               if (sdata->vif.bss_conf.chandef.width !=
> +                   sdata->reserved_chandef.width)
> +                       tmp_changed |= BSS_CHANGED_BANDWIDTH;
> +
> +               sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> +
> +               ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> +               if (old_ctx->refcount == 0)
> +                       ieee80211_free_chanctx(local, old_ctx);
> +               if (ret) {
> +                       /* if assign fails refcount stays the same */
> +                       if (ctx->refcount == 0)
> +                               ieee80211_free_chanctx(local, ctx);
> +                       goto out;
> +               }
> +
> +               *changed = tmp_changed;
> +
> +               ieee80211_vif_copy_chanctx_to_vlans(sdata, false);

Ah, you can't call it like that. This will deadlock because the
function tries to grab chanctx_mtx but you're already holding it. You
need to first split the vif_copy_chanctx() into 2 variants - with and
without grabbing the chanctx_mtx.


> +               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);

It seems you need to call these recalc* regardless what if() branch is
taken so calling them in this branch is pointless, no?

> +       }
> +
> +       *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);



Michał

Re: [PATCH v7 3/4] mac80211: implement chanctx reservation

[Luca Coelho]

On Wed, 2014-03-12 at 14:21 +0100, Michal Kazior wrote:
> On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:
> 
> [...]
> 
> > +       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;
> > +       } else {
> > +               if (sdata->vif.bss_conf.chandef.width !=
> > +                   sdata->reserved_chandef.width)
> > +                       tmp_changed |= BSS_CHANGED_BANDWIDTH;
> > +
> > +               sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> > +
> > +               ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > +               if (old_ctx->refcount == 0)
> > +                       ieee80211_free_chanctx(local, old_ctx);
> > +               if (ret) {
> > +                       /* if assign fails refcount stays the same */
> > +                       if (ctx->refcount == 0)
> > +                               ieee80211_free_chanctx(local, ctx);
> > +                       goto out;
> > +               }
> > +
> > +               *changed = tmp_changed;
> > +
> > +               ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
> 
> Ah, you can't call it like that. This will deadlock because the
> function tries to grab chanctx_mtx but you're already holding it. You
> need to first split the vif_copy_chanctx() into 2 variants - with and
> without grabbing the chanctx_mtx.

Grrrr! Concentrate, Luca, concentrate!


> > +               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);
> 
> It seems you need to call these recalc* regardless what if() branch is
> taken so calling them in this branch is pointless, no?
> 
> > +       }
> > +
> > +       *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);

More stuff I screwed up in my rebases...

--
Luca.

Re: [PATCH v7 3/4] mac80211: implement chanctx reservation

[Michal Kazior]

On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:

[...]

> +       if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
> +               tmp_changed |= BSS_CHANGED_BANDWIDTH;
> +
> +       sdata->vif.bss_conf.chandef = sdata->reserved_chandef;

> +
> +       /* unref our reservation before assigning */
> +       ctx->refcount--;
> +       sdata->reserved_chanctx = NULL;
> +
> +       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;
> +       } else {

> +               if (sdata->vif.bss_conf.chandef.width !=
> +                   sdata->reserved_chandef.width)
> +                       tmp_changed |= BSS_CHANGED_BANDWIDTH;
> +
> +               sdata->vif.bss_conf.chandef = sdata->reserved_chandef;

You're already doing this a few lines earlier, before branching.


Michał

Re: [PATCH v7 3/4] mac80211: implement chanctx reservation

[Luca Coelho]

On Wed, 2014-03-12 at 14:26 +0100, Michal Kazior wrote:
> On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:
> 
> [...]
> 
> > +       if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
> > +               tmp_changed |= BSS_CHANGED_BANDWIDTH;
> > +
> > +       sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> 
> > +
> > +       /* unref our reservation before assigning */
> > +       ctx->refcount--;
> > +       sdata->reserved_chanctx = NULL;
> > +
> > +       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;
> > +       } else {
> 
> > +               if (sdata->vif.bss_conf.chandef.width !=
> > +                   sdata->reserved_chandef.width)
> > +                       tmp_changed |= BSS_CHANGED_BANDWIDTH;
> > +
> > +               sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> 
> You're already doing this a few lines earlier, before branching.

Yeah... too many rebases and I'm not concentrating enough... *sigh*.

[PATCH v7 4/4] mac80211: allow reservation of a running chanctx

[Luciano Coelho]

With single-channel drivers, we need to be able to change a running
chanctx if we want to use chanctx reservation.  Not all drivers may be
able to do this, so add a flag that indicates support for it.

Changing a running chanctx can also be used as an optimization in
multi-channel drivers when the context needs to be reserved for future
usage.

Introduce IEEE80211_CHANCTX_RESERVED chanctx mode to mark a channel as
reserved so nobody else can use it (since we know it's going to
change).  In the future, we may allow several vifs to use the same
reservation as long as they plan to use the chanctx on the same
future channel.

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

   * reworded the TODO, slightly;

In v4:

   * remove IEEE80211_CHANCTX_RESERVED and the reserved_mode element;
   * increase refcount also for "in-place" changes;
   * stop queues also before doing an "in-place" change;
   * refactor ieee80211_use_reserved_chanctx() a bit to fit "in-place"
     better;

In v5:

   * fix checkpatch warnings;

In v6:

   * return ret in ieee80211_chanctx_is_reserved();
   * check if sdata is running in ieee80211_chanctx_is_reserved();
   * keep check for IEEE80211_CHANCTX_EXCLUSIVE in
     ieee80211_find_channel();
   * use break instead of goto out in ieee80211_chanctx_is_reserved();
---
 include/net/mac80211.h |  7 ++++++
 net/mac80211/chan.c    | 59 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 86faa41..b35c608 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1553,6 +1553,12 @@ 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,
@@ -1584,6 +1590,7 @@ 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 9a4b0fb..c8b25cb 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -162,6 +162,27 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
 	}
 }
 
+static bool ieee80211_chanctx_is_reserved(struct ieee80211_local *local,
+				     struct ieee80211_chanctx *ctx)
+{
+	struct ieee80211_sub_if_data *sdata;
+	bool ret = false;
+
+	lockdep_assert_held(&local->chanctx_mtx);
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+		if (sdata->reserved_chanctx == ctx) {
+			ret = true;
+			break;
+		}
+	}
+
+	rcu_read_unlock();
+	return ret;
+}
+
 static struct ieee80211_chanctx *
 ieee80211_find_chanctx(struct ieee80211_local *local,
 		       const struct cfg80211_chan_def *chandef,
@@ -177,7 +198,12 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
 	list_for_each_entry(ctx, &local->chanctx_list, list) {
 		const struct cfg80211_chan_def *compat;
 
-		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
+		/* We don't support chanctx reservation for multiple
+		 * vifs yet, so don't allow reserved chanctxs to be
+		 * reused.
+		 */
+		if ((ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) ||
+		    ieee80211_chanctx_is_reserved(local, ctx))
 			continue;
 
 		compat = cfg80211_chandef_compatible(&ctx->conf.def, chandef);
@@ -653,11 +679,20 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
 	/* try to find another context with the chandef we want */
 	new_ctx = ieee80211_find_chanctx(local, chandef, mode);
 	if (!new_ctx) {
-		/* create a new context */
-		new_ctx = ieee80211_new_chanctx(local, chandef, mode);
-		if (IS_ERR(new_ctx)) {
-			ret = PTR_ERR(new_ctx);
-			goto out;
+		if (curr_ctx->refcount == 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 {
+			/* create a new context and reserve it */
+			new_ctx = ieee80211_new_chanctx(local, chandef, mode);
+			if (IS_ERR(new_ctx)) {
+				ret = PTR_ERR(new_ctx);
+				goto out;
+			}
 		}
 	}
 
@@ -700,12 +735,7 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 
 	old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
 
-	if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
-		tmp_changed |= BSS_CHANGED_BANDWIDTH;
-
-	sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
-
-	/* unref our reservation before assigning */
+	/* unref our reservation */
 	ctx->refcount--;
 	sdata->reserved_chanctx = NULL;
 
@@ -742,11 +772,6 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
 	}
 
 	*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;
-- 
1.9.0

Re: [PATCH v7 4/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:

[...]

> @@ -700,12 +735,7 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
>
>         old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
>
> -       if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
> -               tmp_changed |= BSS_CHANGED_BANDWIDTH;
> -
> -       sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> -
> -       /* unref our reservation before assigning */
> +       /* unref our reservation */
>         ctx->refcount--;
>         sdata->reserved_chanctx = NULL;
>
> @@ -742,11 +772,6 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
>         }
>
>         *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;

I think you need these for if(old_ctx==new_ctx) case, don't you?


Michał

Re: [PATCH v7 4/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

On Wed, 2014-03-12 at 14:24 +0100, Michal Kazior wrote:
> On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:
> 
> [...]
> 
> > @@ -700,12 +735,7 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
> >
> >         old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
> >
> > -       if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
> > -               tmp_changed |= BSS_CHANGED_BANDWIDTH;
> > -
> > -       sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> > -
> > -       /* unref our reservation before assigning */
> > +       /* unref our reservation */
> >         ctx->refcount--;
> >         sdata->reserved_chanctx = NULL;
> >
> > @@ -742,11 +772,6 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
> >         }
> >
> >         *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;
> 
> I think you need these for if(old_ctx==new_ctx) case, don't you?

Rebase screw-ups ad-infinitum...

--
Luca.