| Message ID | 1576684108-30177-2-git-send-email-kvalo@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | ath10k: SAR power limit vendor command | expand |
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_ */
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.
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.
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.
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
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.
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
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
Brian Norris <briannorris@chromium.org> writes: > 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. So to me it feels like the best solution forward is to go with the vendor API, do you agree? We can, of course, later switch to the common API if we manage to create one which is usable for everyone. > 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. Yeah, that sounds like a good idea but I don't think that should block these patches.
On Thu, Jul 16, 2020 at 2:35 AM Kalle Valo <kvalo@codeaurora.org> wrote: > Brian Norris <briannorris@chromium.org> writes: > > 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. > > So to me it feels like the best solution forward is to go with the > vendor API, do you agree? We can, of course, later switch to the common > API if we manage to create one which is usable for everyone. That's fine with me. That's pretty much what I said in my first email: "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." I think we've had some healthy (though very protracted) discussion, and I don't think I've seen anyone bring up anything I wasn't already aware of that would prevent eventual consolidation. As long as we acknowledge those things (item 2 at https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api), I'm happy. > > 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. > > Yeah, that sounds like a good idea but I don't think that should block > these patches. OK, well it was your concern first, IIRC :) So what's next? A v2 that only needs a bit of updated description about "why a vendor API"? And Realtek can feel free to submit this [1] shortly? Thanks, Brian [1] Series: rtw88: Add SAR implementation https://patchwork.kernel.org/project/linux-wireless/list/?series=238219&state=*
Brian Norris <briannorris@chromium.org> writes: > On Thu, Jul 16, 2020 at 2:35 AM Kalle Valo <kvalo@codeaurora.org> wrote: >> Brian Norris <briannorris@chromium.org> writes: >> > 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. >> >> So to me it feels like the best solution forward is to go with the >> vendor API, do you agree? We can, of course, later switch to the common >> API if we manage to create one which is usable for everyone. > > That's fine with me. That's pretty much what I said in my first email: > > "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." > > I think we've had some healthy (though very protracted) discussion, > and I don't think I've seen anyone bring up anything I wasn't already > aware of that would prevent eventual consolidation. As long as we > acknowledge those things (item 2 at > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api), > I'm happy. Good, I was just checking that we all are on the same page. >> > 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. >> >> Yeah, that sounds like a good idea but I don't think that should block >> these patches. > > OK, well it was your concern first, IIRC :) I was commenting about the "check for support" feature :) I think it would be a good idea to have userspace check what vendor interface features/commands are supported with that driver/hardware/firmware combo. But how should that be implemented? Should there be a some kind of generic mechanism used by all drivers or would each driver have their own method to check the supported features? I think that needs a separate discussion. > So what's next? A v2 that only needs a bit of updated description > about "why a vendor API"? I'm busy but hopefully Wen (CCed) can submit v2. Wen? > And Realtek can feel free to submit this [1] shortly? > [1] Series: rtw88: Add SAR implementation > https://patchwork.kernel.org/project/linux-wireless/list/?series=238219&state=* Yeah, please submit that as well.
On Mon, 2020-06-01 at 18:32 -0700, Brian Norris wrote: > I haven't quite reached the 3 month lag on the prior replies here :) But I did, almost ;-) > 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. Right. > 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?) Yes, but only a short 1.5hr video call I guess. I'm not even sure what to do there yet. [snip] Thanks for the overview! > 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. We do that? We list out the available vendor APIs, and in most cases (perhaps not the one you've noticed there) we also list out the availability of real nl80211 APIs in some way. johannes
On Fri, 2020-07-24 at 12:26 +0300, Kalle Valo wrote: > > > So to me it feels like the best solution forward is to go with the > > > vendor API, do you agree? We can, of course, later switch to the common > > > API if we manage to create one which is usable for everyone. But why wouldn't we try that now, while we have it all in our heads (in a way ... even if this discussion drags out forever)? I mean, the range-based approach ought to work, and if we define it as a nested attribute list or so, we can even later add more attributes to it (chain limits, whatnot) without any backward compatibility concerns. So what is it that we _cannot_ do in a more common way today? > > I think we've had some healthy (though very protracted) discussion, > > and I don't think I've seen anyone bring up anything I wasn't already > > aware of that would prevent eventual consolidation. As long as we > > acknowledge those things (item 2 at > > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api), > > I'm happy. > > Good, I was just checking that we all are on the same page. But are we? ;-) I don't really see anything in the new proposal [1] that really explains why the common API that we've sort of vaguely outlined in this thread couldn't work? It just speaks of technical difficulties ("need a reporting API too"), but should we let that stop us? [1] https://patchwork.kernel.org/patch/11686317/ johannes
On Thu, Jul 30, 2020 at 6:24 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > Good, I was just checking that we all are on the same page. > > But are we? ;-) I think you were deferring to, "how would user space use it?" And, "would a common API really help anyone?" (And then you implied "anyone" = "Chrome OS.") I expressed a moderate benefit from a common API, but I'd settle for a non-common (but upstream) one instead. I also doubt many non-hardware-customized Linux distributions will make use of this any time soon. I don't think that implied you were truly on a different page. But your response below may say otherwise: > I don't really see anything in the new proposal [1] that really explains > why the common API that we've sort of vaguely outlined in this thread > couldn't work? It just speaks of technical difficulties ("need a > reporting API too"), but should we let that stop us? > > [1] https://patchwork.kernel.org/patch/11686317/ Indeed, I don't see any reason beyond technical difficulties. I'd love to have a few extra hours in my day to spend on writing such an API, if that would really unblock months of deadlock. Unfortunately, those hours tend to get eaten by all sorts of other things these days, so an honest assessment would probably say I won't get around to it soon. SAR regulatory concerns aren't really sexy enough to convince me to spend my weekends on it... (...oh wait, it's Friday evening already. Hmm.) Regards, Brian
Johannes Berg <johannes@sipsolutions.net> writes: > On Fri, 2020-07-24 at 12:26 +0300, Kalle Valo wrote: > >> > > So to me it feels like the best solution forward is to go with the >> > > vendor API, do you agree? We can, of course, later switch to the common >> > > API if we manage to create one which is usable for everyone. > > But why wouldn't we try that now, while we have it all in our heads (in > a way ... even if this discussion drags out forever)? > > I mean, the range-based approach ought to work, and if we define it as a > nested attribute list or so, we can even later add more attributes to it > (chain limits, whatnot) without any backward compatibility concerns. > > So what is it that we _cannot_ do in a more common way today? > >> > I think we've had some healthy (though very protracted) discussion, >> > and I don't think I've seen anyone bring up anything I wasn't already >> > aware of that would prevent eventual consolidation. As long as we >> > acknowledge those things (item 2 at >> > https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api), >> > I'm happy. >> >> Good, I was just checking that we all are on the same page. > > But are we? ;-) > > I don't really see anything in the new proposal [1] that really explains > why the common API that we've sort of vaguely outlined in this thread > couldn't work? It just speaks of technical difficulties ("need a > reporting API too"), but should we let that stop us? > > [1] https://patchwork.kernel.org/patch/11686317/ I misunderstood then, I thought everyone were leaning towards the vendor API approach. But yeah, of course a common API is much better if people think it's doable. So I'll now drop all the vendor API patches from patchwork and assume/hope that we will get the common API at some point.
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_ */