Return-path: Received: from mail-oa0-f43.google.com ([209.85.219.43]:38536 "EHLO mail-oa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753672AbaCRI6Q convert rfc822-to-8bit (ORCPT ); Tue, 18 Mar 2014 04:58:16 -0400 Received: by mail-oa0-f43.google.com with SMTP id eb12so1584298oac.2 for ; Tue, 18 Mar 2014 01:58:15 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1395132248.15856.21.camel@dubbel> References: <1394547394-3910-1-git-send-email-luciano.coelho@intel.com> <1394547394-3910-4-git-send-email-luciano.coelho@intel.com> <1395130437.15856.13.camel@dubbel> <1395132248.15856.21.camel@dubbel> Date: Tue, 18 Mar 2014 09:58:15 +0100 Message-ID: (sfid-20140318_095830_069781_25843A8A) Subject: Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211 From: Michal Kazior To: Luca Coelho Cc: linux-wireless , Johannes Berg , "sw@simonwunderlich.de" , "Otcheretianski, Andrei" , Eliad Peller Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 18 March 2014 09:44, Luca Coelho wrote: > On Tue, 2014-03-18 at 09:36 +0100, Michal Kazior wrote: >> On 18 March 2014 09:13, Coelho, Luciano wrote: >> > On Fri, 2014-03-14 at 09:40 +0100, Michal Kazior wrote: >> >> On 11 March 2014 15:16, Luciano Coelho 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ł