+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@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.
> +
> + 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, ¶ms->beacon);
> +
> + return ret;
just return mwifiex_cfg80211_change_beacon_data(wiphy, dev, ¶ms->beacon);
