On Tue, Aug 01, 2023 at 05:50:03AM +0000, David Lin wrote: > > > From: Brian Norris <briannorris@xxxxxxxxxxxx> > > 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@xxxxxxx> 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@xxxxxxx> > > > > > 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@xxxxxxxxxxxxxx/ 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
