[1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits
diff mbox series

Message ID 1576684108-30177-2-git-send-email-kvalo@codeaurora.org
State New
Headers show
Series
  • ath10k: SAR power limit vendor command
Related show

Commit Message

Kalle Valo Dec. 18, 2019, 3:48 p.m. UTC
From: Wen Gong <wgong@codeaurora.org>

The vendor commands is to add API for user to configure dynamic SAR
power limits, it will not replace the existing power control
functionality, it is to make more convenient to configure power.

An example of usage(wlan0 is the wireless interface dev name):
iw dev wlan0 vendor send 0x1374 0x92 0x2C 0x00 0x03 0x00 0x14 0x00 0x01
0x00 0x08 0x00 0x07 0x00 0x22 0x00 0x00 0x00 0x08 0x00 0x04 0x00 0x00
0x00 0x00 0x00 0x14 0x00 0x02 0x00 0x08 0x00 0x07 0x00 0x11 0x00 0x00
0x00 0x08 0x00 0x04 0x00 0x01 0x00 0x00 0x00

means of bytes:
0x1374: vendor id
0x92: vendor subcmd id
0x22: 2.4G power limit
0x11: 5G power limit

Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
---
 include/uapi/nl80211-vnd-qca.h | 68 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 include/uapi/nl80211-vnd-qca.h

Comments

Pkshih Dec. 19, 2019, 9:44 a.m. UTC | #1
On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> From: Wen Gong <wgong@codeaurora.org>
> 
> The vendor commands is to add API for user to configure dynamic SAR
> power limits, it will not replace the existing power control
> functionality, it is to make more convenient to configure power.
> 
> An example of usage(wlan0 is the wireless interface dev name):
> iw dev wlan0 vendor send 0x1374 0x92 0x2C 0x00 0x03 0x00 0x14 0x00 0x01
> 0x00 0x08 0x00 0x07 0x00 0x22 0x00 0x00 0x00 0x08 0x00 0x04 0x00 0x00
> 0x00 0x00 0x00 0x14 0x00 0x02 0x00 0x08 0x00 0x07 0x00 0x11 0x00 0x00
> 0x00 0x08 0x00 0x04 0x00 0x01 0x00 0x00 0x00
> 
> means of bytes:
> 0x1374: vendor id
> 0x92: vendor subcmd id
> 0x22: 2.4G power limit
> 0x11: 5G power limit
> 
> Tested with QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
> ---
>  include/uapi/nl80211-vnd-qca.h | 68
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 include/uapi/nl80211-vnd-qca.h
> 
> diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> new file mode 100644
> index 000000000000..482c9409a2c0
> --- /dev/null
> +++ b/include/uapi/nl80211-vnd-qca.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: ISC */
> +/*
> + * Copyright (c) 2019 The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _UAPI_NL80211_VND_QCA_H
> +#define _UAPI_NL80211_VND_QCA_H
> +
> +/* Vendor id to be used in vendor specific command and events to user space
> + * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
> + * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
> + * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
> + * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that
> + */
> +#define QCA_NL80211_VENDOR_ID 0x001374
> +
> +/**
> + * enum qca_nl80211_vendor_subcmds - QCA nl80211 vendor command identifiers
> + *
> + *@QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS and is used to retrieve the
> + *	settings currently in use. The attributes returned by this command
> are
> + *	defined by enum qca_vendor_attr_sar_limits.
> + */
> +enum qca_nl80211_vendor_subcmds {
> +	QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS = 146,
> +	};
> +
> +/**
> + * enum qca_vendor_attr_sar_limits - Attributes for SAR power limits
> + *
> + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC: Nested array of SAR power
> + *	limit specifications. The number of specifications is
> + *	specified by @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_NUM_SPECS. Each
> + *	specification contains a set of
> + *	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_* attributes. A
> + *	specification is uniquely identified by the attributes
> + *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND and always
> + *	contains as a payload the attribute
> + *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT.
> + *
> + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
> + *	indicate for which band this specification applies. Valid
> + *	values are enumerated in enum %nl80211_band (although not all
> + *	bands may be supported by a given device). If the attribute is

Can we define separated enum to address four 5G sub-bands, likes

enum nl80211_sar_band {
	NL80211_SAR_BAND_2G,
	NL80211_SAR_BAND_5G_BAND1,
	NL80211_SAR_BAND_5G_BAND2,
	NL80211_SAR_BAND_5G_BAND3,
	NL80211_SAR_BAND_5G_BAND4,
};

I think this vendor command can be a generic nl80211 command, because
we need SAR
power limit as well.

> + *	not supplied then the specification will be applied to all
> + *	supported bands.
> + *
> + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT: Required (u32)
> + *	value to specify the actual power limit value in units of 0.5
> + *	dBm (i.e., a value of 11 represents 5.5 dBm).

Can we have higher precision, in unit of 0.125?


> + *	This is required, when %QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT is
> + *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT_USER.
> + *
> + * These attributes are used with %QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS
> + * and %QCA_NL80211_VENDOR_SUBCMD_GET_SAR_LIMITS.
> + */
> +enum qca_vendor_attr_sar_limits {
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_INVALID = 0,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC = 3,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND = 4,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT = 7,

Why these enum aren't continual?
The reason may be because QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT and something
me
ntioned in above paragraph but missing in this enum?

They will waste memory when doing nla_parse() with tb[MAX];

> +
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST,
> +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX =
> +		QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST - 1
> +};
> +
> +#endif /* _UAPI_NL80211_VND_QCA_H_ */
Jouni Malinen Dec. 19, 2019, 3:48 p.m. UTC | #2
On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > + * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
> > + * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
> > + * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
> > + * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that

> > + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
> > + *	indicate for which band this specification applies. Valid
> > + *	values are enumerated in enum %nl80211_band (although not all
> > + *	bands may be supported by a given device). If the attribute is
> 
> Can we define separated enum to address four 5G sub-bands, likes
> 
> enum nl80211_sar_band {
> 	NL80211_SAR_BAND_2G,
> 	NL80211_SAR_BAND_5G_BAND1,
> 	NL80211_SAR_BAND_5G_BAND2,
> 	NL80211_SAR_BAND_5G_BAND3,
> 	NL80211_SAR_BAND_5G_BAND4,
> };

Please note that the vendor subcmd and attributes used here are already
deployed and in use as a kernel interface. As such, the existing
attributes cannot really be modified; if anything else would be needed,
that would need to be defined as a new attribute and/or command.

> I think this vendor command can be a generic nl80211 command, because
> we need SAR
> power limit as well.

This was discussed during the 2019 wireless workshop. The conclusion
from that discussion was that while there is clear need for SAR power
limits for various devices and multiple vendors/drivers, it did not look
clear that a single common interface could be defined cleanly taken into
account the differences in the ways vendors have designed the mechanism
in driver and firmware implementations. As such, vendor specific
commands were identified as the approach.

> > + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT: Required (u32)
> > + *	value to specify the actual power limit value in units of 0.5
> > + *	dBm (i.e., a value of 11 represents 5.5 dBm).
> 
> Can we have higher precision, in unit of 0.125?

This existing attribute cannot be modified, i.e., a new one would need
to be added if a different precision is needed. As far as the specific
need for the vendor command defined here is concerned, 0.5 dB unit is
sufficient.

> > + * These attributes are used with %QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS
> > + * and %QCA_NL80211_VENDOR_SUBCMD_GET_SAR_LIMITS.
> > + */
> > +enum qca_vendor_attr_sar_limits {
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_INVALID = 0,
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC = 3,
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND = 4,
> > +	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT = 7,
> 
> Why these enum aren't continual?
> The reason may be because QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT and something
> me
> ntioned in above paragraph but missing in this enum?

This patch does not include all the assigned values (see hostap.git for
full details if desired), i.e., all the values are actually assigned,
but the proposed use case for ath10k does not need the values that were
left out from this header file that is a copy of the authoritative
definition of the vendor attributes.
Brian Norris Dec. 19, 2019, 6:32 p.m. UTC | #3
On Thu, Dec 19, 2019 at 7:48 AM Jouni Malinen <j@w1.fi> wrote:
> On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> > On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > > + * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
> > > + * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
> > > + * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
> > > + * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that

By the way, I wonder -- why have this statement? That's not how I
recall any other piece of kernel development ever working; upstream
Linux defines the upstream Linux API, not some arbitrary user space
project. This statement could be useful for saying, "don't stomp on
those command numbers please," but the response should probably be to
go out and define a totally new vendor ID or something. (See below.)

> > > + * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
> > > + * indicate for which band this specification applies. Valid
> > > + * values are enumerated in enum %nl80211_band (although not all
> > > + * bands may be supported by a given device). If the attribute is
> >
> > Can we define separated enum to address four 5G sub-bands, likes
> >
> > enum nl80211_sar_band {
> >       NL80211_SAR_BAND_2G,
> >       NL80211_SAR_BAND_5G_BAND1,
> >       NL80211_SAR_BAND_5G_BAND2,
> >       NL80211_SAR_BAND_5G_BAND3,
> >       NL80211_SAR_BAND_5G_BAND4,
> > };
>
> Please note that the vendor subcmd and attributes used here are already
> deployed and in use as a kernel interface. As such, the existing
> attributes cannot really be modified; if anything else would be needed,
> that would need to be defined as a new attribute and/or command.

Clarification: you're talking about out-of-tree drivers, which really
have no relevance in upstream discussion, except possibly as examples.
I don't think it's ever been a valid approach to dictate upstream
kernel design based simply on "what $vendor already implemented for
Android."

Maybe it's a better idea to just use different command numbers (or
vendor ID?) here, to avoid stomping on each others' implementation
choices. Otherwise, it sounds like our only choice here is to copy
your Android driver verbatim, or get lost.

> > I think this vendor command can be a generic nl80211 command, because
> > we need SAR
> > power limit as well.
>
> This was discussed during the 2019 wireless workshop. The conclusion
> from that discussion was that while there is clear need for SAR power
> limits for various devices and multiple vendors/drivers, it did not look
> clear that a single common interface could be defined cleanly taken into
> account the differences in the ways vendors have designed the mechanism
> in driver and firmware implementations. As such, vendor specific
> commands were identified as the approach.

[citation needed]
I was in that workshop, and while I recall the assertion, I don't
recall any evidence [1]. In fact, I've watched (off-list) Wen Gong
propose several variations of this exact same API along the way (hint,
I'm the one requesting he upstream this), and it's clear there's
*some* flexibility in the API. If, for example, the driver attempted
to provide a list of frequency bands it supports controlling TX power
for, that would go a long way toward sharing this API between drivers.

Another hint: this is exactly why Pkshih is speaking up here -- I'm
fielding requests from him and his employer on implementing the same
feature, and his API is starting to look an awful lot like yours. So I
suggested he review this proposal to see where and why they differ.

Anyway, I don't really object with starting out with a
Qualcomm-specific and a Realtek-specific vendor command to implement
nearly the same feature, but I'd prefer if people did engage in some
healthy discussion about why they can't share an API, with the hopes
that maybe they can converge someday. In fact, that's exactly what the
Wiki says about this:

https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

"The goal with these rules is to enable use of vendor commands in a
fashion that is transparent enough to allow later reuse by other
components with similar needs, and then potentially defining “real”
nl80211 API for the use case in question."

Regards,
Brian

[1] The closest thing to evidence I've seen is that certain $vendors
decide they don't want to give user space any control at all over the
SAR power tables, for $reasons. But such $vendors should not really
have any say in implementing user space APIs for those $vendors that
do provide such control.
Jouni Malinen Dec. 19, 2019, 6:55 p.m. UTC | #4
On Thu, Dec 19, 2019 at 10:32:06AM -0800, Brian Norris wrote:
> On Thu, Dec 19, 2019 at 7:48 AM Jouni Malinen <j@w1.fi> wrote:
> > On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> > > On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > > > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > > > + * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
> > > > + * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
> > > > + * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
> > > > + * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that
> 
> By the way, I wonder -- why have this statement? That's not how I
> recall any other piece of kernel development ever working; upstream
> Linux defines the upstream Linux API, not some arbitrary user space
> project. This statement could be useful for saying, "don't stomp on
> those command numbers please," but the response should probably be to
> go out and define a totally new vendor ID or something. (See below.)

As far as the OUI 00:13:74 is concerned, the defined mechanism for
getting any new identifier values assigned is by getting a patch applied
into hostap.git. If another OUI is used, that can clearly use different
mechanism for this. Anyway, I'd expect the process for OUI 00:13:74 to
be the most convenient one to use.

This does not mean that there could not be new subcmds and attributes
defined as part of the upstream kernel review and those changes being
modified as part of that review. However, the final ID values for the
subcmds/attributes would happen through whatever mechanism defined for
the particular OUI. For 00:13:74, that would mean defining the
subcmds/attributes first with TBD ID values and once the definitions are
otherwise fine in the kernel patch review, contributing a patch to
hostap.git to get the identifiers assigned, updating the kernel patch to
use the assigned values, and apply it to the kernel.

> > Please note that the vendor subcmd and attributes used here are already
> > deployed and in use as a kernel interface. As such, the existing
> > attributes cannot really be modified; if anything else would be needed,
> > that would need to be defined as a new attribute and/or command.
> 
> Clarification: you're talking about out-of-tree drivers, which really
> have no relevance in upstream discussion, except possibly as examples.
> I don't think it's ever been a valid approach to dictate upstream
> kernel design based simply on "what $vendor already implemented for
> Android."

There is clearly no requirement to use an existing attribute, but since
there is such an attribute already defined, I'd claim it is perfectly
fine to consider it as an option for this. If something else is
identified to be needed, a new subcmd/attribute can obviously be
defined.

> Maybe it's a better idea to just use different command numbers (or
> vendor ID?) here, to avoid stomping on each others' implementation
> choices. Otherwise, it sounds like our only choice here is to copy
> your Android driver verbatim, or get lost.

I'd use the same OUI 00:13:74 since there is a defined process for
getting identifiers assigned from there for upstream Linux WLAN needs.
Defining other/new non-conflicting subcmds/attributes is of course fine
when needed.

> > This was discussed during the 2019 wireless workshop. The conclusion
> > from that discussion was that while there is clear need for SAR power
> > limits for various devices and multiple vendors/drivers, it did not look
> > clear that a single common interface could be defined cleanly taken into
> > account the differences in the ways vendors have designed the mechanism
> > in driver and firmware implementations. As such, vendor specific
> > commands were identified as the approach.
> 
> [citation needed]

I'm not aware of any publicly available meeting minutes that covered the
details for that discussion. My personal notes indicate that there were
at least two vendors indicating existence of vendor specific commands
for configuring SAR parameters, a discussion about the parameters used
for this being different, and a conclusion that this would be an example
kernel interface where a generic nl80211 interface may not be achievable
and a vendor specific interface would be more likely. This discussion
resulted in the discussion on how to use vendor specific nl80211
commands/attributes in upstream drivers and the eventual documentation
of that in the location you noted.
Brian Norris Dec. 19, 2019, 11:40 p.m. UTC | #5
On Thu, Dec 19, 2019 at 10:55 AM Jouni Malinen <j@w1.fi> wrote:
>
> On Thu, Dec 19, 2019 at 10:32:06AM -0800, Brian Norris wrote:
> > On Thu, Dec 19, 2019 at 7:48 AM Jouni Malinen <j@w1.fi> wrote:
> > > On Thu, Dec 19, 2019 at 09:44:52AM +0000, Pkshih wrote:
> > > > On Wed, 2019-12-18 at 17:48 +0200, Kalle Valo wrote:
> > > > > diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
> > > > > + * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
> > > > > + * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
> > > > > + * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
> > > > > + * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that
> >
> > By the way, I wonder -- why have this statement? That's not how I
> > recall any other piece of kernel development ever working; upstream
> > Linux defines the upstream Linux API, not some arbitrary user space
> > project. This statement could be useful for saying, "don't stomp on
> > those command numbers please," but the response should probably be to
> > go out and define a totally new vendor ID or something. (See below.)
>
> As far as the OUI 00:13:74 is concerned, the defined mechanism for
> getting any new identifier values assigned is by getting a patch applied
> into hostap.git. If another OUI is used, that can clearly use different
> mechanism for this. Anyway, I'd expect the process for OUI 00:13:74 to
> be the most convenient one to use.

OK, I guess that makes a little more sense. But I'm not sure I agree
with the phrase "convenient."

> For 00:13:74, that would mean defining the
> subcmds/attributes first with TBD ID values and once the definitions are
> otherwise fine in the kernel patch review, contributing a patch to
> hostap.git to get the identifiers assigned, updating the kernel patch to
> use the assigned values, and apply it to the kernel.

That doesn't really sound convenient at all. But I suppose that's
beside the point, so I won't harp on that.

> > > Please note that the vendor subcmd and attributes used here are already
> > > deployed and in use as a kernel interface. As such, the existing
> > > attributes cannot really be modified; if anything else would be needed,
> > > that would need to be defined as a new attribute and/or command.
> >
> > Clarification: you're talking about out-of-tree drivers, which really
> > have no relevance in upstream discussion, except possibly as examples.
> > I don't think it's ever been a valid approach to dictate upstream
> > kernel design based simply on "what $vendor already implemented for
> > Android."
>
> There is clearly no requirement to use an existing attribute, but since
> there is such an attribute already defined, I'd claim it is perfectly
> fine to consider it as an option for this. If something else is
> identified to be needed, a new subcmd/attribute can obviously be
> defined.

Ack, thanks. I think I misinterpreted your intention to say, "I won't
take suggestions, because the attributes are already decided
elsewhere."

> > > This was discussed during the 2019 wireless workshop. The conclusion
> > > from that discussion was that while there is clear need for SAR power
> > > limits for various devices and multiple vendors/drivers, it did not look
> > > clear that a single common interface could be defined cleanly taken into
> > > account the differences in the ways vendors have designed the mechanism
> > > in driver and firmware implementations. As such, vendor specific
> > > commands were identified as the approach.
> >
> > [citation needed]
>
> I'm not aware of any publicly available meeting minutes that covered the
> details for that discussion. My personal notes indicate that there were
> at least two vendors indicating existence of vendor specific commands
> for configuring SAR parameters, a discussion about the parameters used
> for this being different, and a conclusion that this would be an example
> kernel interface where a generic nl80211 interface may not be achievable
> and a vendor specific interface would be more likely. This discussion
> resulted in the discussion on how to use vendor specific nl80211
> commands/attributes in upstream drivers and the eventual documentation
> of that in the location you noted.

Hmm, I actually think I was only around for the pre-discussion, in
which y'all suggested you might later meet to decide what eventually
became [1]. So maybe I missed some specific examples that would
provide the [citation] I requested.

That being said, I have personally fielded out-of-tree SAR
implementations from 4 different vendors:

(a) Two of them (this ath10k proposal, roughly; and Realtek's) employ
exactly the same concept: N frequency ranges, each with associated
power limits.
(b) Two of them (Intel/variant-of-iwiwifi and Marvell/mwifiex) utilize
a platform-specific (BIOS or Device Tree) mechanism for enumerating
power tables, and the nl80211 API simply takes an index N (e.g., 0 or
1), so user space can say "switch to mode N"

Unfortunately, for (b), I think there are enough reasons to think they
won't share an API similar to (a) (for Marvell, their
platform-specific tables are large undocumented blobs -- I have a
feeling if we already had a common API for (a), they *could* have
implemented some translation in a nicer way in their driver, but they
haven't chosen to do that work and probably won't be convinced to do
so).

But that still means there's some hope for (a).

Anyway, I am happy that there's a documented policy for vendor APIs
[1], and I'm happy to see this proposal out here. I just want to see a
critical eye put to this particular proposal if possible, to see if we
can improve its flexibility (either now, or in a later version of a
QCA vendor command, or even in a common nl80211-proper command).

So to put a little different spin on Pkshih's request: is there any
value in making this particular ath10k proposal a little more generic
(e.g., more granularity or flexibility in frequency bands, or more
precision in power limits), such that other vendors might implement
the same thing? Or would it be better to let each vendor implement
their similar-looking APIs (i.e., (a); or maybe even (b)) on their
own, and only later look at sharing?

Brian

[1] https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
Kalle Valo March 17, 2020, 4:54 p.m. UTC | #6
Brian Norris <briannorris@chromium.org> writes:

>> > > This was discussed during the 2019 wireless workshop. The conclusion
>> > > from that discussion was that while there is clear need for SAR power
>> > > limits for various devices and multiple vendors/drivers, it did not look
>> > > clear that a single common interface could be defined cleanly taken into
>> > > account the differences in the ways vendors have designed the mechanism
>> > > in driver and firmware implementations. As such, vendor specific
>> > > commands were identified as the approach.
>> >
>> > [citation needed]
>>
>> I'm not aware of any publicly available meeting minutes that covered the
>> details for that discussion. My personal notes indicate that there were
>> at least two vendors indicating existence of vendor specific commands
>> for configuring SAR parameters, a discussion about the parameters used
>> for this being different, and a conclusion that this would be an example
>> kernel interface where a generic nl80211 interface may not be achievable
>> and a vendor specific interface would be more likely. This discussion
>> resulted in the discussion on how to use vendor specific nl80211
>> commands/attributes in upstream drivers and the eventual documentation
>> of that in the location you noted.
>
> Hmm, I actually think I was only around for the pre-discussion, in
> which y'all suggested you might later meet to decide what eventually
> became [1]. So maybe I missed some specific examples that would
> provide the [citation] I requested.
>
> That being said, I have personally fielded out-of-tree SAR
> implementations from 4 different vendors:
>
> (a) Two of them (this ath10k proposal, roughly; and Realtek's) employ
> exactly the same concept: N frequency ranges, each with associated
> power limits.
> (b) Two of them (Intel/variant-of-iwiwifi and Marvell/mwifiex) utilize
> a platform-specific (BIOS or Device Tree) mechanism for enumerating
> power tables, and the nl80211 API simply takes an index N (e.g., 0 or
> 1), so user space can say "switch to mode N"
>
> Unfortunately, for (b), I think there are enough reasons to think they
> won't share an API similar to (a) (for Marvell, their
> platform-specific tables are large undocumented blobs -- I have a
> feeling if we already had a common API for (a), they *could* have
> implemented some translation in a nicer way in their driver, but they
> haven't chosen to do that work and probably won't be convinced to do
> so).
>
> But that still means there's some hope for (a).
>
> Anyway, I am happy that there's a documented policy for vendor APIs
> [1], and I'm happy to see this proposal out here. I just want to see a
> critical eye put to this particular proposal if possible, to see if we
> can improve its flexibility (either now, or in a later version of a
> QCA vendor command, or even in a common nl80211-proper command).
>
> So to put a little different spin on Pkshih's request: is there any
> value in making this particular ath10k proposal a little more generic
> (e.g., more granularity or flexibility in frequency bands, or more
> precision in power limits), such that other vendors might implement
> the same thing? Or would it be better to let each vendor implement
> their similar-looking APIs (i.e., (a); or maybe even (b)) on their
> own, and only later look at sharing?

The downside of accepting SAR vendor commands to upstream is that (in
theory) that should be supported a long time:

  4. The newly proposed API shall be subject to stable API rules.

  https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api

So if sometime in the future we add a generic command the driver would
need to support both vendor and generic commands. So, IF it makes sense
to implement a generic command, I would rather have a generic command
implemented from the beginning and drop the SAR vendor command patches
altogether.

For me either solutions are good enough, I'm not familiar enough with
all the different SAR user space interfaces to make a good decision.
Johannes Berg March 20, 2020, 12:55 p.m. UTC | #7
On Tue, 2020-03-17 at 18:54 +0200, Kalle Valo wrote:

> For me either solutions are good enough, I'm not familiar enough with
> all the different SAR user space interfaces to make a good decision.

Brian probably has most insight into this :-)

