Re: [RFC v3] initial channel context implementation

[Michal Kazior <michal.kazior@tieto.com>]

Johannes Berg wrote:
> On Thu, 2012-06-28 at 09:54 +0200, Michal Kazior wrote:
>
>>>> Yes, this is more or less what I also had in mind. I was just thinking
>>>> about solving the issue of channel context and hw.conf.channel
>>>> consistency. If we switch a channel we either modify channel in channel
>>>> context directly (violating the immutability of channel contexts) or we
>>>> iterate and re-set the new channel on each interface (because
>>>> single-channel drivers may still have multiple interfaces and we
>>>> probably want to use sdata->vif.chanctx_conf->channel instead of
>>>> hw.conf.channel inside mac80211).
>>>>
>>>> Now that I think about it I guess violating the immutability for the
>>>> single-channel case is okay. It would greatly simplify the code and we'd
>>>> just put a comment down in hw_config where the only violation would occur.
>>>
>>> I'm not sure why we would violate it? The way I see it, you'd never
>>> change the channel context channel since internally in mac80211 you'd
>>> never want to see a different channel, just like today we use
>>> local->oper_channel everywhere we'd then use sdata->vif.chanctx->channel
>>> throughout, right?
>>>
>>> I think the only thing we need to do is put something like this into
>>> hw_config:
>>>
>>>    if (local->tmp_channel) {
>>>       local->hw.conf.channel = local->tmp_channel;
>>>       ...
>>>    } else {
>>>       local->hw.conf.channel = chanctx->channel;
>>>    }
>>>
>>> No?
>>
>> Using sdata->vif.chanctx_conf->channel instead of local->oper_channel
>> doesn't make any sense to me.
>>
>> Take ieee80211_tx() for example. It does:
>>
>> 	tx.channel = local->hw.conf.channel;
>>
>> We don't use oper_channel here, but hw.conf.channel. TX can happen on
>> different interfaces so for multi-channel operation it should be saying:
>>
>> 	tx.channel = sdata->vif.chanctx_conf->channel;
>>
>> In this case if we want to support the swscan/tmpchan through
>> hw_config() we need update the channel context's channel somehow.
>>
>> I'm more thinking of hw.conf.channel becoming more of a backup value for
>> single-channel drivers while we internally focus on channel contexts.
>
> Yes, makes sense. I forgot all about the TX code. I'm a little wary of
> making the contexts mutable, even in this case, because a lot of code
> uses local->oper_channel as well, and that is expected to really be the
> operating channel all of the time, even if we're scanning at some point
> in time.

Yeah. The other option (maintaining the immutability) is to iterate 
through all interfaces and call ieee80211_vif_use_channel when switching 
channel for single-channel operation. Or do you have something else in 
mind maybe?


> Luckily, tx.channel isn't actually used much, only for the band. So if
> we tag the SKBs with the band earlier (info->band), maybe we don't need
> to use hw.conf.channel as much there for tx.channel?
>
> Other uses where we do need the current channel are
>   * ieee80211_build_probe_req
>   * ieee80211_add_srates_ie/ieee80211_add_ext_srates_ie
>   * __ieee80211_start_scan uses it but need not, could use oper_channel
>     instead and the code never executes for multi-channel
>   * ieee80211_set_tx_power() is interesting, may need to make it all
>     per-sdata now through nl80211 etc.

What will drivers that don't support per-sdata tx_power do? Do all 
multi-vif (not multi-channel) drivers support per-interface tx power?

I guess we'd have to manage:
  a) common tx power value in ieee80211_local
  b) provide a function that calculates the common value
     so drivers may use it (and avoid code duplication)
  c) ..or else drivers would need to implement the calculation on
     their own


>   * rate_idx_to_bitrate can use the sta's sdata's channel
>   * ieee80211_change_bss can use the sdata's channel
>   * debugfs stuff probably just moves to per-sdata files
>   * ibss code all uses sdata channel
>   * ieee80211_if_change_type ... probably just set basic_rates = 0
>   * mesh can use sdata channel
>   * mlme.c should use sdata channel, but there's the channel switch stuff
>   * rate.h should use sta->sdata channel
>
> Much of this is actually means we have bugs today! Whenever we use
> hw.conf.channel and should be using sdata channel soon, we should be
> using local->oper_channel today!

Oh! Now I understand why you wanted to use channel contexts in place of 
oper_channel. This makes sense.


> Maybe it's worth fixing that first, and getting rid of *most* instances
> of hw.conf.channel, so we have a clearer idea of which changes in what
> ways?

Sounds like a good idea.


-- 
Pozdrawiam / Best regards, Michal Kazior.