All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: Kalle Valo <kvalo@codeaurora.org>, Jouni Malinen <j@w1.fi>,
	Pkshih <pkshih@realtek.com>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>
Subject: Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits
Date: Mon, 1 Jun 2020 18:32:31 -0700	[thread overview]
Message-ID: <CA+ASDXMf7iXuE9hQ-XitPPfvXP0EK5FchJLCu2+5Ag=ZC=0H0w@mail.gmail.com> (raw)
In-Reply-To: <e8908eafd8e6050eef8782c6a7019e318d14f65f.camel@sipsolutions.net>

I haven't quite reached the 3 month lag on the prior replies here :)

I do want to see this question resolved somehow, or else we'll be
dragging along out-of-tree patches for...(counts on fingers)...4
different nl80211 APIs for the same feature, and a driver-detecting
user space for it. I think I was half-hoping that we'd get to chat at
netdev about this, but that's not happening any time soon. (Topic for
another day: does a "wireless workshop" still happen at a virtual-only
conference?)

On Fri, Mar 20, 2020 at 5:56 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> But as somebody has said on one of these threads, there seem to
> basically be two kinds of APIs:
>
>  1) some kind of platform-dependent index into a table that the
>     driver/device has, or perhaps the BIOS; and
>
>  2) some kind of per-band (FSVO band) power restriction like here.
>
>
> The first is like iwlwifi, and I think Marvell was mentioned? But
> they're basically out - there's no information, and there's no clue as
> to which indices might even be valid, I think, nor what they mean. So
> there isn't really much value in a common API for that since you can't
> use it in a common fashion - arguably a common API would be worse...

I think they could still be salvageable. In practice, the only way
I've seen them used is to define a "low power" and a "high power"
table. So if there were conventions (e.g., Marvell's could be
documented in Device Tree, where they store their SAR tables), we
could expose an easy high/low knob.

> However, the case of 2, arguably the proposals are very similar?
>
> Qualcomm: optional nl80211_band, 1/2 dBm units
> Realtek: 2.4, four 5 GHz subbands, 1/4 dBm units

nit: I believe it's 1/8 for Realtek. And I thought it was 1 for
Qualcomm, but apparently this changes every week I see a new
(non-upstream-facing) submission from them...which is a part of the
problem I'd like to solve. They don't have to commit to anything if
it's all downstream ;)

> Both have some strange namespace thing where the same namespace contains
> both the outer and inner attributes. Probably should think about the
> policy with NLA_POLICY_NESTED and see how that works.

Nobody really accused vendors of being good at writing netlink APIs :)
And incidentally, that's another reason why I'd rather not just keep
taking vendor command APIs downstream...

> But it any case, these two don't seem like an insurmountable issue to
> combine?

Exactly my point. And that's why I didn't feel like we really got past #2 [0]:
"The submitter shall consider a pure nl80211 implementation and
explain the reasoning against it in the commit message."

> Say, something like defining a list of affected frequency
> ranges in the wiphy properties, and then giving a list of TX powers that
> matches the list of frequency ranges? We can go to 1/4 dBm or so, that's
> not such a big deal, I'd think?

That sounds about right. We'd probably want a reporting API too, so
drivers can tell what ranges they support, so user space doesn't have
to get creative about handling EINVAL or similar.

