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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PA4PR04MB96389DD6C8EC3903CEF01EFFD1642@PA4PR04MB9638.eurprd04.prod.outlook.com>
Date: Thu, 12 Sep 2024 02:22:09 +0000
From: David Lin <yu-hao.lin@....com>
To: Francesco Dolcini <francesco@...cini.it>, "l.stach@...gutronix.de"
	<l.stach@...gutronix.de>
CC: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"briannorris@...omium.org" <briannorris@...omium.org>, "kvalo@...nel.org"
	<kvalo@...nel.org>, Pete Hsieh <tsung-hsien.hsieh@....com>
Subject: RE: [EXT] Re: [PATCH] wifi: mwifiex: fix firmware crash for AP DFS
 mode

> From: Francesco Dolcini <francesco@...cini.it>
> Sent: Wednesday, September 11, 2024 5:33 PM
> To: David Lin <yu-hao.lin@....com>; l.stach@...gutronix.de
> Cc: linux-wireless@...r.kernel.org; linux-kernel@...r.kernel.org;
> briannorris@...omium.org; kvalo@...nel.org; francesco@...cini.it; Pete
> Hsieh <tsung-hsien.hsieh@....com>
> 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@....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.
> 

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.