On 22 January 2014 10:01, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> On Wed, 2014-01-22 at 09:54 +0100, Michal Kazior wrote:
>
>> >> A new worker is introduced: csa_complete_work
>> >> which is used to account per-interface countdowns
>> >> and issue the actual channel switch after last
>> >> interface completes its CSA.
>> >
>> > Not sure I understand this part, don't they all have the same countdown,
>> > and should finish at the same time?
>>
>> Keep in mind you get cfg80211_csa_settings array for the channel
>> switch each having it's own csa beacon count. I can try to refactor
>> the code a little bit though.
>
> Can't we just prohibit that though? That seems pretty much useless,
> although if the interfaces actually have different beacon intervals (if
> such a thing is permitted?) then it would be necessary to do more
> difficult calculations ...
Not really sure what extra calculation would be necessary here beyond
just waiting until last interfaces finishes sending CSA beacons?
>> >> +static int ieee80211_ap_beacon_presp_backup(struct ieee80211_sub_if_data *sdata)
>> >
>> > I'm beginning to think it's time to create a new file
>> > net/mac80211/ap.c :-)
>> >
>> > What's the backup/restore used for anyway?
>>
>> Hmm.. Now that I think about it this might not be really worth it. The
>> fact is, if you're in CSA it means you don't want to remain on a given
>> channel so there's actually little to no point restoring old beacon
>> for that channel.
>>
>> Can I just bss_conf->enable_beacon=false && notify_bss_info_changed()?
>> That would just stop beaconing and avoid TXing on the old channel.
>
> I can't really comment on that. I guess you don't want to stick to the
> old channel either way, although if you're doing CSA for a non-radar
> reason (e.g. P2P) you might be OK with that? But it seems like a rare
> corner case that maybe doesn't need all that much attention?
>
>> Maybe nl80211 should have a csa-completed event (like scan), so it can
>> return success/failure/status so userspace can actually take action if
>> CSA fails for some reason in mid-way?
>
> Maybe? Not sure what it would do? The error case would be more like this
> "oops, AP stopped" event that we've been discussing, no? I still need to
> do that ...
Right. "AP stopped" seems nice. My worry here is it might be tricky to
be practical with CSA wrt locking/races.
>> >> lockdep_assert_held(&local->mtx);
>> >> + lockdep_assert_held(&local->chanctx_mtx);
>> >>
>> >> - /* should never be called if not performing a channel switch. */
>> >> - if (WARN_ON(!sdata->vif.csa_active))
>> >> - return -EINVAL;
>> >> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
>> >
>> > _rcu doesn't seem right.
>>
>> Hmm. Taking iflist_mtx right here is impossible (deadlock). Some
>> refactoring would need to be done here so that it is taken before
>> chanctx_mtx, but after local->mtx at the same time.
>
> Isn't it under RTNL anyway though?
No, it isn't. This is eventually called from a worker. I don't think
you can use RTNL in a per-interface worker, can you?
>> > But then again this whole function, iterating interfaces rather than
>> > channel contexts etc. is terrible IMHO. This seems to pretty much turn
>> > chanctx support upside down, making the channel context structures not
>> > be the first-class citizens I think we want them to be.
>>
>> chanctx aren't aware what interfaces they are assigned to now.
>
> That's true, so maybe you just need to explain this chandef thing
> better? I don't really see the utility (admittedly without actually
> checking carefully in the code where you need it)
Since you can CSA multiple interfaces you can have different chandefs
for those interfaces. This means you need to know the chandef for the
final channel switch.
You could probably cache this to local->csa_chandef when processing
the channel switch request itself, but that means you explicitly limit
yourself to single CSA against a given hw/driver.
At one point I was contemplating having a dedicated structure for
channel switches, and have a list of channel switch structures in
ieee80211_local, but perhaps that's just an overkill.
>> > I'd say Luca's approach has more future, as it makes channel context
>> > support native. With this, I don't even see how you *could* later add
>> > real channel context support. A function like this, that essentially
>> > assumes exactly a single chanctx that might be doing CSA and similar
>> > seems to make that rather difficult.
>>
>> I'm not really aware of Luca's work.
>
> I guess he should post it :)
>
>> >> - sdata->vif.bss_conf.chandef = *chandef;
>> >> - ctx->conf.def = *chandef;
>> >> + /* multi-channel is not supported, multi-vif is */
>> >> + if (num_chanctx > 1)
>> >> + return NULL;
>> >
>> > This is essentially the same thing, but written as a chanctx function?
>>
>> I'm not following. Same thing as what?
>
> It's rather similar to the chandef function above and I'm not sure I
> understand where the differences are needed.
The function is basically used to verify if channel switch is allowed
when processing CSA request and right before doing the actual channel
switch as a sanity check.
The request is validated against the assumption that there is only 1
chanctx and chanctx->refcount matches the number of CSA interface
requests. Without multi-channel it doesn't make much sense to move 2
interfaces from a channel context that has 5 interfaces bound to it as
that would mean you drag 3 unsuspecting interfaces to a different
channel (at least with the current code). I assume this will be
obsolete with Luca's patches as it changes how channel switching
interacts with chanctx?
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
