Received-SPF: pass (google.com: domain of linux-wireless+bounces-4799-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 15 Mar 2024 17:45:15 -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 2/2] wifi: mwifiex: add host mlme for AP mode
Message-ID: <ZfTrm7bxMykHO-M-@google.com>
References: <20240306020053.18054-1-yu-hao.lin@nxp.com>
 <20240306020053.18054-3-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-3-yu-hao.lin@nxp.com>

On Wed, Mar 06, 2024 at 10:00:53AM +0800, David Lin wrote:
> Add host based MLME to enable WPA3 functionalities in AP 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>

Quick pass for now; nothing jumps out at me today, but I'll give a
better look/Ack next week:

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


> @@ -3951,12 +3974,43 @@ mwifiex_cfg80211_tdls_cancel_chan_switch(struct wiphy *wiphy,
>  	}
>  }
>  
> +static int
> +mwifiex_cfg80211_uap_add_station(struct mwifiex_private *priv, const u8 *mac,
> +				 struct station_parameters *params)
> +{
> +	struct mwifiex_sta_info add_sta;
> +	int ret;
> +
> +	memcpy(add_sta.peer_mac, mac, ETH_ALEN);
> +	add_sta.params = params;
> +
> +	ret = mwifiex_send_cmd(priv, HostCmd_CMD_ADD_NEW_STATION,
> +			       HostCmd_ACT_ADD_STA, 0, (void *)&add_sta, true);
> +
> +	if (!ret) {
> +		struct station_info *sinfo;
> +
> +		sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);

Couldn't this just be stack allocation?

		struct staion_info sinfo;

		cfg80211_new_sta(priv->netdev, mac, &sinfo, GFP_KERNEL);

I'm not sure you need to kzalloc() something here, if you're freeing it
a few lines later.


> +		if (!sinfo)
> +			return -ENOMEM;
> +
> +		cfg80211_new_sta(priv->netdev, mac, sinfo, GFP_KERNEL);
> +		kfree(sinfo);
> +	}
> +
> +	return ret;
> +}

Brian