Received-SPF: pass (google.com: domain of linux-wireless+bounces-4797-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 15 Mar 2024 17:00:11 -0700
From: Brian Norris <briannorris@chromium.org>
To: David Lin <yu-hao.lin@nxp.com>
Cc: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvalo@kernel.org, francesco@dolcini.it, tsung-hsien.hsieh@nxp.com,
	Francesco Dolcini <francesco.dolcini@toradex.com>
Subject: Re: [PATCH v9 1/2] wifi: mwifiex: add host mlme for client mode
Message-ID: <ZfThCwGj-P5Owlsn@google.com>
References: <20240306020053.18054-1-yu-hao.lin@nxp.com>
 <20240306020053.18054-2-yu-hao.lin@nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240306020053.18054-2-yu-hao.lin@nxp.com>

Hi David,

Thanks for the persistence here (and thanks Francesco for all the review
help). I think things are mostly well structured here, but I'll also
admit it's a pretty large bit of work to review at once. So please bear
with me if it takes a bit of time to really get through it. I'll post a
few thoughts now, but it's possible I'll have more after another pass.

On Wed, Mar 06, 2024 at 10:00:52AM +0800, David Lin wrote:
> Add host based MLME to enable WPA3 functionalities in client mode.
> This feature required a firmware with the corresponding V2 Key API
> support. The feature (WPA3) is currently enabled and verified only
> on IW416. Also, verified no regression with change when host MLME
> is disabled.
> 
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> 
> v9:
>    - remove redundent code.
> 
> v8:
>    - first full and complete patch to support host based MLME for client
>      mode.
> 
> ---
>  .../net/wireless/marvell/mwifiex/cfg80211.c   | 315 ++++++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c |  25 ++
>  drivers/net/wireless/marvell/mwifiex/decl.h   |  22 ++
>  drivers/net/wireless/marvell/mwifiex/fw.h     |  33 ++
>  drivers/net/wireless/marvell/mwifiex/init.c   |   6 +
>  drivers/net/wireless/marvell/mwifiex/join.c   |  66 +++-
>  drivers/net/wireless/marvell/mwifiex/main.c   |  54 +++
>  drivers/net/wireless/marvell/mwifiex/main.h   |  16 +
>  drivers/net/wireless/marvell/mwifiex/scan.c   |   6 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c   |  13 +
>  drivers/net/wireless/marvell/mwifiex/sdio.h   |   2 +
>  .../net/wireless/marvell/mwifiex/sta_event.c  |  36 +-
>  .../net/wireless/marvell/mwifiex/sta_ioctl.c  |   2 +-
>  drivers/net/wireless/marvell/mwifiex/sta_tx.c |   9 +-
>  drivers/net/wireless/marvell/mwifiex/util.c   |  80 +++++
>  15 files changed, 671 insertions(+), 14 deletions(-)

(Per the above, I'd normally consider whether ~671 new lines is worth
splitting into multiple patches. But I don't see any great logical ways
to do that.)

> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index b909a7665e9c..bcf4f87dcaab 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c

> @@ -4204,6 +4210,302 @@ mwifiex_cfg80211_change_station(struct wiphy *wiphy, struct net_device *dev,
>  	return ret;
>  }
>  
> +static int
> +mwifiex_cfg80211_authenticate(struct wiphy *wiphy,
> +			      struct net_device *dev,
> +			      struct cfg80211_auth_request *req)
> +{
> +	struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
> +	struct mwifiex_adapter *adapter = priv->adapter;
> +	struct sk_buff *skb;
> +	u16 pkt_len, auth_alg;
> +	int ret;
> +	struct mwifiex_ieee80211_mgmt *mgmt;
> +	struct mwifiex_txinfo *tx_info;
> +	u32 tx_control = 0, pkt_type = PKT_TYPE_MGMT;
> +	u8 addr[ETH_ALEN] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};


> +	memcpy(mgmt->addr4, addr, ETH_ALEN);

	eth_broadcast_addr(mgmt->addr4);

> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c

> @@ -1558,6 +1596,14 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter)
>  		INIT_WORK(&adapter->rx_work, mwifiex_rx_work_queue);
>  	}
>  
> +	adapter->host_mlme_workqueue =
> +		alloc_workqueue("MWIFIEX_HOST_MLME_WORK_QUEUE",
> +				WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND, 0);

Hmm, why do you need a whole new workqueue? This driver is already full
of race conditions, while many race conditions are avoided simply
because most work is sequentialized onto the main work queue. If you
don't have a good reason here, I'd probably prefer you share the
existing queue.

