Re: [PATCH 1/2] nl80211: vendor-cmd: qca: add dynamic SAR power limits

[Brian Norris <briannorris@chromium.org>]

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.