Received-SPF: pass (google.com: domain of linux-wireless+bounces-5754-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 2 Apr 2024 10:38:38 -0700
From: Brian Norris <briannorris@chromium.org>
To: David Lin <yu-hao.lin@nxp.com>,
	Johannes Berg <johannes@sipsolutions.net>
Cc: Johannes Berg <johannes@sipsolutions.net>,
	Francesco Dolcini <francesco@dolcini.it>,
	"kvalo@kernel.org" <kvalo@kernel.org>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Pete Hsieh <tsung-hsien.hsieh@nxp.com>,
	"rafael.beims" <rafael.beims@toradex.com>,
	Francesco Dolcini <francesco.dolcini@toradex.com>
Subject: Re: [EXT] Re: [PATCH v9 0/2] wifi: mwifiex: add code to support host
 mlme
Message-ID: <ZgxCngq_Rguc4qs8@google.com>
References: <ZfTspRKFgrO9xCTH@google.com>
 <969e95ccc4a1d35b45212b7fcb536ee90995e3b5.camel@sipsolutions.net>
 <PA4PR04MB9638D253189D6DD330B198B2D1332@PA4PR04MB9638.eurprd04.prod.outlook.com>
 <PA4PR04MB9638BE73DDBCE1CE8AA32BA8D1332@PA4PR04MB9638.eurprd04.prod.outlook.com>
 <b2d9a7ef53c5ab4212617e8edf202bbafe52e2f8.camel@sipsolutions.net>
 <ZftaJEIeNfV7YrVo@google.com>
 <PA4PR04MB9638F5037D1AB9BCF51CC9D9D1322@PA4PR04MB9638.eurprd04.prod.outlook.com>
 <Zf4rDifM6bLuqpX2@google.com>
 <4e5f3741819e457c5c79d825c6520cb9ee531b95.camel@sipsolutions.net>
 <PA4PR04MB96386917877832602F221282D13A2@PA4PR04MB9638.eurprd04.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PA4PR04MB96386917877832602F221282D13A2@PA4PR04MB9638.eurprd04.prod.outlook.com>

Hi Johannes and David,

On Fri, Mar 29, 2024 at 10:06:19AM +0000, David Lin wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > 
> > The way I see it, the issue here isn't necessarily the fact that this uses the
> > auth command (and then requires assoc, of course), but that we see here
> > this is "growing" towards a more mac80211-like model, with the code
> > duplication (albeit little that it is today) implied by that. To me, it seems like
> > the firmware is moving into the "oh we can't do all _that_ in firmware"
> > territory, and that brings it closer to mac80211.
> > 
> > At the same time, as you say, mac80211 is doing more and more offload
> > capability, so it seems like apart from "today the firmware requires an assoc
> > command rather than assoc frame processing in the host", it's actually not
> > _that_ far apart any more!
> > 
> > Now that may be an issue in the short term, but I wouldn't be surprised at
> > all if desiring to implement FILS and other new features in this space would
> > make the driver move to assoc frame processing in the host as well, because
> > it's getting more and more complex, just like auth.
> > 
> > At which point - yeah the APIs are still significantly different, but again we'd
> > end up implementing something that exists in mac80211 today and taking it
> > into mwifiex?

So it seems there's 2 possible sticking points: code duplication, and
overall trend in the specs and implementation that might increase
duplication?

To me, it seems like the duplication is minimal today, or at least, not
much as a result of anything in this patch proposal. There's some
repetition of 802.11 definitions already, but that's probably
orthogonal.

I have less understanding and foresight on the "trend" questions,
although David seems to somewhat agree in his response below -- that NXP
intends to handle modern security features in the host more and more,
which could indeed mean a bit more framing-related duplication.

> Mwifiex is designed based on a "Thick FW" architecture. 
> As security features become more complicated, this patch adds WPA3 support by offloading to wpa_supplicant/hostapd
> With this patch, auth, assoc and key handshakes are handled in wpa_supplicant/hostapd. 
> Wpa_supplicant communicates peer capability and derived keys to driver/FW via cfg80211 assoc and add_key commands.   
> The cfg80211 commands are standard. Implementation between driver and firmware is vendor specific. It shall not break any existing architecture.
> 
> > > It may be harder to add
> > > future additions to the mac80211 stack [*], if we have to add new
> > > concerns of a non-mac80211 implementation in the mix.
> > 
> > Not sure that makes a difference for mac80211 in itself, obviously changes in
> > this space would then affect mwifiex, but that shouldn't be much of an
> > issue.
> > 
> 
> With this patch Mwifiex still a non-mac80211 implementation. 
> Driver communicates with wpa_supplicant/hostapd via cfg80211 
> I think how driver/FW communicate each other is proprietary, I don't see a dependency with mac80211 here

David, I may have pointed in the wrong direction by claiming "conflict"
with mac80211. I believe Johannes's concerns are in code duplication.
Pretty much all other actively-maintained Linux WiFi drivers are based
on mac80211, so that we don't all have to implement the same frame
construction and parsing code. mwifiex is somewhat of an outlier in
actively adding new features while remaining a cfg80211-only driver.

But for myself, I'm not even fully convinced mac80211-style stuff makes
sense here. Even just looking at the auth() stuff in patch 1, this
driver is far from a thin layer that allows mac80211 to handle the
802.11 details. Just look at the 'priv->host_mlme_reg' and
HostCmd_CMD_MGMT_FRAME_REG stuff -- it seems that the simple act of
sending a single 802.11 frame requires opting into some FW-specific
command mask. This feels "thick", like David mentioned above.

> > I'm less worried about this individual patch than what it says about the
> > direction this driver and firmware are taking, and I fear we'll end up in a
> > situation where over time this driver actually gets to a point where it should
> > be using mac80211, but because it's such a piece-meal affair (auth frames
> > now, etc.) and large architectural change, they'd never actually do that.
> > 
> > To be fair, that might also require firmware API changes in some way. I used
> > to think that was something we should never require, but I'm not so sure
> > now any more - certainly we've changed our (Intel) FW API in support of
> > Linux architecture many times, and overall that's for a better product (on
> > Linux at least.)