I've also seen the Qualcomm proposal include a per-chain option
(although it's missing from QCA6174) so it might be reasonable to
account for that up-front (similar to STA_INFO_CHAIN_SIGNAL?).

One note: while I can imagine that vendors might have other
configurability parameters that aren't reflected in this proposal
(e.g., per-PHY-mode? per-chain?), I've also found that some of the
complexities that vendors offer don't *really* end up being needed by
ODMs/OEMs when building and certifying devices.

> On the other hand, what does that actually buy us? If you cannot have
> common userspace that knows how a given platform must behave, then it's
> not very worthwhile to have common API for it?
>
> Brian, what do you think from a platform/userspace perspective - how do
> you actually determine the SAR limits? I'm guessing you just have a
> table of sorts, but how do you get the table? Would you actually have
> common userspace and benefit from having common API?

This strikes at the sort of thing where Chrome OS tailors the
experience to the hardware, whereas the average Linux distro does not.
So I can describe what we do, but it's tough to say how useful this
ends up being to everyone else. But maybe that's beside the point,
because your average distro will probably not know how to hook up SAR
regardless (but much less so if there's no mainline API for it).

Anyway, the table: Chrome OS has a generalized per-SKU configuration
system, which keys off of a thing called "SKU ID." A number of
factors, including board-level straps or EEPROMs, come together to
determine which model and SKU a given system is. Once the config
system knows that, it can retrieve the data table from its database
(it's just a giant JSON blob). NB: while we currently ship separate
builds (with separate JSON configs) for separate families of systems
(think SoC families), the config system *could* work as a single table
for all Chrome OS systems.

Currently, because each vendor implements this differently enough, we
have separate schema for each of the 2 drivers [1]. But we could just
as well unify this into a range-based system to match the range
proposal above.

Common user space: today, we have a single user space; it's just ugly
[2]. Some of that ugliness is due to the unnecessary divergence in
range-based APIs, which probably would clean up a little if we
supported a common range API. (We'd still be left with our Marvell and
Intel index approaches.)

Really, I could live with per-vendor APIs. My primary goal is to get
these upstream in some form, so vendors can stop using things like
this as a reason for shipping us non-upstream code, and so we can
reduce the delta between upstream and Chrome OS kernels.

I also think that, for the cases that warrant it (i.e., the option 2
-- Realtek and Qualcomm cases, so far), it would be good to have a
common API, but that's a somewhat secondary concern for me.

Also, Kalle had some concerns about stable ABI questions: shouldn't we
bake in some kind of "check for support" feature to these kinds of
APIs [3]? That would help ease transition, if we do start with a
vendor API and move to a common one in the future. Given the nature of
this feature, I wouldn't expect there will be a large variety of users
making use of the APIs -- and I, for one, would be happy to migrate my
user space over some period of time, as needed.

Brian

[0] https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
[1] https://chromium.googlesource.com/chromiumos/platform2/+/master/chromeos-config/README.md#wifi
[2] https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/powerd/set_wifi_transmit_power.cc
[3] e.g., we ran into the lack of such support here:
7010998c6caf nl80211: add NL80211_CMD_UPDATE_FT_IES to supported commands

WARNING: multiple messages have this Message-ID
From: Brian Norris <briannorris@chromium.org>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: Jouni Malinen <j@w1.fi>, Pkshih <pkshih@realtek.com>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>,
	Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits
Date: Mon, 1 Jun 2020 18:32:31 -0700	[thread overview]
Message-ID: <CA+ASDXMf7iXuE9hQ-XitPPfvXP0EK5FchJLCu2+5Ag=ZC=0H0w@mail.gmail.com> (raw)
In-Reply-To: <e8908eafd8e6050eef8782c6a7019e318d14f65f.camel@sipsolutions.net>

I haven't quite reached the 3 month lag on the prior replies here :)

I do want to see this question resolved somehow, or else we'll be
dragging along out-of-tree patches for...(counts on fingers)...4
different nl80211 APIs for the same feature, and a driver-detecting
user space for it. I think I was half-hoping that we'd get to chat at
netdev about this, but that's not happening any time soon. (Topic for
another day: does a "wireless workshop" still happen at a virtual-only
conference?)

On Fri, Mar 20, 2020 at 5:56 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> But as somebody has said on one of these threads, there seem to
> basically be two kinds of APIs:
>
>  1) some kind of platform-dependent index into a table that the
>     driver/device has, or perhaps the BIOS; and
>
>  2) some kind of per-band (FSVO band) power restriction like here.
>
>
> The first is like iwlwifi, and I think Marvell was mentioned? But
> they're basically out - there's no information, and there's no clue as
> to which indices might even be valid, I think, nor what they mean. So
> there isn't really much value in a common API for that since you can't
> use it in a common fashion - arguably a common API would be worse...

