On 5 March 2014 12:11, Luca Coelho <luca@xxxxxxxxx> wrote:
> From: Luciano Coelho <luciano.coelho@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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;
> ---
> include/net/mac80211.h | 7 ++++
> net/mac80211/chan.c | 97 +++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 75 insertions(+), 29 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 9dfdba5..d634f41 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -162,6 +162,26 @@ 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 (sdata->reserved_chanctx == ctx) {
> + ret = true;
> + goto out;
> + }
> + }
> +
> +out:
> + rcu_read_unlock();
> + return false;
`return ret` ;-)
It's probably a good idea to check ieee80211_sdata_running() before
even considering reserved_chanctx?
> +}
> +
> static struct ieee80211_chanctx *
> ieee80211_find_chanctx(struct ieee80211_local *local,
> const struct cfg80211_chan_def *chandef,
> @@ -177,7 +197,11 @@ 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 (ieee80211_chanctx_is_reserved(local, ctx))
> continue;
Are you really sure you want to drop the ctx->mode == EXCLUSIVE check here?
> - /* reserve the new or existing context */
> sdata->reserved_chanctx = new_ctx;
> new_ctx->refcount++;
> -
> sdata->reserved_chandef = *chandef;
Shouldn't this be in the [2/3]?
> out:
> mutex_unlock(&local->chanctx_mtx);
> @@ -703,37 +734,45 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
> ieee80211_stop_queues_by_reason(&local->hw,
> IEEE80211_MAX_QUEUE_MAP,
> IEEE80211_QUEUE_STOP_REASON_CHANCTX);
> + /* unref our reservation */
> + ctx->refcount--;
> + sdata->reserved_chanctx = NULL;
>
> - ieee80211_unassign_vif_chanctx(sdata, old_ctx);
> - if (old_ctx->refcount == 0)
> - ieee80211_free_chanctx(local, old_ctx);
> + if (old_ctx == ctx) {
> + /* This is our own context, just change it */
> + ret = __ieee80211_vif_change_channel(sdata, old_ctx,
> + &local_changed);
> + if (ret)
> + goto out;
> + } else {
> + 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;
> + if (sdata->vif.bss_conf.chandef.width !=
> + sdata->reserved_chandef.width)
> + local_changed |= BSS_CHANGED_BANDWIDTH;
>
> - sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> + 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;
> + 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_recalc_chanctx_chantype(local, ctx);
> + ieee80211_recalc_smps_chanctx(local, ctx);
> + ieee80211_recalc_radar_chanctx(local, ctx);
> }
Not really sure if you need to `else` and re-indent the whole thing
because you already do a `goto` in the `if`..
> *changed = local_changed;
> -
> - ieee80211_recalc_chanctx_chantype(local, ctx);
> - ieee80211_recalc_smps_chanctx(local, ctx);
> - ieee80211_recalc_radar_chanctx(local, ctx);
> -out_wake:
> +out:
> 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;
> }
Are you sure you want to remove the `out_wake` from here? Why not in the [2/3]?
Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
