From: Carl Huang <cjhuang@codeaurora.org>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: briannorris@chromium.org, linux-wireless@vger.kernel.org,
dianders@chromium.org, ath10k@lists.infradead.org,
kuabhs@google.com
Subject: Re: [PATCH 1/3] nl80211: add common API to configure SAR power limitations.
Date: Wed, 11 Nov 2020 15:44:25 +0800 [thread overview]
Message-ID: <00c810b30b91397e562ca54475940afc@codeaurora.org> (raw)
In-Reply-To: <64e072a168c12f58847a5ee16bfdb7e47576284f.camel@sipsolutions.net>
On 2020-11-06 18:25, Johannes Berg wrote:
> Hi,
>
> Looks pretty good. Some comments, mostly nits, below.
>
Thank you for the comments, Johannes.
I don't understand below well, please help explain:
> And even if we do need the index, then perhaps we should use the
> (otherwise anyway ignored) nla_type() of the container, instead of an
> explicit inner attribute?
>
>
>> +/**
>> + * nl80211_sar_attrs - Attributes for SAR spec
>
> missing enum
>
sure
>> + *
>> + * @NL80211_SAR_ATTR_TYPE: the SAR type and it's defined in
>> %nl80211_sar_type.
>
> better use &enum nl80211_sar_type for a link in docs
>
>> + *
>> + * @NL80211_SAR_ATTR_SPECS: Nested array of SAR power
>> + * limit specifications. Each specification contains a set
>> + * of %nl80211_sar_specs_attrs.
>> + *
>> + * For SET operation, it contains array of
>> NL80211_SAR_ATTR_SPECS_POWER
>
> some odd indent?
>
> Usually we just use a single tab.
>
sure
>> +/**
>> + * nl80211_sar_specs_attrs - Attributes for SAR power limit specs
>
> again, enum missing
>
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_POWER: Required (u32)value to specify the
>> actual
>> + * power limit value in units of 0.25 dBm if type is
>> + * NL80211_SAR_TYPE_POWER. (i.e., a value of 44 represents 11 dBm).
>> + * 0 means userspace doesn't have SAR limitation on this associated
>> range.
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_RANGE_INDEX: Required (u32) value to
>> specify the
>> + * index of exported freq range table and the associated power
>> limitation
>> + * is applied to this range.
>> + *
>> + * Userspace isn't required to set all the ranges advertised by WLAN
>> driver,
>> + * and userspace can skip some certain ranges. These skipped ranges
>> don't
>> + * have SAR limitations, and these are same as setting the
>> + * %NL80211_SAR_ATTR_SPECS_POWER to 0. But it's required to set at
>> least one range,
>> + * no matter the power limiation is 0 or not.
>
> (typo - limitation)
>
> Should "0" really be the magic value? Theoretically, 0 and even
> negative
> values are valid. Perhaps we should just use something big (0xffffffff)
> to indicate no limit, or just not have such a "no limitation" value
> because userspace can always set it to something very big that means no
> practical limitation anyway?
>
> OK actually you have a U8 now so the high limit is 63.75dBm, but
> there's
> not really a good reason for that, since U32 takes the same space in
> netlink anyway.
>
Looks 0 and negative value are not practical as it means <= 1mw,
but I can use S32 instead.
Not sure if a magic value is needed? If it's needed, then perhaps
0x7fffffff
is good for it?
> And wait, I thought we agreed to remove the index? Now I'm confused.
>
Using index in SET operation doesn't add burden to userspace and kernel,
but it provides some flexibility so userspace can skip some certain
ranges.
> And even if we do need the index, then perhaps we should use the
> (otherwise anyway ignored) nla_type() of the container, instead of an
> explicit inner attribute?
>
I don't understand what means here. Use nla_type for what?
>> + *
>> + * Every SET operation overwrites previous SET operation.
>> + *
>> + * @NL80211_SAR_ATTR_SPECS_START_FREQ: Required (u32) value to
>> specify the start
>> + * frequency of this range edge when registering SAR capability to
>> wiphy. It's
>> + * not a channel center frequency. The unit is KHz.
>
> "kHz" not "KHz", in a few places other than this too
>
>> +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;
>
> extra space?
>
>> + for (i = 0; i < num_freq_ranges; i++) {
>> + sub_freq_range = nla_nest_start(msg, i + 1);
>> +
>> + nla_put_u32(msg, NL80211_SAR_ATTR_SPECS_START_FREQ,
>> + rdev->wiphy.sar_capa->freq_ranges[i].start_freq);
>> +
>> + nla_put_u32(msg, NL80211_SAR_ATTR_SPECS_END_FREQ,
>> + rdev->wiphy.sar_capa->freq_ranges[i].end_freq);
>
>
> Need to check the return values of these three calls.
>
sure
>
> And an aside, unrelated to this particular code: Should we do some kind
> of validation that the ranges reported actually overlap all supported
> channels (taking 20 MHz bandwidth into account)?
>
>> + nla_parse_nested(tb, NL80211_SAR_ATTR_MAX,
>> info->attrs[NL80211_ATTR_SAR_SPEC],
>> + sar_policy, info->extack);
>
> If you're not checking the return value then no point in passing a
> policy or extack :-)
>
> And yes, it's already validated, so you don't have to do it again.
>
Yes, will use NULL instead of info->extack
>> + sar_spec->type = type;
>> + specs = 0;
>> + nla_for_each_nested(spec_list, tb[NL80211_SAR_ATTR_SPECS], rem) {
>> + if (nla_parse(spec,
>> + NL80211_SAR_ATTR_SPECS_MAX,
>> + nla_data(spec_list),
>> + nla_len(spec_list),
>> + sar_specs_policy,
>> + NULL)) {
>
> Similar here, don't really need to validate it since it's done by the
> policy.
>
sure
>> + err = -EINVAL;
>> + goto error;
>> + }
>> +
>> + /* for power type, power value and index must be presented */
>> + if ((!spec[NL80211_SAR_ATTR_SPECS_POWER] ||
>> + !spec[NL80211_SAR_ATTR_SPECS_RANGE_INDEX]) &&
>> + type == NL80211_SAR_TYPE_POWER) {
>
> maybe "switch (type) {...}" or something and return -EINVAL also if
> it's
> a type not supported in the code yet, i.e. default case?
>
> Otherwise we might add a type, and forget this pretty easily.
>
Good suggestion, will change to switch case.
>> + err = -EINVAL;
>> + goto error;
>> + }
>> +
>> + power = nla_get_u8(spec[NL80211_SAR_ATTR_SPECS_POWER]);
>> + sar_spec->sub_specs[specs].power = power;
>
> and that probably should then be in a sub function or something also
> inside the particular type.
>
> or maybe just all in a separate function? dunno. not really
> _necessary_,
> but the lines are getting kinda long already, and one more indentation
> level with the switch won't help ...
>
I'll move this to a separate function.
> johannes
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
next prev parent reply other threads:[~2020-11-11 7:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-06 10:07 [PATCH 0/3] add common API to configure SAR Carl Huang
2020-11-06 10:07 ` [PATCH 1/3] nl80211: add common API to configure SAR power limitations Carl Huang
2020-11-06 10:25 ` Johannes Berg
2020-11-11 7:44 ` Carl Huang [this message]
2020-11-19 20:25 ` Abhishek Kumar
2020-11-20 7:01 ` Carl Huang
2020-11-06 10:07 ` [PATCH 2/3] mac80211: add ieee80211_set_sar_specs Carl Huang
2020-11-06 10:07 ` [PATCH 3/3] ath10k: allow dynamic SAR power limits via common API Carl Huang
2020-11-19 20:02 ` Abhishek Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=00c810b30b91397e562ca54475940afc@codeaurora.org \
--to=cjhuang@codeaurora.org \
--cc=ath10k@lists.infradead.org \
--cc=briannorris@chromium.org \
--cc=dianders@chromium.org \
--cc=johannes@sipsolutions.net \
--cc=kuabhs@google.com \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).