I think they could still be salvageable. In practice, the only way
I've seen them used is to define a "low power" and a "high power"
table. So if there were conventions (e.g., Marvell's could be
documented in Device Tree, where they store their SAR tables), we
could expose an easy high/low knob.

> However, the case of 2, arguably the proposals are very similar?
>
> Qualcomm: optional nl80211_band, 1/2 dBm units
> Realtek: 2.4, four 5 GHz subbands, 1/4 dBm units

nit: I believe it's 1/8 for Realtek. And I thought it was 1 for
Qualcomm, but apparently this changes every week I see a new
(non-upstream-facing) submission from them...which is a part of the
problem I'd like to solve. They don't have to commit to anything if
it's all downstream ;)

> Both have some strange namespace thing where the same namespace contains
> both the outer and inner attributes. Probably should think about the
> policy with NLA_POLICY_NESTED and see how that works.

Nobody really accused vendors of being good at writing netlink APIs :)
And incidentally, that's another reason why I'd rather not just keep
taking vendor command APIs downstream...

> But it any case, these two don't seem like an insurmountable issue to
> combine?

Exactly my point. And that's why I didn't feel like we really got past #2 [0]:
"The submitter shall consider a pure nl80211 implementation and
explain the reasoning against it in the commit message."

> Say, something like defining a list of affected frequency
> ranges in the wiphy properties, and then giving a list of TX powers that
> matches the list of frequency ranges? We can go to 1/4 dBm or so, that's
> not such a big deal, I'd think?

That sounds about right. We'd probably want a reporting API too, so
drivers can tell what ranges they support, so user space doesn't have
to get creative about handling EINVAL or similar.

