On 18 March 2014 09:44, Luca Coelho <luca@xxxxxxxxx> wrote:
> On Tue, 2014-03-18 at 09:36 +0100, Michal Kazior wrote:
>> On 18 March 2014 09:13, Coelho, Luciano <luciano.coelho@xxxxxxxxx> wrote:
>> > On Fri, 2014-03-14 at 09:40 +0100, Michal Kazior wrote:
>> >> On 11 March 2014 15:16, Luciano Coelho <luciano.coelho@xxxxxxxxx> wrote:
>>
>> [...]
>>
>> >> > + list_for_each_entry(ctx, &local->chanctx_list, list) {
>> >> > + if (ctx->conf.radar_enabled)
>> >> > + radar_detect |= BIT(ctx->conf.def.width);
>> >>
>> >> Is this really correct?
>> >
>> > I'm not sure anymore. :)
>> >
>> >
>> >> The original behaviour was to:
>> >>
>> >> for each wdev:
>> >> get_chan_state(..., &radar_width)
>> >>
>> >> where get_chan_state() ORs BIT(width) accordingly to iftype, etc.
>> >>
>> >> For 2 APs with different operational widths (e.g. 20 and 40) this means:
>> >>
>> >> a) old code: width_detect = BIT(WIDTH_20) | BIT(WIDTH_40)
>> >> b) new code: width_detect = BIT(chanctx->width) which is BIT(WIDTH_40)
>> >>
>> >> I think radar_detect should be computed:
>> >>
>> >> radar_detect = 0 // **
>> >> for each vif:
>> >> if vif.ctx == ctx && vif.radar_required:
>> >> radar_detect = BIT(vif.bss.width)
>> >>
>> >> ** you could argue to initialize with BIT(ctx->width), but all things
>> >> seem to use ieee80211_vif_use_channel() (including CAC/radar
>> >> detection) so bss_conf.chandef.width should be set correctly and
>> >> iteration alone should suffice.
>> >
>> > But does this really make sense? This was more or less the idea I had
>> > when I changed the cfg80211_chandef_dfs_required() to return a bit mask
>> > (which I later reverted).
>> >
>> > I think the current behavior may be wrong. In the old
>> > cfg80211_can_use_iftype_chan() we match *any* of the bits in
>> > radar_detect with the bits supported in the interface combinations. So,
>> > in your example, we would accept the combination even if the driver did
>> > not support WIDTH_40 (because it would match WIDTH_20). If the context
>> > is set to WIDTH_40, don't we have to make sure the driver can detect
>> > radars on that width?
>>
>> Fair point, but current behaviour of cfg80211_can_use_iftype_chan()
>> doesn't seem like a correct one to me. It shouldn't match any but
>> *all* of the radar_detect bits, no?
>
> I don't really know. I think that if we're using 40MHz, we need to be
> able to detect on the entire 40MHz, and that includes the 20MHz part as
> well. So being able to detect on 20MHz in that same channel context is
> irrelevant, isn't it?
True but take this hypothetical case:
You have a driver combination that supports radar_detect only at
40MHz. You start AP @ 40MHz with radar. You match the radar_detect.
You start AP @ 20MHz with radar. You match again radar_detect (because
there's the AP @ 40MHz running). You stop AP @ 40Mhz. You now have
only an AP running at 20MHz which requires radar_detect for 20MHz but
there's no combination to match that. Oops. We should've never allowed
the AP @ 20MHz to start.
I doubt there's hardware that needs a combination like that but it
just bothers me current structure and logic allows it.
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
