Return-path: Received: from mail-qg0-f48.google.com ([209.85.192.48]:42937 "EHLO mail-qg0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750714AbaETJZX (ORCPT ); Tue, 20 May 2014 05:25:23 -0400 Received: by mail-qg0-f48.google.com with SMTP id i50so280309qgf.35 for ; Tue, 20 May 2014 02:25:22 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1399798250-20987-1-git-send-email-emmanuel.grumbach@intel.com> <1399798250-20987-3-git-send-email-emmanuel.grumbach@intel.com> From: Arik Nemtsov Date: Tue, 20 May 2014 12:25:07 +0300 Message-ID: (sfid-20140520_112529_755056_4F8F9F81) Subject: Re: [PATCH 2/7] cfg80211: introduce regulatory flags controlling bw To: "Luis R. Rodriguez" Cc: Emmanuel Grumbach , Johannes Berg , linux-wireless , Arik Nemtsov Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Tue, May 20, 2014 at 11:51 AM, Luis R. Rodriguez wrote: > On Tue, May 20, 2014 at 1:33 AM, Arik Nemtsov wrote: >> On Tue, May 20, 2014 at 11:26 AM, Luis R. Rodriguez >> wrote: >>> On Sun, May 11, 2014 at 1:50 AM, Emmanuel Grumbach >>> wrote: >>>> From: Arik Nemtsov >>>> >>>> Allow setting bandwidth related regulatory flags. These flags are mapped >>>> to the corresponding channel flags in the specified range. >>>> Make sure the new flags are consulted when calculating the maximum >>>> bandwidth allowed by a regulatory-rule. >>>> >>>> Also allow propagating the GO_CONCURRENT modifier from a reg-rule to a >>>> channel. >>>> >>>> Change-Id: I1b0506ab332cdd268cbf4b02e03574f5c30ba5c0 >>>> Signed-off-by: Arik Nemtsov >>>> Signed-off-by: Emmanuel Grumbach >>>> --- >>>> include/uapi/linux/nl80211.h | 12 ++++++++++++ >>>> net/wireless/reg.c | 30 ++++++++++++++++++++++++++++-- >>>> 2 files changed, 40 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h >>>> index 406010d..f332231 100644 >>>> --- a/include/uapi/linux/nl80211.h >>>> +++ b/include/uapi/linux/nl80211.h >>>> @@ -2558,6 +2558,11 @@ enum nl80211_sched_scan_match_attr { >>>> * @NL80211_RRF_AUTO_BW: maximum available bandwidth should be calculated >>>> * base on contiguous rules and wider channels will be allowed to cross >>>> * multiple contiguous/overlapping frequency ranges. >>>> + * @NL80211_RRF_GO_CONCURRENT: See &NL80211_FREQUENCY_ATTR_GO_CONCURRENT >>>> + * @NL80211_RRF_NO_HT40MINUS: channels can't be used in HT40- operation >>>> + * @NL80211_RRF_NO_HT40PLUS: channels can't be used in HT40+ operation >>>> + * @NL80211_RRF_NO_80MHZ: 80MHz operation not allowed >>>> + * @NL80211_RRF_NO_160MHZ: 160MHz operation not allowed >>>> */ >>>> enum nl80211_reg_rule_flags { >>>> NL80211_RRF_NO_OFDM = 1<<0, >>>> @@ -2570,11 +2575,18 @@ enum nl80211_reg_rule_flags { >>>> NL80211_RRF_NO_IR = 1<<7, >>>> __NL80211_RRF_NO_IBSS = 1<<8, >>>> NL80211_RRF_AUTO_BW = 1<<11, >>>> + NL80211_RRF_GO_CONCURRENT = 1<<12, >>>> + NL80211_RRF_NO_HT40MINUS = 1<<13, >>>> + NL80211_RRF_NO_HT40PLUS = 1<<14, >>>> + NL80211_RRF_NO_80MHZ = 1<<15, >>>> + NL80211_RRF_NO_160MHZ = 1<<16, >>>> }; >>> >>> Automatic calculation on max bandwidth scales better, I'd much prefer >>> these only be used and properly documented to only be used in cases >>> where we want to be explicit about this, I don't think this should be >>> the automatic behavior, or at least your patch in no way explains any >>> justification as to why the move is being done, so I cannot guess what >>> the logic was here. >> >> We're not going to use CRDA/wireless-regdb as the regulatory data >> source. > > Why not? There's no good reason I can point to. It's just a decision by Intel regulatory. This is not only for Linux, but for everything. > >> These flags fit the reg-data storage format of the intel FW. >> It's just a different way of doing things. > > You can covert things, we have tons of drivers doing that already. Again, it's not my call to do this. > >>>> #define NL80211_RRF_PASSIVE_SCAN NL80211_RRF_NO_IR >>>> #define NL80211_RRF_NO_IBSS NL80211_RRF_NO_IR >>>> #define NL80211_RRF_NO_IR NL80211_RRF_NO_IR >>>> +#define NL80211_RRF_NO_HT40 (NL80211_RRF_NO_HT40MINUS |\ >>>> + NL80211_RRF_NO_HT40PLUS) >>>> >>>> /* For backport compatibility with older userspace */ >>>> #define NL80211_RRF_NO_IR_ALL (NL80211_RRF_NO_IR | __NL80211_RRF_NO_IBSS) >>>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c >>>> index 558b0e3..efd6d0d 100644 >>>> --- a/net/wireless/reg.c >>>> +++ b/net/wireless/reg.c >>>> @@ -572,8 +572,9 @@ static const struct ieee80211_regdomain *reg_get_regdomain(struct wiphy *wiphy) >>>> return get_cfg80211_regdom(); >>>> } >>>> >>>> -unsigned int reg_get_max_bandwidth(const struct ieee80211_regdomain *rd, >>>> - const struct ieee80211_reg_rule *rule) >>>> +static unsigned int >>>> +reg_get_max_bandwidth_from_range(const struct ieee80211_regdomain *rd, >>>> + const struct ieee80211_reg_rule *rule) >>> >>> This change is renaming a routine we used heavily to something tucked >>> in the closet and then introducing the usage of the new flags, which >>> have no justifications nor proper regdb updates, this also doesn't >>> even account for the old regdb situations so I'll have to provide a >>> huge NACK for all these reasons. >>> >>> You want to consider wireless-regdb / CRDA when making these changes. >> >> I can rename things any way you'd like. > > You missed my point, you just did not rename something you went and > then made this new unused flag you introduced be used in tons of > locations, that'd introduce tons of regressions. This is on purpose - I want this code to consider the new flags in all existing locations. I'm not sure I follow regarding regressions. The new flags are not set in any existing regulatory database. > >> Not sure how this can hurt old >> regdb situations though. The new flags are not used in any rules >> there. > > Exactly, you can't expect the rules to work then. Why won't old regdb rules work? The NL80211_RRF_NO_160MHZ for instance is not used anywhere in old or new regdbs. So all the new code in reg_get_max_bandwidth is ignored in current or older crda/regdb flows. What am I missing? Arik