RE: [EXT] Re: [PATCH v9 1/2] wifi: mwifiex: add host mlme for client mode

[David Lin <yu-hao.lin@nxp.com>]

> From: Brian Norris <briannorris@chromium.org>
> Sent: Saturday, March 16, 2024 8:00 AM
> 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; Pete Hsieh
> <tsung-hsien.hsieh@nxp.com>; Francesco Dolcini
> <francesco.dolcini@toradex.com>
> Subject: [EXT] Re: [PATCH v9 1/2] wifi: mwifiex: add host mlme for client 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
> 
> 
> Hi David,
> 
> Thanks for the persistence here (and thanks Francesco for all the review help).
> I think things are mostly well structured here, but I'll also admit it's a pretty
> large bit of work to review at once. So please bear with me if it takes a bit of
> time to really get through it. I'll post a few thoughts now, but it's possible I'll
> have more after another pass.
> 

Thanks for your help and take your time.

> On Wed, Mar 06, 2024 at 10:00:52AM +0800, David Lin wrote:
> > Add host based MLME to enable WPA3 functionalities in client 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>
> > ---
> >
> > v9:
> >    - remove redundent code.
> >
> > v8:
> >    - first full and complete patch to support host based MLME for client
> >      mode.
> >
> > ---
> >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 315
> ++++++++++++++++++
> >  drivers/net/wireless/marvell/mwifiex/cmdevt.c |  25 ++
> >  drivers/net/wireless/marvell/mwifiex/decl.h   |  22 ++
> >  drivers/net/wireless/marvell/mwifiex/fw.h     |  33 ++
> >  drivers/net/wireless/marvell/mwifiex/init.c   |   6 +
> >  drivers/net/wireless/marvell/mwifiex/join.c   |  66 +++-
> >  drivers/net/wireless/marvell/mwifiex/main.c   |  54 +++
> >  drivers/net/wireless/marvell/mwifiex/main.h   |  16 +
> >  drivers/net/wireless/marvell/mwifiex/scan.c   |   6 +
> >  drivers/net/wireless/marvell/mwifiex/sdio.c   |  13 +
> >  drivers/net/wireless/marvell/mwifiex/sdio.h   |   2 +
> >  .../net/wireless/marvell/mwifiex/sta_event.c  |  36 +-
> >  .../net/wireless/marvell/mwifiex/sta_ioctl.c  |   2 +-
> >  drivers/net/wireless/marvell/mwifiex/sta_tx.c |   9 +-
> >  drivers/net/wireless/marvell/mwifiex/util.c   |  80 +++++
> >  15 files changed, 671 insertions(+), 14 deletions(-)
> 
> (Per the above, I'd normally consider whether ~671 new lines is worth splitting
> into multiple patches. But I don't see any great logical ways to do that.)
> 

Francesco suggested to use two patches for this host mlme new feature from previous many patches. I knew it is a lot of changes, but I think it should be the best way to add host mlme with two patches (one for client and one for AP).

> > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > index b909a7665e9c..bcf4f87dcaab 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> 
> > @@ -4204,6 +4210,302 @@ mwifiex_cfg80211_change_station(struct wiphy
> *wiphy, struct net_device *dev,
> >       return ret;
> >  }
> >
> > +static int
> > +mwifiex_cfg80211_authenticate(struct wiphy *wiphy,
> > +                           struct net_device *dev,
> > +                           struct cfg80211_auth_request *req) {
> > +     struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
> > +     struct mwifiex_adapter *adapter = priv->adapter;
> > +     struct sk_buff *skb;
> > +     u16 pkt_len, auth_alg;
> > +     int ret;
> > +     struct mwifiex_ieee80211_mgmt *mgmt;
> > +     struct mwifiex_txinfo *tx_info;
> > +     u32 tx_control = 0, pkt_type = PKT_TYPE_MGMT;
> > +     u8 addr[ETH_ALEN] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
> 
> 
> > +     memcpy(mgmt->addr4, addr, ETH_ALEN);
> 
>         eth_broadcast_addr(mgmt->addr4);
> 
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> 
> > @@ -1558,6 +1596,14 @@ mwifiex_reinit_sw(struct mwifiex_adapter
> *adapter)
> >               INIT_WORK(&adapter->rx_work,
> mwifiex_rx_work_queue);
> >       }
> >
> > +     adapter->host_mlme_workqueue =
> > +
> alloc_workqueue("MWIFIEX_HOST_MLME_WORK_QUEUE",
> > +                             WQ_HIGHPRI | WQ_MEM_RECLAIM |
> > + WQ_UNBOUND, 0);
> 
> Hmm, why do you need a whole new workqueue? This driver is already full of
> race conditions, while many race conditions are avoided simply because most
> work is sequentialized onto the main work queue. If you don't have a good
> reason here, I'd probably prefer you share the existing queue.
> 
> Or otherwise, if this is *truly* independent and race-free, do you actually need
> to create a new queue? We could just use schedule_work(), which uses the
> system queue.
> 
> If you do really need it, is it possible to key off 'host_mlme_enabled'
> or similar? There's no need to allocate the queue if we're not using it.
> 

Will add the checking of 'host_mlme_enabled' to create this work queue if needed in patch v10.

> > +     if (!adapter->host_mlme_workqueue)
> > +             goto err_kmalloc;
> > +
> > +     INIT_WORK(&adapter->host_mlme_work,
> > + mwifiex_host_mlme_work_queue);
> > +
> >       /* Register the device. Fill up the private data structure with
> >        * relevant information from the card. Some code extracted from
> >        * mwifiex_register_dev()
> 
> 
> > --- a/drivers/net/wireless/marvell/mwifiex/util.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/util.c
> > @@ -370,6 +370,46 @@ mwifiex_parse_mgmt_packet(struct
> mwifiex_private
> > *priv, u8 *payload, u16 len,
> >
> >       return 0;
> >  }
> > +
> > +/* This function sends deauth packet to the kernel. */ void
> > +mwifiex_host_mlme_disconnect(struct mwifiex_private *priv,
> > +                               u16 reason_code, u8 *sa) {
> > +     u8 broadcast_addr[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> > +     u8 frame_buf[100];
> > +     struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt
> > +*)frame_buf;
> > +
> > +     memset(frame_buf, 0, sizeof(frame_buf));
> > +     mgmt->frame_control = (__force __le16)IEEE80211_STYPE_DEAUTH;
> 
> Hmm, "__force" is a pretty good sign that you're doing something wrong.
> Please think twice before using it.
> 

Will modify it in patch v10.

> I believe the right answer here is cpu_to_le16(). It's a no-op on little endian
> architectures, but it'll make big-endian work.
> 
> > +     mgmt->duration = 0;
> > +     mgmt->seq_ctrl = 0;
> > +     mgmt->u.deauth.reason_code = (__force __le16)reason_code;
> 
> Same here.
> 

Will do in patch v10.

> > +
> > +     if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA) {
> > +             memcpy(mgmt->da, broadcast_addr, ETH_ALEN);
> 
>                 eth_broadcast_addr(mgmt->da);
> 

Will change it in patch v10.

> > +             memcpy(mgmt->sa,
> > +
> priv->curr_bss_params.bss_descriptor.mac_address,
> > +                    ETH_ALEN);
> > +             memcpy(mgmt->bssid, priv->cfg_bssid, ETH_ALEN);
> > +             priv->auth_flag = 0;
> > +             priv->auth_alg = WLAN_AUTH_NONE;
> 
> 
> > @@ -417,6 +457,46 @@ mwifiex_process_mgmt_packet(struct
> mwifiex_private *priv,
> >       pkt_len -= ETH_ALEN;
> >       rx_pd->rx_pkt_length = cpu_to_le16(pkt_len);
> >
> > +     if (priv->host_mlme_reg &&
> > +         (GET_BSS_ROLE(priv) != MWIFIEX_BSS_ROLE_UAP) &&
> > +         (ieee80211_is_auth(ieee_hdr->frame_control) ||
> > +          ieee80211_is_deauth(ieee_hdr->frame_control) ||
> > +          ieee80211_is_disassoc(ieee_hdr->frame_control))) {
> > +             if (ieee80211_is_auth(ieee_hdr->frame_control)) {
> > +                     if (priv->auth_flag &
> HOST_MLME_AUTH_PENDING) {
> > +                             if (priv->auth_alg != WLAN_AUTH_SAE)
> {
> > +                                     priv->auth_flag &=
> > +
> ~HOST_MLME_AUTH_PENDING;
> > +                                     priv->auth_flag |=
> > +
> HOST_MLME_AUTH_DONE;
> > +                             }
> > +                     } else {
> > +                             return 0;
> > +                     }
> > +
> > +                     mwifiex_dbg(priv->adapter, MSG,
> > +                                 "auth: receive authentication from
> %pM\n",
> > +                                 ieee_hdr->addr3);
> > +             } else {
> > +                     if (!priv->wdev.connected)
> > +                             return 0;
> > +
> > +                     if
> (ieee80211_is_deauth(ieee_hdr->frame_control)) {
> > +                             mwifiex_dbg(priv->adapter, MSG,
> > +                                         "auth: receive deauth
> from %pM\n",
> > +                                         ieee_hdr->addr3);
> > +                             priv->auth_flag = 0;
> > +                             priv->auth_alg = WLAN_AUTH_NONE;
> > +                     } else {
> > +                             mwifiex_dbg(priv->adapter, MSG,
> > +                                         "assoc: receive disasso
> from
> > + %pM\n",
> 
> I get that sometimes abbreviations are nice, but perhaps at least use a
> consistent one? I see "disassoc" elsewhere.
> 

Will modify it in patch v10.

> > +                                         ieee_hdr->addr3);
> 
> Brian