Re: [PATCH] wifi: mwifiex: fix firmware crash for AP DFS mode

[Francesco Dolcini <francesco@dolcini.it>]

+Lucas (in case he missed this patch)

On Fri, Aug 30, 2024 at 04:07:19PM +0800, David Lin wrote:
> Firmware crashes when AP works on a DFS channel and radar detection occurs.
> This patch fixes the issue, also add "fake_radar_detect" entry to mimic
> radar detection for testing purpose.

Do we want such kind of "fake" code in the driver?

I do not agree that we mix an actual bug fix with additional testing code,
and if I understand correctly the commit message this is what we are doing
here.

BTW, I think you should elaborate more in the commit message
"This patch fixes the issue" to allow this patch to be reviewed.

With that said I had a quick look at the patch, I think that those points need
to be clarified before I can look more into it.

> 
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/11h.c    | 56 +++++++++++++++----
>  .../net/wireless/marvell/mwifiex/cfg80211.c   | 50 ++++++++---------
>  .../net/wireless/marvell/mwifiex/cfg80211.h   |  4 +-
>  .../net/wireless/marvell/mwifiex/debugfs.c    | 42 ++++++++++++++
>  drivers/net/wireless/marvell/mwifiex/decl.h   |  1 +
>  drivers/net/wireless/marvell/mwifiex/main.h   |  1 +
>  6 files changed, 115 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11h.c b/drivers/net/wireless/marvell/mwifiex/11h.c
> index b90f922f1cdc..e7e7a154831f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11h.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11h.c
> @@ -7,7 +7,7 @@
>  
>  #include "main.h"
>  #include "fw.h"
> -
> +#include "cfg80211.h"
>  
>  void mwifiex_init_11h_params(struct mwifiex_private *priv)
>  {
> @@ -220,8 +220,11 @@ int mwifiex_11h_handle_chanrpt_ready(struct mwifiex_private *priv,
>  				cancel_delayed_work_sync(&priv->dfs_cac_work);
>  				cfg80211_cac_event(priv->netdev,
>  						   &priv->dfs_chandef,
> -						   NL80211_RADAR_DETECTED,
> +						   NL80211_RADAR_CAC_ABORTED,
>  						   GFP_KERNEL);
> +				cfg80211_radar_event(priv->adapter->wiphy,
> +						     &priv->dfs_chandef,
> +						     GFP_KERNEL);
>  			}
>  			break;
>  		default:
> @@ -244,9 +247,16 @@ int mwifiex_11h_handle_radar_detected(struct mwifiex_private *priv,
>  
>  	mwifiex_dbg(priv->adapter, MSG,
>  		    "radar detected; indicating kernel\n");
> -	if (mwifiex_stop_radar_detection(priv, &priv->dfs_chandef))
> -		mwifiex_dbg(priv->adapter, ERROR,
> -			    "Failed to stop CAC in FW\n");
> +
> +	if (priv->wdev.cac_started) {
> +		if (mwifiex_stop_radar_detection(priv, &priv->dfs_chandef))
> +			mwifiex_dbg(priv->adapter, ERROR,
> +				    "Failed to stop CAC in FW\n");
> +		cancel_delayed_work_sync(&priv->dfs_cac_work);
> +		cfg80211_cac_event(priv->netdev, &priv->dfs_chandef,
> +				   NL80211_RADAR_CAC_ABORTED, GFP_KERNEL);
> +	}
> +
>  	cfg80211_radar_event(priv->adapter->wiphy, &priv->dfs_chandef,
>  			     GFP_KERNEL);
>  	mwifiex_dbg(priv->adapter, MSG, "regdomain: %d\n",
> @@ -267,27 +277,53 @@ void mwifiex_dfs_chan_sw_work_queue(struct work_struct *work)
>  	struct mwifiex_uap_bss_param *bss_cfg;
>  	struct delayed_work *delayed_work = to_delayed_work(work);
>  	struct mwifiex_private *priv =
> -			container_of(delayed_work, struct mwifiex_private,
> -				     dfs_chan_sw_work);
> +		container_of(delayed_work, struct mwifiex_private,
> +			     dfs_chan_sw_work);
> +	struct mwifiex_adapter *adapter = priv->adapter;
> +
> +	if (mwifiex_del_mgmt_ies(priv))
> +		mwifiex_dbg(adapter, ERROR,
> +			    "Failed to delete mgmt IEs!\n");
>  
>  	bss_cfg = &priv->bss_cfg;
>  	if (!bss_cfg->beacon_period) {
> -		mwifiex_dbg(priv->adapter, ERROR,
> +		mwifiex_dbg(adapter, ERROR,
>  			    "channel switch: AP already stopped\n");
This change of adding `struct mwifiex_adapter *adapter` and refactoring the
related code is 100% fine, but mixing it here is just making the review work
more complex.

> +
> +	if (priv->uap_stop_tx) {
> +		if (!netif_carrier_ok(priv->netdev))

is this if needed? why? can't you just call netif_carrier_on() in every case?

> +			netif_carrier_on(priv->netdev);


> +		mwifiex_wake_up_net_dev_queue(priv->netdev, adapter);
> +		priv->uap_stop_tx = false;
> +	}
>  }
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 722ead51e912..c5e8f12da0ae 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -1892,6 +1882,20 @@ static int mwifiex_cfg80211_change_beacon(struct wiphy *wiphy,
>  	return 0;
>  }
>  
> +/* cfg80211 operation handler for change_beacon.
> + * Function retrieves and sets modified management IEs to FW.
> + */
> +static int mwifiex_cfg80211_change_beacon(struct wiphy *wiphy,
> +					  struct net_device *dev,
> +					  struct cfg80211_ap_update *params)
> +{
> +	int ret;
> +
> +	ret = mwifiex_cfg80211_change_beacon_data(wiphy, dev, &params->beacon);
> +
> +	return ret;

just return mwifiex_cfg80211_change_beacon_data(wiphy, dev, &params->beacon);