On Tue, 2014-02-04 at 16:25 +0100, Michal Kazior wrote:
> On 4 February 2014 13:59, Luciano Coelho <luca@xxxxxxxxx> wrote:
> > [...]
> > + /* try to find another context with the chandef we want */
> > + new_ctx = ieee80211_find_chanctx(local, chandef,
> > + IEEE80211_CHANCTX_SHARED);
> > + if (new_ctx) {
> > + /* reserve the existing compatible context */
> > + sdata->csa_reserved_chanctx = new_ctx;
> > + new_ctx->refcount++;
> > + } else if (curr_ctx->refcount == 1) {
> > + /* We're the only user of the current context, mark it
> > + * as exclusive, so nobody tries to use it until we
> > + * finish the channel switch. This is an optimization
> > + * to prevent waste of contexts when the number is
> > + * limited.
> > + */
> > +
> > + sdata->csa_reserved_chanctx = curr_ctx;
> > + sdata->csa_chandef = *chandef;
> > +
> > + sdata->csa_chanctx_old_mode = curr_ctx->mode;
> > +
> > + curr_ctx->mode = IEEE80211_CHANCTX_EXCLUSIVE;
>
> The only reason to do this would seem drv_add_chanctx() can fail if
> you have too many chanctx in driver.
This is just an optimization case. I'm not even sure every driver will
be able to change the chanctx on the fly so we probably would need to
add a HWCONF flag for that.
And the idea here is to prevent others from joining out chanctx since we
are going to change it in the near future. Whoever joins now probably
wants the current configuration not our future one.
Maybe we could add a new mode, IEEE80211_CHANCTX_CHANNEL_SWITCH, so
other vifs would only be able to reserve it if their future config
matches our future config?
This starts complicating things too much for a small optimization that
may not even work with every driver. Perhaps is better to just remove
the old context and add a new for this case as well...
> Perhaps we could defer drv_add_chanctx() for reserved chanctx?
I'm not sure I understand. For this situation (ie. we're alone in the
chanctx) we don't do a drv_add_chanctx().
> > [...]
> > + 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;
> > +
> > + /* TODO: what happens if another vif created a context
> > + * that is compatible with our future chandef while
> > + * this one has been marked as exclusive? Merge?
> > + */
> > +
> > + ctx->mode = sdata->csa_chanctx_old_mode;
> > +
> > + *changed = local_changed;
> > + goto out;
> > + }
> > +
> > + ieee80211_stop_queues_by_reason(&local->hw,
> > + IEEE80211_MAX_QUEUE_MAP,
> > + IEEE80211_QUEUE_STOP_REASON_CSA);
> > +
>
> Shouldn't queues be stopped depending on block_tx in CSA IE?
That is the case when we *receive* the CSA. At this point, we're
already making the actual channel switch. The queues are stopped here
because we're going to unassign the current chanctx and assign a new one
and we don't want to send frames meanwhile. It doesn't hurt if they
were already stopped before, it will be a NOP.
> > + 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 != ctx->conf.def.width)
> > + local_changed |= BSS_CHANGED_BANDWIDTH;
>
> I wonder if this check makes any sense. With multi-interface you can
> have varied bss_conf.width and not match the shared width of a channel
> context, can't you?
You're right. I guess I should set this flag according to what happens
when I call ieee80211_recalc_chanctx_chantype(). 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).
I'll look into that.
> > +
> > + sdata->vif.bss_conf.chandef = ctx->conf.def;
> > +
> > + /* unref our reservation before assigning */
> > + ctx->refcount--;
> > + ret = ieee80211_assign_vif_chanctx(sdata, ctx);
>
> This looks wrong. You don't seem to care about old_ctx->refcount here
> at all. If you perform CSA for 2 interfaces on a single-channel hw
> you'll have 2 interfaces on 2 different channels at one point.
I'm unassigning the vif from old_ctx (a few lines above). And now, I'm
reassigning the vif to ctx (the new one).
I think instead of caring about the single-channel stuff here, it should
be handled in the ieee80211_reserve_chanctx() function. It's one of the
reasons for having the reservation in the first place.
> The way I see it should be more like this:
>
> old_ctx->csa_completions++;
> if (old_ctx->csa_completions != old_ctx->csa_requests)
> goto wait_for_more;
>
> disconnect_non_csa_interfaces(old_ctx); // *
> // get list of csa vifs from old_ctx
> for_each_csa_vif_in_ctx(sdata)
> unassign_vif_chanctx(sdata, old_ctx)
> free_chanctx(old_ctx) // **
> drv_add_chanctx(new_ctx)
> for_each_csa_vif_in_ctx(sdata)
> assign_vif_chanctx(sdata, new_ctx)
>
> old_ctx->csa_requests would be increased when you reserve a channel.
>
> * "switch or die" policy; ideally this should go through cfg80211
> somehow so that everything is teared down properly and userspace is
> notified (AP stopped)
>
> ** this might remove the need for the exclusive reserved chanctx?
One of the problems here is that you're checking if the switch can
succeed only at the end of the process. The idea is to fail channel
switch requests as soon as they come in.
I agree that, during the reservation, we need a bit more logic to take
care of the interface combinations and the single-channel cases. But
not here. When I implemented this patch, I was not considering !chanctx
nor single-radio device. ;)
The idea with reservation is more or less like this, as an example:
We have a STA and a P2P-GO (which follows the STA).
1. STA gets CSA from the AP
2. STA makes a reservation for the new context (here the possibility of
creating a new context is checked)
3. the GO controller (ie. wpa_s) gets a notification that there is a
switch going on for the STA
4. the GO wants to follow, so it also reserves the same new context (the
new context is not really used yet, but reserved twice)
5. the STA completes the CS countdown and uses its reservation
6. the GO completes the CS countdown and uses its reservation
7. now the original context doesn't have any user anymore (ie.
refcount=0) and can be freed
5 and 6 can happen in a different order too, it doesn't matter.
Of course this assumes multi-radio. The assumption in this patch is
that if you don't have multi-radio and have 2 interfaces on the same
channel, the channel switch will fail (and we disconnect). Fixing that
comes on top with your patches. ;)
I like your idea of synchronizing things before the switch to help the
single-channel case. But I think we should still have the reservation.
When using the reservation we could check whether the and trying to
free old_ctx) we could check whether the contexts are valid together, if
not, delay the usage of the reserved context until the new combinations
is fine.
>
> > + if (ret) {
> > + /* if assign fails refcount stays the same */
> > + if (ctx->refcount == 0)
> > + ieee80211_free_chanctx(local, ctx);
> > + goto out;
> > + }
> > +
> > + /* TODO: should we wake the queues here? */
>
> Hmm.. you probably should wake queues if this was the last channel
> switch for the hw.
Yes, the queues should be woken here, when the new chanctx is fully
available.
--
Luca.
--
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
