On 28 February 2014 15:32, Luca Coelho <luca@xxxxxxxxx> wrote:
> On Fri, 2014-02-28 at 15:07 +0100, Michal Kazior wrote:
>> On 28 February 2014 14:41, Luca Coelho <luca@xxxxxxxxx> wrote:
>> > On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote:
>> >> The check is simply I have in mind is simply:
>> >>
>> >> bool ieee80211_chanctx_needs_channel_change(struct ieee80211_local
>> >> *local, struct ieee80211_chanctx *ctx) {
>> >> lockdep_assert_held(&local->chanctx_mtx);
>> >> rcu_read_lock();
>> >> list_for_each_entry_rcu(sdata, &local->interfaces, list) {
>> >> if (sdata->reserved_chanctx != ctx)
>> >> continue;
>> >> if (get_current_chanctx(sdata) == sdata->reserved_chanctx)
>> >> return true;
>> >> }
>> >> rcu_read_unlock();
>> >> return false;
>> >> }
>> >>
>> >> IOW if there's a least one vif bound to given chanctx and the vif has
>> >> both current and future chanctx the same, then the chanctx requires
>> >> in-place channel change (and this matches your original condition
>> >> (mode == RESERVED)).
>> >>
>> >> This should be future proof for multi-interface/channel.
>> >
>> > Okay, I get your point, it's not strictly necessary. But this would be
>> > needed in other places too, for example in the combinations check. We
>> > don't want to allow a new interface to join a chanctx that is going to
>> > change. In my merge between the combination check series and this one,
>> > I have this: http://pastebin.coelho.fi/65603f9d06b28cb2.txt
>>
>> Hmm.. Good point, but the snippet doesn't prevent new vifs from
>> joining a chanctx that's going to change channel.
>
> No, the prevention actually happens in ieee80211_find_chanctx() and it's
> already part of this series. I just wanted to point out that this is
> needed in several different places.
If you consider multi-vif I don't think doing that in find_chanctx()
is the best thing. If you skip reserved chanctx in find_chanctx() then
you can't use this function when you reserve chanctx that needs
channel change with more than 1 vif.
>> I'm also not quite sure if you need it in the combo check at all.
>> Can't you just throw EBUSY when you try to assign a new vif to chanctx
>> that's going to change channel?
>
> A reserved channel context is taking up space, so new interfaces can't
> rely on being able to use it. Let's say a HW supports 1 chanctx and
> there is one vif. Now the vif wants to change channel and reserves its
> chanctx to be changed later. If a new vif needs a chanctx, it can't use
> the one that is reserved, because it will be changed in the future. So,
> during the combination checks, we need to calculate the number of needed
> chanctxs for the new vif to be added is 2, so it should fail.
..but returning EBUSY in assign_vif clearly fulfils this requirement.
And I'm still not convinced you need to take "reserved" chanctx into
any consideration during combination checks.
>> For multi-channel hw you could allow creating new chanctx (if there's
>> enough channels in current combination) and make 2 chanctx that will
>> be compatible in the future (and worry about merging them later), or
>> you could deny that until reservation is finalized.
>
> Yes, if you have enough chanctxs to use, it's not a problem. But during
> the count we need to consider the ones that will be changed (and are
> thus marked as RESERVED) almost as an EXCLUSIVE chanctx, so they are
> counted separately. The difference between EXCLUSIVE and RESERVED, is
> only that a RESERVED chanctx can have more than one vif (as long as all
> of them have compatible future chandefs).
I think it's just an obfuscation. Either way "reserved" is an
orthogonal state to the original mode. You just want to keep it in the
"old_mode" and have some [mode, old_mode] combinations invalid leaving
just 4 states that can be actually set.
>> > If I'd use the iteration function there would be a lot of iterations
>> > going on. Not sure that's a problem though.
>> >
>> > The advantages of your approach is that we need less moving parts (ie.
>> > less stuff to save in sdata). The advantage of using a new mode is that
>> > it would require less code to run.
>>
>> I'd rather not have to worry about memoizing variables and
>> recalculating them when it's not strictly necessary (this isn't tx
>> path). In both cases you have to worry about locking which I think is
>> enough.
>
> As I said, I don't have a preference. Except that, being lazy, I'd
> prefer not to change what I already did. :P
For what I care I can just make follow up patches that rework some of
the bits I'm complaining about now that I think are necessary for
multi-vif channel switching as long as Johannes is okay with that
(i.e. merge your current code and then accept my re-work). I'm not in
any place to force you to do my bidding ;-)
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
