diff mbox

[v5,2/3] mac80211: implement chanctx reservation

Message ID 1394017904-4012-3-git-send-email-luca@coelho.fi (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Luca Coelho March 5, 2014, 11:11 a.m. UTC 
From: Luciano Coelho <luciano.coelho@intel.com>

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;
---
 net/mac80211/chan.c        | 127 +++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  14 +++++
 2 files changed, 141 insertions(+)

Comments

Michal Kazior March 5, 2014, 12:04 p.m. UTC | #1 
On 5 March 2014 12:11, Luca Coelho <luca@coelho.fi> wrote:

[...]

> +       /* unref our reservation before assigning */
> +       ctx->refcount--;
> +       sdata->reserved_chanctx = NULL;
> +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> +       if (ret) {
> +               /* if assign fails refcount stays the same */
> +               if (ctx->refcount == 0)
> +                       ieee80211_free_chanctx(local, ctx);
> +               goto out_wake;
> +       }

This actually won't work if there are AP VLANs (I noticed that yesterday).

You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
current locking requirements. I have a patch for that though ;-) I'll
send it soon.


Micha?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luca Coelho March 7, 2014, 6:48 a.m. UTC | #2 
On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
> On 5 March 2014 12:11, Luca Coelho <luca@coelho.fi> wrote:
> 
> [...]
> 
> > +       /* unref our reservation before assigning */
> > +       ctx->refcount--;
> > +       sdata->reserved_chanctx = NULL;
> > +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > +       if (ret) {
> > +               /* if assign fails refcount stays the same */
> > +               if (ctx->refcount == 0)
> > +                       ieee80211_free_chanctx(local, ctx);
> > +               goto out_wake;
> > +       }
> 
> This actually won't work if there are AP VLANs (I noticed that yesterday).
> 
> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
> current locking requirements. I have a patch for that though ;-) I'll
> send it soon.

How is this related to this patch?

Anyway, good to know that you have fixed it. :)

--
Luca.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior March 10, 2014, 7:03 a.m. UTC | #3 
On 7 March 2014 07:48, Luca Coelho <luca@coelho.fi> wrote:
> On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
>> On 5 March 2014 12:11, Luca Coelho <luca@coelho.fi> wrote:
>>
>> [...]
>>
>> > +       /* unref our reservation before assigning */
>> > +       ctx->refcount--;
>> > +       sdata->reserved_chanctx = NULL;
>> > +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
>> > +       if (ret) {
>> > +               /* if assign fails refcount stays the same */
>> > +               if (ctx->refcount == 0)
>> > +                       ieee80211_free_chanctx(local, ctx);
>> > +               goto out_wake;
>> > +       }
>>
>> This actually won't work if there are AP VLANs (I noticed that yesterday).
>>
>> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
>> current locking requirements. I have a patch for that though ;-) I'll
>> send it soon.
>
> How is this related to this patch?
>
> Anyway, good to know that you have fixed it. :)

If you re-assign a chanctx of an AP that has VLANs you leave VLANs
with the old chanctx pointer. Those pointers should be updated or else
you'll end up dereferencing an invalid pointer.


Micha?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luca Coelho March 10, 2014, 9:08 a.m. UTC | #4 
On Mon, 2014-03-10 at 08:03 +0100, Michal Kazior wrote:
> On 7 March 2014 07:48, Luca Coelho <luca@coelho.fi> wrote:
> > On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
> >> On 5 March 2014 12:11, Luca Coelho <luca@coelho.fi> wrote:
> >>
> >> [...]
> >>
> >> > +       /* unref our reservation before assigning */
> >> > +       ctx->refcount--;
> >> > +       sdata->reserved_chanctx = NULL;
> >> > +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> >> > +       if (ret) {
> >> > +               /* if assign fails refcount stays the same */
> >> > +               if (ctx->refcount == 0)
> >> > +                       ieee80211_free_chanctx(local, ctx);
> >> > +               goto out_wake;
> >> > +       }
> >>
> >> This actually won't work if there are AP VLANs (I noticed that yesterday).
> >>
> >> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
> >> current locking requirements. I have a patch for that though ;-) I'll
> >> send it soon.
> >
> > How is this related to this patch?
> >
> > Anyway, good to know that you have fixed it. :)
> 
> If you re-assign a chanctx of an AP that has VLANs you leave VLANs
> with the old chanctx pointer. Those pointers should be updated or else
> you'll end up dereferencing an invalid pointer.

I see.  So, I need to call ieee80211_vif_copy_chanctx_to_vlans() here,
but to do that I need your patch that allows it to be called without the
RTNL.

