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

From: David Lin
Date: Wed Sep 11 2024 - 22:22:48 EST

> From: Francesco Dolcini <francesco@xxxxxxxxxx>
> Sent: Wednesday, September 11, 2024 5:33 PM
> To: David Lin <yu-hao.lin@xxxxxxx>; l.stach@xxxxxxxxxxxxxx
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> briannorris@xxxxxxxxxxxx; kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete
> Hsieh <tsung-hsien.hsieh@xxxxxxx>
> Subject: [EXT] Re: [PATCH] wifi: mwifiex: fix firmware crash for AP DFS 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
>
>
> +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.
>

This file can be used to test this patch on other chips without really radar detection from HW.
We only test this patch with IW416.

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

O.K.

> 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.
>

O.K.

> >
> > Signed-off-by: David Lin <yu-hao.lin@xxxxxxx>
> > ---
> > 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.
>

O.K. I will remove it.

> > +
> > + 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?
>

If netif_carrier_ok(), there is no need to call netif_carrier_on().

> > + 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);

O.K. Will change it in patch v2.