> From: Brian Norris <briannorris@xxxxxxxxxxxx>
> Sent: Saturday, March 16, 2024 8:00 AM
> To: David Lin <yu-hao.lin@xxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete Hsieh
> <tsung-hsien.hsieh@xxxxxxx>; Francesco Dolcini
> <francesco.dolcini@xxxxxxxxxxx>
> 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@xxxxxxx>
> > Reviewed-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > ---
> >
> > 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
