Re: [EXT] Re: [PATCH] wifi: mwifiex: added code to support host mlme.

[Brian Norris <briannorris@chromium.org>]

On Tue, Aug 01, 2023 at 05:50:03AM +0000, David Lin wrote:
> 
> > From: Brian Norris <briannorris@chromium.org>
> > Wait, your company can't afford to have anyone respond to maintainer mail
> > for years [1], but you can afford to add new features? Crazy.
> >
> 
> This feature is needed for WPA3.

Yeah, I read the description.

> > On Thu, Jul 27, 2023 at 11:19 PM David Lin <yu-hao.lin@nxp.com> wrote:
> > >
> > > 1. For station mode first.
> > > 2. This feature is a must for WPA3.
> > > 3. The code is tested with IW416. There is no guarantee for other chips.
> >
> > ^^ That's not a good sign.
> >
> > > Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> >
> > >  drivers/net/wireless/marvell/mwifiex/util.c   |  74 ++++
> > >  14 files changed, 558 insertions(+), 13 deletions(-)
> >
> > > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > > @@ -28,6 +28,10 @@ module_param(driver_mode, ushort, 0);
> > > MODULE_PARM_DESC(driver_mode,
> > >                  "station=0x1(default), ap-sta=0x3, station-p2p=0x5,
> > > ap-sta-p2p=0x7");
> > >
> > > +bool host_mlme;
> > > +module_param(host_mlme, bool, 0);
> > > +MODULE_PARM_DESC(host_mlme, "Host MLME support enable:1,
> > disable:0");
> > > +
> >
> > I hear Kalle doesn't like module parameters like this. They're a cop out on
> > properly supporting features (also, see your own commit message). I'd have to
> > dig through the archives to find the latest advice and rules on this.
> >
> > Overall, I'm not enthusiastic about this change.
> 
> The parameter 'host_mlme' is added to protect original code. It will be disabled as default.

Right, I read the code too.

The point is, module parameters (or debugfs files) for controlling core
protocol functionality are highly discouraged here. See the following,
for some additional notes about this:

https://lore.kernel.org/linux-wireless/87d09u7tyr.fsf@codeaurora.org/
Subject: Re: [PATCH] rtw88: disable TX-AMSDU on 2.4G band

I really need to work on writing this up for the wiki...

On a constructive note: why do you want the module parameter at all?
Because you don't trust the code at all? Because you don't trust it for
the chips you haven't tested? Because you you don't trust it for the
firmware version(s) you haven't tested?

If you don't trust the code at all, don't except us to merge your patch.

If you don't trust it for certain chips or firmware versions, then
detect those at runtime to properly disable the feature. (And, I highly
suspect that not all firmware versions will support this. Don't make the
user guess.)

Basically, for the cases you care about enabling a new feature on for
production use, it shouldn't require playing with module parameters.

Side note: I think you probably shouldn't be advertising things like
NL80211_FEATURE_SAE with this feature disabled; that'll likely confuse
user space into thinking it can try WPA3, when it'll just fail as soon
as they try it.

Brian