From: David Lin <yu-hao.lin@nxp.com>
To: Brian Norris <briannorris@chromium.org>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kvalo@kernel.org" <kvalo@kernel.org>,
"francesco@dolcini.it" <francesco@dolcini.it>,
Pete Hsieh <tsung-hsien.hsieh@nxp.com>,
Francesco Dolcini <francesco.dolcini@toradex.com>
Subject: RE: [EXT] Re: [PATCH v9 1/2] wifi: mwifiex: add host mlme for client mode
Date: Mon, 18 Mar 2024 02:00:35 +0000 [thread overview]
Message-ID: <PA4PR04MB9638F2586DF7E8A41330C8C9D12D2@PA4PR04MB9638.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <ZfThCwGj-P5Owlsn@google.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
next prev parent reply other threads:[~2024-03-18 2:00 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 2:00 [PATCH v9 0/2] wifi: mwifiex: add code to support host mlme David Lin
2024-03-06 2:00 ` [PATCH v9 1/2] wifi: mwifiex: add host mlme for client mode David Lin
2024-03-16 0:00 ` Brian Norris
2024-03-18 2:00 ` David Lin [this message]
2024-03-18 11:41 ` Francesco Dolcini
2024-03-19 1:36 ` [EXT] " David Lin
2024-03-06 2:00 ` [PATCH v9 2/2] wifi: mwifiex: add host mlme for AP mode David Lin
2024-03-16 0:45 ` Brian Norris
2024-03-18 2:04 ` [EXT] " David Lin
2024-04-18 3:37 ` David Lin
2024-03-15 9:49 ` [PATCH v9 0/2] wifi: mwifiex: add code to support host mlme Francesco Dolcini
2024-03-15 23:59 ` Brian Norris
2024-03-18 11:28 ` Francesco Dolcini
2024-03-19 10:33 ` Kalle Valo
2024-03-20 21:06 ` Brian Norris
2024-03-16 0:49 ` Brian Norris
2024-03-19 12:12 ` Johannes Berg
2024-03-20 0:59 ` [EXT] " David Lin
2024-03-20 1:10 ` David Lin
2024-03-20 9:12 ` Johannes Berg
2024-03-20 21:50 ` Brian Norris
2024-03-21 4:07 ` David Lin
2024-03-23 1:06 ` Brian Norris
2024-03-25 16:15 ` Johannes Berg
2024-03-29 10:06 ` David Lin
2024-04-02 17:38 ` Brian Norris
2024-04-10 7:30 ` David Lin
2024-04-10 7:55 ` Johannes Berg
2024-04-10 10:33 ` David Lin
2024-04-10 17:56 ` Johannes Berg
2024-04-11 7:57 ` David Lin
2024-04-11 8:02 ` Johannes Berg
2024-04-10 7:52 ` Johannes Berg
2024-03-25 15:58 ` Johannes Berg
2024-03-29 9:58 ` David Lin
2024-03-18 9:24 ` Kalle Valo
2024-03-19 1:40 ` [EXT] " David Lin
2024-03-20 21:28 ` Brian Norris
2024-03-21 2:14 ` [EXT] " David Lin
2024-03-16 0:07 ` Brian Norris
2024-03-18 2:20 ` [EXT] " David Lin
2024-03-18 11:45 ` Francesco Dolcini
2024-03-20 21:13 ` Brian Norris
2024-03-21 2:12 ` David Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=PA4PR04MB9638F2586DF7E8A41330C8C9D12D2@PA4PR04MB9638.eurprd04.prod.outlook.com \
--to=yu-hao.lin@nxp.com \
--cc=briannorris@chromium.org \
--cc=francesco.dolcini@toradex.com \
--cc=francesco@dolcini.it \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=tsung-hsien.hsieh@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).