From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from z5.mailgun.us ([104.130.96.5]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kaC8B-00029e-GO for ath10k@lists.infradead.org; Wed, 04 Nov 2020 06:18:53 +0000 MIME-Version: 1.0 Date: Wed, 04 Nov 2020 14:18:40 +0800 From: Carl Huang Subject: Re: [RFC 1/2] nl80211: add common API to configure SAR power limitations. In-Reply-To: References: <20201030205639.1452712-1-kuabhs@chromium.org> <20201031024631.1528113-1-kuabhs@chromium.org> Message-ID: <0d820718ca30962f783fc13f2d9db2bc@codeaurora.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: Abhishek Kumar Cc: Abhishek Kumar , briannorris@chromium.org, linux-wireless@vger.kernel.org, Doug Anderson , ath10k@lists.infradead.org, Johannes Berg On 2020-11-04 09:17, Abhishek Kumar wrote: > On Tue, Nov 3, 2020 at 5:15 AM Johannes Berg > wrote: >> >> On Tue, 2020-11-03 at 10:34 +0800, Carl Huang wrote: >> > 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. >> >> 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k