Or otherwise, if this is *truly* independent and race-free, do you
actually need to create a new queue? We could just use schedule_work(),
which uses the system queue.

If you do really need it, is it possible to key off 'host_mlme_enabled'
or similar? There's no need to allocate the queue if we're not using it.

> +	if (!adapter->host_mlme_workqueue)
> +		goto err_kmalloc;
> +
> +	INIT_WORK(&adapter->host_mlme_work, mwifiex_host_mlme_work_queue);
> +
>  	/* Register the device. Fill up the private data structure with
>  	 * relevant information from the card. Some code extracted from
>  	 * mwifiex_register_dev()


> --- a/drivers/net/wireless/marvell/mwifiex/util.c
> +++ b/drivers/net/wireless/marvell/mwifiex/util.c
> @@ -370,6 +370,46 @@ mwifiex_parse_mgmt_packet(struct mwifiex_private *priv, u8 *payload, u16 len,
>  
>  	return 0;
>  }
> +
> +/* This function sends deauth packet to the kernel. */
> +void mwifiex_host_mlme_disconnect(struct mwifiex_private *priv,
> +				  u16 reason_code, u8 *sa)
> +{
> +	u8 broadcast_addr[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> +	u8 frame_buf[100];
> +	struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)frame_buf;
> +
> +	memset(frame_buf, 0, sizeof(frame_buf));
> +	mgmt->frame_control = (__force __le16)IEEE80211_STYPE_DEAUTH;

Hmm, "__force" is a pretty good sign that you're doing something wrong.
Please think twice before using it.

I believe the right answer here is cpu_to_le16(). It's a no-op on little
endian architectures, but it'll make big-endian work.

> +	mgmt->duration = 0;
> +	mgmt->seq_ctrl = 0;
> +	mgmt->u.deauth.reason_code = (__force __le16)reason_code;

Same here.

> +
> +	if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA) {
> +		memcpy(mgmt->da, broadcast_addr, ETH_ALEN);

		eth_broadcast_addr(mgmt->da);

> +		memcpy(mgmt->sa,
> +		       priv->curr_bss_params.bss_descriptor.mac_address,
> +		       ETH_ALEN);
> +		memcpy(mgmt->bssid, priv->cfg_bssid, ETH_ALEN);
> +		priv->auth_flag = 0;
> +		priv->auth_alg = WLAN_AUTH_NONE;


> @@ -417,6 +457,46 @@ mwifiex_process_mgmt_packet(struct mwifiex_private *priv,
>  	pkt_len -= ETH_ALEN;
>  	rx_pd->rx_pkt_length = cpu_to_le16(pkt_len);
>  
> +	if (priv->host_mlme_reg &&
> +	    (GET_BSS_ROLE(priv) != MWIFIEX_BSS_ROLE_UAP) &&
> +	    (ieee80211_is_auth(ieee_hdr->frame_control) ||
> +	     ieee80211_is_deauth(ieee_hdr->frame_control) ||
> +	     ieee80211_is_disassoc(ieee_hdr->frame_control))) {
> +		if (ieee80211_is_auth(ieee_hdr->frame_control)) {
> +			if (priv->auth_flag & HOST_MLME_AUTH_PENDING) {
> +				if (priv->auth_alg != WLAN_AUTH_SAE) {
> +					priv->auth_flag &=
> +						~HOST_MLME_AUTH_PENDING;
> +					priv->auth_flag |=
> +						HOST_MLME_AUTH_DONE;
> +				}
> +			} else {
> +				return 0;
> +			}
> +
> +			mwifiex_dbg(priv->adapter, MSG,
> +				    "auth: receive authentication from %pM\n",
> +				    ieee_hdr->addr3);
> +		} else {
> +			if (!priv->wdev.connected)
> +				return 0;
> +
> +			if (ieee80211_is_deauth(ieee_hdr->frame_control)) {
> +				mwifiex_dbg(priv->adapter, MSG,
> +					    "auth: receive deauth from %pM\n",
> +					    ieee_hdr->addr3);
> +				priv->auth_flag = 0;
> +				priv->auth_alg = WLAN_AUTH_NONE;
> +			} else {
> +				mwifiex_dbg(priv->adapter, MSG,
> +					    "assoc: receive disasso from %pM\n",

I get that sometimes abbreviations are nice, but perhaps at least use a
consistent one? I see "disassoc" elsewhere.

> +					    ieee_hdr->addr3);

Brian