> From: Brian Norris <briannorris@xxxxxxxxxxxx>
> Sent: Thursday, May 23, 2024 8:59 AM
> To: David Lin <yu-hao.lin@xxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete Hsieh
> <tsung-hsien.hsieh@xxxxxxx>; Francesco Dolcini
> <francesco.dolcini@xxxxxxxxxxx>
> Subject: [EXT] Re: [PATCH v10 2/2] wifi: mwifiex: add host mlme for AP mode
>
> 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
>
>
> Hi,
>
> Hopefully-last comment for patch 2:
>
> On Thu, Apr 18, 2024 at 02:06:26PM +0800, David Lin wrote:
> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
>
> > @@ -1712,7 +1735,7 @@ static const u32 mwifiex_cipher_suites[] = { };
> >
> > /* Supported mgmt frame types to be advertised to cfg80211 */ -static
> > const struct ieee80211_txrx_stypes
> > +static struct ieee80211_txrx_stypes
> > mwifiex_mgmt_stypes[NUM_NL80211_IFTYPES] = {
> > [NL80211_IFTYPE_STATION] = {
> > .tx = BIT(IEEE80211_STYPE_ACTION >> 4) |
> ...
> > + if (adapter->host_mlme_enabled) {
> > + mwifiex_mgmt_stypes[NL80211_IFTYPE_AP].tx = 0xffff;
> > + mwifiex_mgmt_stypes[NL80211_IFTYPE_AP].rx =
> > + BIT(IEEE80211_STYPE_ASSOC_REQ >> 4) |
> > + BIT(IEEE80211_STYPE_REASSOC_REQ >> 4) |
> > + BIT(IEEE80211_STYPE_PROBE_REQ >> 4) |
> > + BIT(IEEE80211_STYPE_DISASSOC >> 4) |
> > + BIT(IEEE80211_STYPE_AUTH >> 4) |
> > + BIT(IEEE80211_STYPE_DEAUTH >> 4) |
> > + BIT(IEEE80211_STYPE_ACTION >> 4);
> > + }
>
> This doesn't look like a sound approach. I think you should keep
> 'mwifiex_mgmt_stypes' const, because if you're making modifications to it,
> then you may break multi-adapter situations. Consider someone loads this
> driver with host_mlme_enabled==true, and then later registers a device with
> host_mlme_enabled==false. The second adapter will get the wrong values.
>
> I think 'mwifiex_mgmt_stypes' is small enough you might as well just make a
> second copy with the MLME-enabled values, rather than fiddling with
> modifying a single copy.
>
> Aside from that:
>
> Acked-by: Brian Norris <briannorris@xxxxxxxxxxxx>
>
> (Feel free to carry that to a v11, since my only remaining substantial concerns
> are with patch 1 I think.)
>
> Brian
I will modify mwifiex_mgmt_stypes for patch v11 and carry your "Acked-by" tag.
David
