ath10k.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Abhishek Kumar <kuabhs@chromium.org>
To: Carl Huang <cjhuang@codeaurora.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, 1 Dec 2020 00:37:04 -0800	[thread overview]
Message-ID: <CACTWRwt43EJ9njgtJvVEd9vx3Abs7=X6cDrOL4aAqPRMQyJB8A@mail.gmail.com> (raw)
In-Reply-To: <8f0121e9996bad2266a928279e7eb26a@codeaurora.org>

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>

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

  reply	other threads:[~2020-12-01  8:37 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 [this message]
2020-12-01  9:40         ` Carl Huang
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='CACTWRwt43EJ9njgtJvVEd9vx3Abs7=X6cDrOL4aAqPRMQyJB8A@mail.gmail.com' \
    --to=kuabhs@chromium.org \
    --cc=ath10k@lists.infradead.org \
    --cc=briannorris@chromium.org \
    --cc=cjhuang@codeaurora.org \
    --cc=dianders@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).