So, now this series will depend on that patch, I'll talk to Johannes.

--
Luca.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Kazior March 10, 2014, 9:18 a.m. UTC | #5 
On 10 March 2014 10:08, Luca Coelho <lrothc@gmail.com> wrote:
> On Mon, 2014-03-10 at 08:03 +0100, Michal Kazior wrote:
>> On 7 March 2014 07:48, Luca Coelho <luca@coelho.fi> wrote:
>> > On Wed, 2014-03-05 at 13:04 +0100, Michal Kazior wrote:
>> >> On 5 March 2014 12:11, Luca Coelho <luca@coelho.fi> wrote:
>> >>
>> >> [...]
>> >>
>> >> > +       /* unref our reservation before assigning */
>> >> > +       ctx->refcount--;
>> >> > +       sdata->reserved_chanctx = NULL;
>> >> > +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
>> >> > +       if (ret) {
>> >> > +               /* if assign fails refcount stays the same */
>> >> > +               if (ctx->refcount == 0)
>> >> > +                       ieee80211_free_chanctx(local, ctx);
>> >> > +               goto out_wake;
>> >> > +       }
>> >>
>> >> This actually won't work if there are AP VLANs (I noticed that yesterday).
>> >>
>> >> You can't call ieee80211_vif_copy_chanctx_to_vlans() here due to
>> >> current locking requirements. I have a patch for that though ;-) I'll
>> >> send it soon.
>> >
>> > How is this related to this patch?
>> >
>> > Anyway, good to know that you have fixed it. :)
>>
>> If you re-assign a chanctx of an AP that has VLANs you leave VLANs
>> with the old chanctx pointer. Those pointers should be updated or else
>> you'll end up dereferencing an invalid pointer.
>
> I see.  So, I need to call ieee80211_vif_copy_chanctx_to_vlans() here,
> but to do that I need your patch that allows it to be called without the
> RTNL.
>
> So, now this series will depend on that patch, I'll talk to Johannes.

There's also another catch with having unassign-assign chanctx
swapping. There's a brief period of time where chanctx_mtx unprotected
code (think rcu based sections) access the chanctx_conf to get
band/sband. This ends up with bloody NULL dereferences if you have a
5GHz-only hw. I'm currently patching those up. This would be another
(although indirect) dependency for the patchset.

We might also want to have a ieee80211_vif_chanctx() wrapper that
would return either assigned chanctx or reserved chanctx before the
reservation patchset. Otherwise, even with those NULL dereferences
patched you risk WARN_ONs and other (logic) failures.


Micha?
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index a27c6ec..9dfdba5 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -419,6 +419,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_unassign_vif_chanctx(sdata, ctx);
 	if (ctx->refcount == 0)
 		ieee80211_free_chanctx(local, ctx);
@@ -611,6 +614,130 @@  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;
+		}
+	}
+
+	/* reserve the new or existing context */
+	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, local_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);
+
+	ieee80211_stop_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+
+	ieee80211_unassign_vif_chanctx(sdata, old_ctx);
+	if (old_ctx->refcount == 0)
+		ieee80211_free_chanctx(local, old_ctx);
+
+	if (sdata->vif.bss_conf.chandef.width != sdata->reserved_chandef.width)
+		local_changed |= BSS_CHANGED_BANDWIDTH;
+
+	sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
+
+	/* unref our reservation before assigning */
+	ctx->refcount--;
+	sdata->reserved_chanctx = NULL;
+	ret = ieee80211_assign_vif_chanctx(sdata, ctx);
+	if (ret) {
+		/* if assign fails refcount stays the same */
+		if (ctx->refcount == 0)
+			ieee80211_free_chanctx(local, ctx);
+		goto out_wake;
+	}
+
+	*changed = local_changed;
+
+	ieee80211_recalc_chanctx_chantype(local, ctx);
+	ieee80211_recalc_smps_chanctx(local, ctx);
+	ieee80211_recalc_radar_chanctx(local, ctx);
+out_wake:
+	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+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 8603dfb..ae8acc2 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;
 
@@ -899,6 +903,7 @@  enum queue_stop_reason {
 	IEEE80211_QUEUE_STOP_REASON_SKB_ADD,
 	IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL,
 	IEEE80211_QUEUE_STOP_REASON_FLUSH,
+	IEEE80211_QUEUE_STOP_REASON_CHANCTX,
 };
 
 #ifdef CONFIG_MAC80211_LEDS
@@ -1776,6 +1781,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);