ath10k.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Carl Huang <cjhuang@codeaurora.org>
To: Abhishek Kumar <kuabhs@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Doug Anderson <dianders@chromium.org>,
	ath10k <ath10k@lists.infradead.org>,
	Abhishek Kumar <kuabhs@google.com>
Subject: Re: [PATCH v2 1/3] nl80211: add common API to configure SAR power limitations.
Date: Tue, 01 Dec 2020 17:40:20 +0800	[thread overview]
Message-ID: <59737cabfa1e308dcf8cd21613d5e5ee@codeaurora.org> (raw)
In-Reply-To: <CACTWRwt43EJ9njgtJvVEd9vx3Abs7=X6cDrOL4aAqPRMQyJB8A@mail.gmail.com>

On 2020-12-01 16:37, Abhishek Kumar wrote:
> The V2 patch looks good to me.
> Regarding Brian's comment
> 
>> [1] By the way, you aren't checking for duplicates; so users could
>> pass the same index many times, and it's not clear from the API
>> definition what should happen. It seems the current implementation is
>> that you'll just use the last value provided.
> 
> I don't think we should be adding any logic in the kernel to check for
> duplicates, but rather userspace should take care of those. As long as
> the data provided abides by the data policy, the kernel should bother.
> But I do agree with Brian's other comment that it might be made more
> clear in comment. If at all a V3 is needed, then we should add that,
> or else it looks fine.
> 
> Reviewed-by: Abhishek Kumar <kuabhs@chromium.org>
> 
I didn't notice this comment, I actually added the check in the kernel 
in V3.
I think it's OK to check in kernel. I also add some comments that 
duplicated
index is not allowed.

> Thanks
> Abhishek
> 
> On Mon, Nov 30, 2020 at 2:10 AM Carl Huang <cjhuang@codeaurora.org> 
> wrote:
>> 
>> On 2020-11-21 10:42, Brian Norris wrote:
>> > On Fri, Nov 20, 2020 at 12:53 AM Carl Huang <cjhuang@codeaurora.org>
>> > wrote:
>> >>
>> >> NL80211_CMD_SET_SAR_SPECS is added to configure SAR from
>> >> user space. NL80211_ATTR_SAR_SPEC is used to pass the SAR
>> >> power specification when used with NL80211_CMD_SET_SAR_SPECS.
>> >>
>> >> Wireless driver needs to register SAR type, supported frequency
>> >> ranges to wiphy, so user space can query it. The index in
>> >> frequency range is used to specify which sub band the power
>> >> limitation applies to. The SAR type is for compatibility, so later
>> >> other SAR mechanism can be implemented without breaking the user
>> >> space SAR applications.
>> >>
>> >> Normal process is user space quries the SAR capability, and
>> >> gets the index of supported frequency ranges and associates the
>> >> power limitation with this index and sends to kernel.
>> >>
>> >> Here is an example of message send to kernel:
>> >> 8c 00 00 00 08 00 01 00 00 00 00 00 38 00 2b 81
>> >> 08 00 01 00 00 00 00 00 2c 00 02 80 14 00 00 80
>> >> 08 00 02 00 00 00 00 00 08 00 01 00 38 00 00 00
>> >> 14 00 01 80 08 00 02 00 01 00 00 00 08 00 01 00
>> >> 48 00 00 00
>> >>
>> >> NL80211_CMD_SET_SAR_SPECS:  0x8c
>> >> NL80211_ATTR_WIPHY:     0x01(phy idx is 0)
>> >> NL80211_ATTR_SAR_SPEC:  0x812b (NLA_NESTED)
>> >> NL80211_SAR_ATTR_TYPE:  0x00 (NL80211_SAR_TYPE_POWER)
>> >> NL80211_SAR_ATTR_SPECS: 0x8002 (NLA_NESTED)
>> >> freq range 0 power: 0x38 in 0.25dbm unit (14dbm)
>> >> freq range 1 power: 0x48 in 0.25dbm unit (18dbm)
>> >>
>> >> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
>> >
>> > I think the API is reasonably clear and usable. I'm a little skeptical
>> > that the complexity related to indexes is absolutely necessary [1],
>> > but at least you make clear what should happen in the case of missing
>> > indexes (treated as "max"). But you've addressed my concerns, I think:
>> >
>> > Reviewed-by: Brian Norris <briannorris@chromium.org>
>> >
>> > I haven't done the most thorough review on the implementation pieces
>> > (and ath10k), but I at least wanted to put my thoughts out there.
>> >
>> > Thanks,
>> > Brian
>> >
>> > [1] By the way, you aren't checking for duplicates; so users could
>> > pass the same index many times, and it's not clear from the API
>> > definition what should happen. It seems the current implementation is
>> > that you'll just use the last value provided.
>> Thanks for the comments.
>> It's right the last value is used.
>> I can describe it more clearly if V3 is needed.
> 
> _______________________________________________
> 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

  reply	other threads:[~2020-12-01  9:40 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-20  8:53 [PATCH v2 0/3] add common API to configure SAR Carl Huang
2020-11-20  8:53 ` [PATCH v2 1/3] nl80211: add common API to configure SAR power limitations Carl Huang
2020-11-21  2:42   ` Brian Norris
2020-11-30 10:10     ` Carl Huang
2020-12-01  8:37       ` Abhishek Kumar
2020-12-01  9:40         ` Carl Huang [this message]
2020-11-20  8:53 ` [PATCH v2 2/3] mac80211: add ieee80211_set_sar_specs Carl Huang
2020-12-01  8:38   ` Abhishek Kumar
2020-11-20  8:53 ` [PATCH v2 3/3] ath10k: allow dynamic SAR power limits via common API Carl Huang
2020-11-20 11:43   ` kernel test robot

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=59737cabfa1e308dcf8cd21613d5e5ee@codeaurora.org \
    --to=cjhuang@codeaurora.org \
    --cc=ath10k@lists.infradead.org \
    --cc=briannorris@chromium.org \
    --cc=dianders@chromium.org \
    --cc=kuabhs@chromium.org \
    --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).