Re: [PATCH 2/2] cfg80211: reg: support ieee80211-(min|max)-center-freq DT properties

[Arend van Spriel <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>]

On 28-12-16 22:30, Rafał Miłecki wrote:
> On 28 December 2016 at 22:28, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 28 December 2016 at 22:07, Arend van Spriel
>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>> On 28-12-16 16:59, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> They allow specifying hardware limitations of supported channels. This
>>>> may be useful for specifying single band devices or devices that support
>>>> only some part of the whole band.
>>>> E.g. some tri-band routers have separated radios for lower and higher
>>>> part of 5 GHz band.
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>> ---
>>>>  net/wireless/reg.c | 34 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>>>> index 5dbac37..35ba5c7 100644
>>>> --- a/net/wireless/reg.c
>>>> +++ b/net/wireless/reg.c
>>>> @@ -1123,6 +1123,26 @@ const char *reg_initiator_name(enum nl80211_reg_initiator initiator)
>>>>  }
>>>>  EXPORT_SYMBOL(reg_initiator_name);
>>>>
>>>> +static bool reg_center_freq_of_valid(struct wiphy *wiphy,
>>>> +                                  struct ieee80211_channel *chan)
>>>> +{
>>>> +     struct device_node *np = wiphy_dev(wiphy)->of_node;
>>>> +     u32 val;
>>>> +
>>>> +     if (!np)
>>>> +             return true;
>>>> +
>>>> +     if (!of_property_read_u32(np, "ieee80211-min-center-freq", &val) &&
>>>> +         chan->center_freq < KHZ_TO_MHZ(val))
>>>> +             return false;
>>>> +
>>>> +     if (!of_property_read_u32(np, "ieee80211-max-center-freq", &val) &&
>>>> +         chan->center_freq > KHZ_TO_MHZ(val))
>>>> +             return false;
>>>
>>> I suspect these functions rely on CONFIG_OF. So might not build for
>>> !CONFIG_OF.
>>
>> I compiled it with
>> # CONFIG_OF is not set
>>
>> Can you test it and provide a non-working config if you see a
>> compilation error, please?
> 
> include/linux/of.h provides a lot of dummy static inline functions if
> CONFIG_OF is not used (they also allow compiler to drop most of the
> code).

of_propeirty_read_u32 is static inline in of.h calling
of_property_read_u32_array, which has a dummy variant in of.h returning
-ENOSYS so -38. Pretty sure that is not what you want. At least it does
not allow the compiler to drop any code so probably better to do:

if (!IS_ENABLED(CONFIG_OF) || !np)
	return true;

So with this patch you change the channel to DISABLED. I am not very
familiar with reg.c so do you know if this is done before or after
calling regulatory notifier in the driver. brcmfmac will enable channels
querying the device upon regulatory notifier call, which may undo the
behavior introduced by your patch.

Regards,
Arend