RE: [EXT] Re: [PATCH v10 2/2] wifi: mwifiex: add host mlme for AP mode

[David Lin]

> 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