I've also seen the Qualcomm proposal include a per-chain option
(although it's missing from QCA6174) so it might be reasonable to
account for that up-front (similar to STA_INFO_CHAIN_SIGNAL?).

One note: while I can imagine that vendors might have other
configurability parameters that aren't reflected in this proposal
(e.g., per-PHY-mode? per-chain?), I've also found that some of the
complexities that vendors offer don't *really* end up being needed by
ODMs/OEMs when building and certifying devices.

> On the other hand, what does that actually buy us? If you cannot have
> common userspace that knows how a given platform must behave, then it's
> not very worthwhile to have common API for it?
>
> Brian, what do you think from a platform/userspace perspective - how do
> you actually determine the SAR limits? I'm guessing you just have a
> table of sorts, but how do you get the table? Would you actually have
> common userspace and benefit from having common API?

This strikes at the sort of thing where Chrome OS tailors the
experience to the hardware, whereas the average Linux distro does not.
So I can describe what we do, but it's tough to say how useful this
ends up being to everyone else. But maybe that's beside the point,
because your average distro will probably not know how to hook up SAR
regardless (but much less so if there's no mainline API for it).

Anyway, the table: Chrome OS has a generalized per-SKU configuration
system, which keys off of a thing called "SKU ID." A number of
factors, including board-level straps or EEPROMs, come together to
determine which model and SKU a given system is. Once the config
system knows that, it can retrieve the data table from its database
(it's just a giant JSON blob). NB: while we currently ship separate
builds (with separate JSON configs) for separate families of systems
(think SoC families), the config system *could* work as a single table
for all Chrome OS systems.

Currently, because each vendor implements this differently enough, we
have separate schema for each of the 2 drivers [1]. But we could just
as well unify this into a range-based system to match the range
proposal above.

Common user space: today, we have a single user space; it's just ugly
[2]. Some of that ugliness is due to the unnecessary divergence in
range-based APIs, which probably would clean up a little if we
supported a common range API. (We'd still be left with our Marvell and
Intel index approaches.)

Really, I could live with per-vendor APIs. My primary goal is to get
these upstream in some form, so vendors can stop using things like
this as a reason for shipping us non-upstream code, and so we can
reduce the delta between upstream and Chrome OS kernels.

I also think that, for the cases that warrant it (i.e., the option 2
-- Realtek and Qualcomm cases, so far), it would be good to have a
common API, but that's a somewhat secondary concern for me.

Also, Kalle had some concerns about stable ABI questions: shouldn't we
bake in some kind of "check for support" feature to these kinds of
APIs [3]? That would help ease transition, if we do start with a
vendor API and move to a common one in the future. Given the nature of
this feature, I wouldn't expect there will be a large variety of users
making use of the APIs -- and I, for one, would be happy to migrate my
user space over some period of time, as needed.

Brian

[0] https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
[1] https://chromium.googlesource.com/chromiumos/platform2/+/master/chromeos-config/README.md#wifi
[2] https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/powerd/set_wifi_transmit_power.cc
[3] e.g., we ran into the lack of such support here:
7010998c6caf nl80211: add NL80211_CMD_UPDATE_FT_IES to supported commands

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

  reply	other threads:[~2020-06-02  1:32 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-18 15:48 [PATCH 0/2] ath10k: SAR power limit vendor command Kalle Valo
2019-12-18 15:48 ` Kalle Valo
2019-12-18 15:48 ` [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits Kalle Valo
2019-12-18 15:48   ` Kalle Valo
2019-12-19  9:44   ` Pkshih
2019-12-19  9:44     ` Pkshih
2019-12-19 15:48     ` Jouni Malinen
2019-12-19 15:48       ` Jouni Malinen
2019-12-19 18:32       ` Brian Norris
2019-12-19 18:32         ` Brian Norris
2019-12-19 18:55         ` Jouni Malinen
2019-12-19 18:55           ` Jouni Malinen
2019-12-19 23:40           ` Brian Norris
2019-12-19 23:40             ` Brian Norris
2020-03-17 16:54             ` Kalle Valo
2020-03-17 16:54               ` Kalle Valo
2020-03-20 12:55               ` Johannes Berg
2020-03-20 12:55                 ` Johannes Berg
2020-06-02  1:32                 ` Brian Norris [this message]
2020-06-02  1:32                   ` Brian Norris
2020-07-16  9:35                   ` Kalle Valo
2020-07-16  9:35                     ` Kalle Valo
2020-07-16 18:56                     ` Brian Norris
2020-07-16 18:56                       ` Brian Norris
2020-07-24  9:26                       ` Kalle Valo
2020-07-24  9:26                         ` Kalle Valo
2020-07-30 13:24                         ` Johannes Berg
2020-07-30 13:24                           ` Johannes Berg
2020-08-01  1:31                           ` Brian Norris
2020-08-01  1:31                             ` Brian Norris
2020-09-08  5:55                           ` Kalle Valo
2020-09-08  5:55                           ` Kalle Valo
2020-07-30 13:17                   ` Johannes Berg
2020-07-30 13:17                     ` Johannes Berg
2019-12-18 15:48 ` [PATCH 2/2] ath10k: allow dynamic SAR power limits to be configured Kalle Valo
2019-12-18 15:48   ` Kalle Valo
2019-12-19  9:45   ` Pkshih
2019-12-19  9:45     ` Pkshih
2020-04-16  7:38   ` Kalle Valo
2020-04-16  7:38   ` Kalle Valo

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='CA+ASDXMf7iXuE9hQ-XitPPfvXP0EK5FchJLCu2+5Ag=ZC=0H0w@mail.gmail.com' \
    --to=briannorris@chromium.org \
    --cc=ath10k@lists.infradead.org \
    --cc=j@w1.fi \
    --cc=johannes@sipsolutions.net \
    --cc=kvalo@codeaurora.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=pkshih@realtek.com \
    --subject='Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.