> From: Brian Norris <briannorris@xxxxxxxxxxxx> > Sent: Wednesday, August 2, 2023 1:40 AM > To: David Lin <yu-hao.lin@xxxxxxx> > Cc: linux-wireless@xxxxxxxxxxxxxxx; Sharvari Harisangam > <sharvari.harisangam@xxxxxxx>; Pete Hsieh <tsung-hsien.hsieh@xxxxxxx> > Subject: Re: [EXT] Re: [PATCH] wifi: mwifiex: added code to support host mlme. > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > 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.kern/ > el.org%2Flinux-wireless%2F87d09u7tyr.fsf%40codeaurora.org%2F&data=05%7C > 01%7Cyu-hao.lin%40nxp.com%7Cd77f381b70454d7b690208db92b656ea%7C6 > 86ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638265084070163156%7C > Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI > 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=0yFba5Gg9tig6Hq > w6ocPsowITivOaPhSLNpVtjWVKp0%3D&reserved=0 > 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. I will remove module parameter "host_mlme" in PATCH V2. I will also fix NL80211_FEATURE_SAE advertisement issue. Thanks for your suggestions. > > Brian
