On 2020-11-04 09:17, Abhishek Kumar wrote:
> On Tue, Nov 3, 2020 at 5:15 AM Johannes Berg
> <johannes at sipsolutions.net> wrote:
>>
>> On Tue, 2020-11-03 at 10:34 +0800, Carl Huang wrote:
>> > On 2020-10-31 10:46, Abhishek Kumar wrote:
>> > > From: kuabhs at 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.
>>
>> Wait, what? The IFINDEX is still required, that identifies the
>> interface
>> - though it probably shouldn't be required, this should be per wiphy,
>> so
>> you could also use ATTR_WIPHY_IDX?
>>
>> You're thinking of ... something
>> else. NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX perhaps?
>
> Yes, probably the index mapping that you are talking about is using
> NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX .
>
You're right. I just remembered Johannes's comments to remove
the NL80211_SAR_ATTR_SPECS_FREQ_RANGE_INDEX and made mistake here.
> I think Johannes has already covered most of the comments. I have one
> last one mentioned below.
>
>> +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;
>> +
>> + if (nla_put_u16(msg, NL80211_SAR_ATTR_TYPE,
>> rdev->wiphy.sar_capa->type))
>> + goto fail;
>
> The nla_put_u16 NL80211_SAR_ATTR_TYPE here uses u16 whereas in the to
> get this property below it uses nla_get_u32 . I think this should be
> consistent ?
>
>> + if (!tb[NL80211_SAR_ATTR_TYPE])
>> + return -EINVAL;
>> +
>> + type = nla_get_u32(tb[NL80211_SAR_ATTR_TYPE]);
>> +
>> + if (!tb[NL80211_SAR_ATTR_SPECS])
>> + return -EINVAL;
>
> As mentioned above the get and put for NL80211_SAR_ATTR_TYPE is not
> consistent.
>
Correct. Thanks for pointing out and it will be fixed in next version.
> -Abhishek
>
> _______________________________________________
> ath10k mailing list
> ath10k at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k