Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.

From: Carl Huang <cjhuang@codeaurora.org>
To: Abhishek Kumar <kuabhs@chromium.org>
Cc: briannorris@chromium.org, linux-wireless@vger.kernel.org,
	dianders@chromium.org, ath10k@lists.infradead.org,
	johannes@sipsolutions.net
Subject: Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations.
Date: Tue, 03 Nov 2020 10:34:47 +0800	[thread overview]
Message-ID: <e5302eb0b20339d3f6544652e4034993@codeaurora.org> (raw)
In-Reply-To: <20201031024631.1528113-1-kuabhs@chromium.org>

On 2020-10-31 10:46, Abhishek Kumar wrote:
> From: kuabhs@chromium.org
> 
> There are few more additional comments here.
>> + * @NL80211_CMD_SET_SAR_SPECS: SAR power limitation configuration is
>> + *     passed using %NL80211_ATTR_SAR_SPEC.
>> + *
> 
> This command requires NL80211_ATTR_IFINDEX, probably should be better 
> to add
> this in the comment ?
> 
Per Johannes's comments, this explicit index is not required. Are you 
fine
with it?

Instead, user-space application records the array index when querying 
the SAR
capability. When set, the nested array index will be used to set the 
power.
This requires user-space has a strict mapping of index. and 
NL80211_ATTR_IFINDEX
is to be removed.

>> +static int
>> +nl80211_put_sar_specs(struct cfg80211_registered_device *rdev,
>> +                     struct sk_buff *msg)
>> +{
>> +       struct nlattr *sar_capa, *specs, *sub_freq_range;
>> +       u8  num_freq_ranges;
>> +       int i;
>> +
>> +       if (!rdev->wiphy.sar_capa)
>> +               return 0;
>> +
>> +       num_freq_ranges = rdev->wiphy.sar_capa->num_freq_ranges;
>> +
>> +       sar_capa = nla_nest_start(msg, NL80211_ATTR_SAR_SPEC);
>> +       if (!sar_capa)
>> +               return -ENOSPC;
> 
> I see some places nla_nest_start_noflag being used and here 
> nla_nest_start.
> Whats the specific reason to do that ? In the newer Kernel versions, I 
> believe
> nla_nest_start is preferred.
> 
This will be addressed in next version.

>> +               power = 
>> nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
>> +               sar_spec->sub_specs[specs].power = power;
>> +
>> +               /* if NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX isn't 
>> present,
>> +                * then the power applies to all bands. But it's only 
>> valid
>> +                * for the first entry.
>> +                */
>> +               if (!spec[NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX]) {
>> +                       if (specs) {
>> +                               err = -EINVAL;
>> +                               goto error;
>> +                       } else {
>> +                               
>> sar_spec->sub_specs[specs].freq_range_index =
>> +                                       NL80211_SAR_ALL_FREQ_RNAGES;
>> +                               specs++;
>> +                               break;
>> +                       }
>> +               }
> 
> Here I see you are assigning same power to all freq band if for the 
> first band
> the freq index is not provided. Is there any specific reason to only
> check the first
> here ? Probably this logic should move into specific drivers. Thoughts 
> ?
> 
This logic will be removed per Johannes's comments.

Please read  Johannes's comments. If you agree with him and has no other 
advices,
then I will post the second version of it.

> -Abhishek
> 
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k