I really didn't want to have to be the referee here, I was hoping you'd
figure this all out between yourselves... oh well.

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...


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

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.


But it any case, these two don't seem like an insurmountable issue to
combine? 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?


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?

johannes

Patch
diff mbox series

diff --git a/include/uapi/nl80211-vnd-qca.h b/include/uapi/nl80211-vnd-qca.h
new file mode 100644
index 000000000000..482c9409a2c0
--- /dev/null
+++ b/include/uapi/nl80211-vnd-qca.h
@@ -0,0 +1,68 @@ 
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (c) 2019 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _UAPI_NL80211_VND_QCA_H
+#define _UAPI_NL80211_VND_QCA_H
+
+/* Vendor id to be used in vendor specific command and events to user space
+ * NOTE: The authoritative place for definition of QCA_NL80211_VENDOR_ID,
+ * vendor subcmd definitions prefixed with QCA_NL80211_VENDOR_SUBCMD, and
+ * qca_wlan_vendor_attr is open source file src/common/qca-vendor.h in
+ * git://w1.fi/srv/git/hostap.git; the values here are just a copy of that
+ */
+#define QCA_NL80211_VENDOR_ID 0x001374
+
+/**
+ * enum qca_nl80211_vendor_subcmds - QCA nl80211 vendor command identifiers
+ *
+ *@QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS and is used to retrieve the
+ *	settings currently in use. The attributes returned by this command are
+ *	defined by enum qca_vendor_attr_sar_limits.
+ */
+enum qca_nl80211_vendor_subcmds {
+	QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS = 146,
+	};
+
+/**
+ * enum qca_vendor_attr_sar_limits - Attributes for SAR power limits
+ *
+ * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC: Nested array of SAR power
+ *	limit specifications. The number of specifications is
+ *	specified by @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_NUM_SPECS. Each
+ *	specification contains a set of
+ *	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_* attributes. A
+ *	specification is uniquely identified by the attributes
+ *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND and always
+ *	contains as a payload the attribute
+ *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT.
+ *
+ * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND: Optional (u32) value to
+ *	indicate for which band this specification applies. Valid
+ *	values are enumerated in enum %nl80211_band (although not all
+ *	bands may be supported by a given device). If the attribute is
+ *	not supplied then the specification will be applied to all
+ *	supported bands.
+ *
+ * @QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT: Required (u32)
+ *	value to specify the actual power limit value in units of 0.5
+ *	dBm (i.e., a value of 11 represents 5.5 dBm).
+ *	This is required, when %QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT is
+ *	%QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SELECT_USER.
+ *
+ * These attributes are used with %QCA_NL80211_VENDOR_SUBCMD_SET_SAR_LIMITS
+ * and %QCA_NL80211_VENDOR_SUBCMD_GET_SAR_LIMITS.
+ */
+enum qca_vendor_attr_sar_limits {
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_INVALID = 0,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC = 3,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_BAND = 4,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_SPEC_POWER_LIMIT = 7,
+
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST,
+	QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_MAX =
+		QCA_WLAN_VENDOR_ATTR_SAR_LIMITS_AFTER_LAST - 1
+};
+
+#endif /* _UAPI_NL80211_VND_QCA_H_ */