| Message ID | 1393512081-31453-3-git-send-email-luca@coelho.fi (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote: > 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). > > When reserving the context, the following algorithm is used: > > 1) try to find an existing context that matches our future chandef and > reserve it if it exists; > 2) otherwise, check if we're the only vif in the current context, in > which case we can just change our current context. To prevent > other vifs from joining this context in the meantime, we mark it as > exclusive. This is an optimization to avoid using extra contexts > when not necessary and requires the driver to support changing a > context on-the-fly; This commit part is out of date (you've moved this into a separate patch). > @@ -611,6 +614,142 @@ 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); Empty line :-) > + if (sdata->vif.bss_conf.chandef.width != ctx->conf.def.width) > + local_changed |= BSS_CHANGED_BANDWIDTH; > + > + sdata->vif.bss_conf.chandef = ctx->conf.def; I think you should be simply using sdata->csa_chandef here. What's the point of setting the csa_chandef during reservation otherwise? csa_chandef should probably be renamed to reserved_chandef for consistency. > + > + /* 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; > + } > + > + ieee80211_wake_queues_by_reason(&sdata->local->hw, > + IEEE80211_MAX_QUEUE_MAP, > + IEEE80211_QUEUE_STOP_REASON_CHANCTX); If you fail to assign chanctx you don't wake queues back. Is this intended? > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h > index 8603dfb..998fbbb 100644 > --- a/net/mac80211/ieee80211_i.h > +++ b/net/mac80211/ieee80211_i.h > @@ -756,6 +756,8 @@ struct ieee80211_sub_if_data { > bool csa_radar_required; > struct cfg80211_chan_def csa_chandef; > > + struct ieee80211_chanctx *reserved_chanctx; > + It's a good idea to document this is protected by chanctx_mtx (and not wdev.mtx). 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
On Thu, 2014-02-27 at 16:16 +0100, Michal Kazior wrote: > On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote: > > 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). > > > > When reserving the context, the following algorithm is used: > > > > 1) try to find an existing context that matches our future chandef and > > reserve it if it exists; > > > 2) otherwise, check if we're the only vif in the current context, in > > which case we can just change our current context. To prevent > > other vifs from joining this context in the meantime, we mark it as > > exclusive. This is an optimization to avoid using extra contexts > > when not necessary and requires the driver to support changing a > > context on-the-fly; > > This commit part is out of date (you've moved this into a separate patch). Good point, I'll fix this part. > > > @@ -611,6 +614,142 @@ 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); > > Empty line :-) Oops! > > + if (sdata->vif.bss_conf.chandef.width != ctx->conf.def.width) > > + local_changed |= BSS_CHANGED_BANDWIDTH; > > + > > + sdata->vif.bss_conf.chandef = ctx->conf.def; > > I think you should be simply using sdata->csa_chandef here. What's the > point of setting the csa_chandef during reservation otherwise? Yes, this is wrong. > csa_chandef should probably be renamed to reserved_chandef for consistency. I don't want to mess with CSA in this patch. I'll leave the csa_chandef as it is. But you're right, I should use the "future" chandef, which I'll add as 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; > > + } > > + > > + ieee80211_wake_queues_by_reason(&sdata->local->hw, > > + IEEE80211_MAX_QUEUE_MAP, > > + IEEE80211_QUEUE_STOP_REASON_CHANCTX); > > If you fail to assign chanctx you don't wake queues back. Is this intended? This was not intended, it was a mistake. :) I'll fix it. > > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h > > index 8603dfb..998fbbb 100644 > > --- a/net/mac80211/ieee80211_i.h > > +++ b/net/mac80211/ieee80211_i.h > > @@ -756,6 +756,8 @@ struct ieee80211_sub_if_data { > > bool csa_radar_required; > > struct cfg80211_chan_def csa_chandef; > > > > + struct ieee80211_chanctx *reserved_chanctx; > > + > > It's a good idea to document this is protected by chanctx_mtx (and not > wdev.mtx). Good idea. -- 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
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index a27c6ec..bd42d17 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,142 @@ 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) +{ + 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, + IEEE80211_CHANCTX_SHARED); + if (!new_ctx) { + /* create a new context */ + new_ctx = ieee80211_new_chanctx(local, chandef, + IEEE80211_CHANCTX_SHARED); + 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->csa_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); + + /* TODO: we're assuming that the bandwidth of the context + * changes here, but in fact, it will only change if the + * combination of the channels used in this context change. + * We should set this flag according to what happens when + * ieee80211_recalc_chanctx_chantype() is called. Maybe the + * nicest thing to do would be to change that function so that + * it returns changed flags (which will be either 0 or + * BSS_CHANGED_BANDWIDTH). + */ + + if (sdata->vif.bss_conf.chandef.width != ctx->conf.def.width) + local_changed |= BSS_CHANGED_BANDWIDTH; + + sdata->vif.bss_conf.chandef = ctx->conf.def; + + /* 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; + } + + ieee80211_wake_queues_by_reason(&sdata->local->hw, + IEEE80211_MAX_QUEUE_MAP, + IEEE80211_QUEUE_STOP_REASON_CHANCTX); + + *changed = local_changed; + + ieee80211_recalc_chanctx_chantype(local, ctx); + ieee80211_recalc_smps_chanctx(local, ctx); + ieee80211_recalc_radar_chanctx(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 8603dfb..998fbbb 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -756,6 +756,8 @@ struct ieee80211_sub_if_data { bool csa_radar_required; struct cfg80211_chan_def csa_chandef; + struct ieee80211_chanctx *reserved_chanctx; + /* used to reconfigure hardware SM PS */ struct work_struct recalc_smps; @@ -899,6 +901,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 +1779,14 @@ 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); +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);