Yeah, I feel like I see hints of stuff (in public driver code, and in
internal discussions with vendors) where things really work best in
mac80211 land if firmware is developed with *some* consideration for the
mac80211 Linux architecture (or else already is a very thin firmware
from the start). I don't feel like Marvell/NXP has that consideration at
all, and so trying to drag its firmware into mac80211 regardless would
bring its own unique pain. But that's just a lightly-educated feeling.

> > Also: David, I'd appreciate if you actually took this discussion seriously; so
> > far you've not really contributed any technical arguments.
> 
> I think we are using standard cfg80211 commands. How it's handled
> between driver/FW is proprietary, it's carefully verified and shall
> not impact other features or break any architecture. 

David, repeating the "carefully verified" stuff doesn't really help the
subject at hand, and it's not really a technical argument either, when
it's not accompanied by any proofs. Being careful and running a good QA
cycle are excellent and appreciated, but that's not what we're talking
about any more. We're trying to understand what the firmware
architecture is like, and what driver architecture matches your future
development best, with the needs of the Linux community in mind.

(Now, as an example useful argument: if you claimed that implementing a
mac80211 driver with a generalized ieee80211_ops::tx() function would
introduce unvetted combinations of control logic that cannot be
reconciled with your HostCmd protocols, or that QA can't properly vet
that ... well, that would be a start of a technical argument. But it
would require more than a sentence or two to describe why that's
impossible or difficult.)

> We do not see a need why driver has to be redesigned based on
> mac80211. We can keep adding new features using cfg80211.

All in all, other than being a little grumpy about reviewing new
features here, I'm still leaning toward David's statement here -- that I
don't see why it *must* be rewritten toward mac80211. But I still defer
to Johannes, and I'm just trying to be a bit of a referee, or the
proverbial "devil's advocate".

Brian