2024-05-23 00:59:32

by Brian Norris

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

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 <[email protected]>

(Feel free to carry that to a v11, since my only remaining substantial
concerns are with patch 1 I think.)

Brian

2024-05-23 02:20:44

by David Lin

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

> From: Brian Norris <[email protected]>
> Sent: Thursday, May 23, 2024 8:59 AM
> To: David Lin <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Pete Hsieh
> <[email protected]>; Francesco Dolcini
> <[email protected]>
> 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 <[email protected]>
>
> (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