[PATCH v3 0/5] A few more cleanups and fixes for mwifiex

[PATCH v3 0/5] A few more cleanups and fixes for mwifiex

[Jonas Dreßler]

v1: https://lore.kernel.org/linux-wireless/20211016101743.15565-1-verdre@v0yd.nl/T/#t
v2: https://lore.kernel.org/linux-wireless/20211016103656.16791-1-verdre@v0yd.nl/T/#t

Changes between v2 and v3:
 - Added "Fixes" tag to commit fixing DELBA requests

Just a few more cleanups and two fixes for mwifiex.

Jonas Dreßler (5):
  mwifiex: Don't log error on suspend if wake-on-wlan is disabled
  mwifiex: Log an error on command failure during key-material upload
  mwifiex: Fix an incorrect comment
  mwifiex: Send DELBA requests according to spec
  mwifiex: Deactive host sleep using HSCFG after it was activated
    manually

 drivers/net/wireless/marvell/mwifiex/11n.c    |  7 ++++---
 .../net/wireless/marvell/mwifiex/cfg80211.c   | 12 ++++++++---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 21 +++++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.c   | 18 ++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h   |  1 +
 .../net/wireless/marvell/mwifiex/sta_cmd.c    |  4 ++++
 6 files changed, 57 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH v3 1/5] mwifiex: Don't log error on suspend if wake-on-wlan is disabled

[Jonas Dreßler]

It's not an error if someone chooses to put their computer to sleep, not
wanting it to wake up because the person next door has just discovered
what a magic packet is. So change the loglevel of this annoying message
from ERROR to INFO.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index ef697572a293..987558c4fc79 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -3492,7 +3492,7 @@ static int mwifiex_cfg80211_suspend(struct wiphy *wiphy,
 	}
 
 	if (!wowlan) {
-		mwifiex_dbg(adapter, ERROR,
+		mwifiex_dbg(adapter, INFO,
 			    "None of the WOWLAN triggers enabled\n");
 		ret = 0;
 		goto done;
-- 
2.31.1

Re: [PATCH v3 1/5] mwifiex: Don't log error on suspend if wake-on-wlan is disabled

[Joe Perches]

On Sat, 2021-10-16 at 17:32 +0200, Jonas Dreßler wrote:
> It's not an error if someone chooses to put their computer to sleep, not
> wanting it to wake up because the person next door has just discovered
> what a magic packet is. So change the loglevel of this annoying message
> from ERROR to INFO.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index ef697572a293..987558c4fc79 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -3492,7 +3492,7 @@ static int mwifiex_cfg80211_suspend(struct wiphy *wiphy,
>  	}
>  
>  	if (!wowlan) {
> -		mwifiex_dbg(adapter, ERROR,
> +		mwifiex_dbg(adapter, INFO,
>  			    "None of the WOWLAN triggers enabled\n");

None of these are have a loglevel of KERN_ERR,
all of these are logged at KERN_INFO.

What I don't understand is why mwifiex_dbg is using KERN_INFO at all
and not KERN_DEBUG.

[]

drivers/net/wireless/marvell/mwifiex/main.h:#define mwifiex_dbg(adapter, mask, fmt, ...)                                \
drivers/net/wireless/marvell/mwifiex/main.h-    _mwifiex_dbg(adapter, MWIFIEX_DBG_##mask, fmt, ##__VA_ARGS__)

[]

drivers/net/wireless/marvell/mwifiex/main.c:void _mwifiex_dbg(const struct mwifiex_adapter *adapter, int mask,
drivers/net/wireless/marvell/mwifiex/main.c-              const char *fmt, ...)
drivers/net/wireless/marvell/mwifiex/main.c-{
drivers/net/wireless/marvell/mwifiex/main.c-    struct va_format vaf;
drivers/net/wireless/marvell/mwifiex/main.c-    va_list args;
drivers/net/wireless/marvell/mwifiex/main.c-
drivers/net/wireless/marvell/mwifiex/main.c-    if (!(adapter->debug_mask & mask))
drivers/net/wireless/marvell/mwifiex/main.c-            return;
drivers/net/wireless/marvell/mwifiex/main.c-
drivers/net/wireless/marvell/mwifiex/main.c-    va_start(args, fmt);
drivers/net/wireless/marvell/mwifiex/main.c-
drivers/net/wireless/marvell/mwifiex/main.c-    vaf.fmt = fmt;
drivers/net/wireless/marvell/mwifiex/main.c-    vaf.va = &args;
drivers/net/wireless/marvell/mwifiex/main.c-
drivers/net/wireless/marvell/mwifiex/main.c-    if (adapter->dev)
drivers/net/wireless/marvell/mwifiex/main.c-            dev_info(adapter->dev, "%pV", &vaf);
drivers/net/wireless/marvell/mwifiex/main.c-    else
drivers/net/wireless/marvell/mwifiex/main.c-            pr_info("%pV", &vaf);
drivers/net/wireless/marvell/mwifiex/main.c-
drivers/net/wireless/marvell/mwifiex/main.c-    va_end(args);
drivers/net/wireless/marvell/mwifiex/main.c-}
drivers/net/wireless/marvell/mwifiex/main.c:EXPORT_SYMBOL_GPL(_mwifiex_dbg);

Re: [PATCH v3 1/5] mwifiex: Don't log error on suspend if wake-on-wlan is disabled

[Kalle Valo]

Jonas Dreßler <verdre@v0yd.nl> wrote:

> It's not an error if someone chooses to put their computer to sleep, not
> wanting it to wake up because the person next door has just discovered
> what a magic packet is. So change the loglevel of this annoying message
> from ERROR to INFO.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

5 patches applied to wireless-drivers-next.git, thanks.

03893e93aff8 mwifiex: Don't log error on suspend if wake-on-wlan is disabled
fd7f8c321b78 mwifiex: Log an error on command failure during key-material upload
a8a8fc7b2a71 mwifiex: Fix an incorrect comment
cc8a8bc37466 mwifiex: Send DELBA requests according to spec
5943a864fe84 mwifiex: Deactive host sleep using HSCFG after it was activated manually

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20211016153244.24353-2-verdre@v0yd.nl/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v3 2/5] mwifiex: Log an error on command failure during key-material upload

[Jonas Dreßler]

Sometimes the KEY_MATERIAL command can fail with the 88W8897 firmware
(when this happens exactly seems pretty random). This appears to prevent
the access point from starting, so it seems like a good idea to log an
error in that case.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 987558c4fc79..6f23ec34e2e2 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -519,8 +519,14 @@ mwifiex_cfg80211_set_default_mgmt_key(struct wiphy *wiphy,
 	encrypt_key.is_igtk_def_key = true;
 	eth_broadcast_addr(encrypt_key.mac_addr);
 
-	return mwifiex_send_cmd(priv, HostCmd_CMD_802_11_KEY_MATERIAL,
-				HostCmd_ACT_GEN_SET, true, &encrypt_key, true);
+	if (mwifiex_send_cmd(priv, HostCmd_CMD_802_11_KEY_MATERIAL,
+			     HostCmd_ACT_GEN_SET, true, &encrypt_key, true)) {
+		mwifiex_dbg(priv->adapter, ERROR,
+			    "Sending KEY_MATERIAL command failed\n");
+		return -1;
+	}
+
+	return 0;
 }
 
 /*
-- 
2.31.1

[PATCH v3 3/5] mwifiex: Fix an incorrect comment

[Jonas Dreßler]

We're sending DELBA requests here, not ADDBA requests.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/11n.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
index 6696bce56178..b0695432b26a 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n.c
@@ -125,7 +125,7 @@ int mwifiex_ret_11n_delba(struct mwifiex_private *priv,
 					   tx_ba_tbl->ra);
 	} else { /*
 		  * In case of failure, recreate the deleted stream in case
-		  * we initiated the ADDBA
+		  * we initiated the DELBA
 		  */
 		if (!INITIATOR_BIT(del_ba_param_set))
 			return 0;
-- 
2.31.1

[PATCH v3 4/5] mwifiex: Send DELBA requests according to spec

[Jonas Dreßler]

While looking at on-air packets using Wireshark, I noticed we're never
setting the initiator bit when sending DELBA requests to the AP: While
we set the bit on our del_ba_param_set bitmask, we forget to actually
copy that bitmask over to the command struct, which means we never
actually set the initiator bit.

Fix that and copy the bitmask over to the host_cmd_ds_11n_delba command
struct.

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/11n.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
index b0695432b26a..9ff2058bcd7e 100644
--- a/drivers/net/wireless/marvell/mwifiex/11n.c
+++ b/drivers/net/wireless/marvell/mwifiex/11n.c
@@ -657,14 +657,15 @@ int mwifiex_send_delba(struct mwifiex_private *priv, int tid, u8 *peer_mac,
 	uint16_t del_ba_param_set;
 
 	memset(&delba, 0, sizeof(delba));
-	delba.del_ba_param_set = cpu_to_le16(tid << DELBA_TID_POS);
 
-	del_ba_param_set = le16_to_cpu(delba.del_ba_param_set);
+	del_ba_param_set = tid << DELBA_TID_POS;
+
 	if (initiator)
 		del_ba_param_set |= IEEE80211_DELBA_PARAM_INITIATOR_MASK;
 	else
 		del_ba_param_set &= ~IEEE80211_DELBA_PARAM_INITIATOR_MASK;
 
+	delba.del_ba_param_set = cpu_to_le16(del_ba_param_set);
 	memcpy(&delba.peer_mac_addr, peer_mac, ETH_ALEN);
 
 	/* We don't wait for the response of this command */
-- 
2.31.1

Re: [PATCH v3 4/5] mwifiex: Send DELBA requests according to spec

[Pali Rohár]

On Saturday 16 October 2021 17:32:43 Jonas Dreßler wrote:
> While looking at on-air packets using Wireshark, I noticed we're never
> setting the initiator bit when sending DELBA requests to the AP: While
> we set the bit on our del_ba_param_set bitmask, we forget to actually
> copy that bitmask over to the command struct, which means we never
> actually set the initiator bit.
> 
> Fix that and copy the bitmask over to the host_cmd_ds_11n_delba command
> struct.
> 
> Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

Acked-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/net/wireless/marvell/mwifiex/11n.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n.c b/drivers/net/wireless/marvell/mwifiex/11n.c
> index b0695432b26a..9ff2058bcd7e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n.c
> @@ -657,14 +657,15 @@ int mwifiex_send_delba(struct mwifiex_private *priv, int tid, u8 *peer_mac,
>  	uint16_t del_ba_param_set;
>  
>  	memset(&delba, 0, sizeof(delba));
> -	delba.del_ba_param_set = cpu_to_le16(tid << DELBA_TID_POS);
>  
> -	del_ba_param_set = le16_to_cpu(delba.del_ba_param_set);
> +	del_ba_param_set = tid << DELBA_TID_POS;
> +
>  	if (initiator)
>  		del_ba_param_set |= IEEE80211_DELBA_PARAM_INITIATOR_MASK;
>  	else
>  		del_ba_param_set &= ~IEEE80211_DELBA_PARAM_INITIATOR_MASK;
>  
> +	delba.del_ba_param_set = cpu_to_le16(del_ba_param_set);
>  	memcpy(&delba.peer_mac_addr, peer_mac, ETH_ALEN);
>  
>  	/* We don't wait for the response of this command */
> -- 
> 2.31.1
> 

[PATCH v3 5/5] mwifiex: Deactive host sleep using HSCFG after it was activated manually

[Jonas Dreßler]

When powersaving (so either wifi powersaving or deep sleep, depending on
which state the firmware is in) is disabled, the way the firmware goes
into host sleep is different: Usually the firmware implicitely enters
host sleep on the next SLEEP event we get when we configured host sleep
via HSCFG before. When powersaving is disabled though, there are no
SLEEP events, the way we enter host sleep in that case is different: The
firmware will send us a HS_ACT_REQ event and after that we "manually"
make the firmware enter host sleep by sending it another HSCFG command
with the action HS_ACTIVATE.

Now waking up from host sleep appears to be different depending on
whether powersaving is enabled again: When powersaving is enabled, the
firmware implicitely leaves host sleep as soon as it wakes up and sends
us an AWAKE event. When powersaving is disabled though, it apparently
doesn't implicitely leave host sleep, but instead we need to send it a
HSCFG command with the HS_CONFIGURE action and the HS_CFG_CANCEL
condition. We didn't do that so far, which is why waking up from host
sleep was broken when powersaving is disabled.

So add some additional state to mwifiex_adapter where we keep track of
whether host sleep was activated manually via HS_ACTIVATE, and if that
was the case, deactivate it manually again via HS_CFG_CANCEL.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 21 +++++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.c   | 18 ++++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h   |  1 +
 .../net/wireless/marvell/mwifiex/sta_cmd.c    |  4 ++++
 4 files changed, 44 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 171a25742600..d6a61f850c6f 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -608,6 +608,11 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no,
 		return -1;
 	}
 
+	if (priv->adapter->hs_activated_manually &&
+	    cmd_no != HostCmd_CMD_802_11_HS_CFG_ENH) {
+		mwifiex_cancel_hs(priv, MWIFIEX_ASYNC_CMD);
+		priv->adapter->hs_activated_manually = false;
+	}
 
 	/* Get a new command node */
 	cmd_node = mwifiex_get_cmd_node(adapter);
@@ -714,6 +719,15 @@ mwifiex_insert_cmd_to_pending_q(struct mwifiex_adapter *adapter,
 		}
 	}
 
+	/* Same with exit host sleep cmd, luckily that can't happen at the same time as EXIT_PS */
+	if (command == HostCmd_CMD_802_11_HS_CFG_ENH) {
+		struct host_cmd_ds_802_11_hs_cfg_enh *hs_cfg =
+			&host_cmd->params.opt_hs_cfg;
+
+		if (le16_to_cpu(hs_cfg->action) == HS_ACTIVATE)
+				add_tail = false;
+	}
+
 	spin_lock_bh(&adapter->cmd_pending_q_lock);
 	if (add_tail)
 		list_add_tail(&cmd_node->list, &adapter->cmd_pending_q);
@@ -1216,6 +1230,13 @@ mwifiex_process_hs_config(struct mwifiex_adapter *adapter)
 		    __func__);
 
 	adapter->if_ops.wakeup(adapter);
+
+	if (adapter->hs_activated_manually) {
+		mwifiex_cancel_hs(mwifiex_get_priv (adapter, MWIFIEX_BSS_ROLE_ANY),
+				  MWIFIEX_ASYNC_CMD);
+		adapter->hs_activated_manually = false;
+	}
+
 	adapter->hs_activated = false;
 	clear_bit(MWIFIEX_IS_HS_CONFIGURED, &adapter->work_flags);
 	clear_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 7943fd3b3058..58d434f1285d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -401,6 +401,12 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 		     !adapter->scan_processing) &&
 		    !adapter->data_sent &&
 		    !skb_queue_empty(&adapter->tx_data_q)) {
+			if (adapter->hs_activated_manually) {
+				mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY),
+						  MWIFIEX_ASYNC_CMD);
+				adapter->hs_activated_manually = false;
+			}
+
 			mwifiex_process_tx_queue(adapter);
 			if (adapter->hs_activated) {
 				clear_bit(MWIFIEX_IS_HS_CONFIGURED,
@@ -418,6 +424,12 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 		    !mwifiex_bypass_txlist_empty(adapter) &&
 		    !mwifiex_is_tdls_chan_switching
 			(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA))) {
+			if (adapter->hs_activated_manually) {
+				mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY),
+						  MWIFIEX_ASYNC_CMD);
+				adapter->hs_activated_manually = false;
+			}
+
 			mwifiex_process_bypass_tx(adapter);
 			if (adapter->hs_activated) {
 				clear_bit(MWIFIEX_IS_HS_CONFIGURED,
@@ -434,6 +446,12 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 		    !adapter->data_sent && !mwifiex_wmm_lists_empty(adapter) &&
 		    !mwifiex_is_tdls_chan_switching
 			(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA))) {
+			if (adapter->hs_activated_manually) {
+				mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY),
+						  MWIFIEX_ASYNC_CMD);
+				adapter->hs_activated_manually = false;
+			}
+
 			mwifiex_wmm_process_tx(adapter);
 			if (adapter->hs_activated) {
 				clear_bit(MWIFIEX_IS_HS_CONFIGURED,
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 5923c5c14c8d..90012cbcfd15 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -986,6 +986,7 @@ struct mwifiex_adapter {
 	struct timer_list wakeup_timer;
 	struct mwifiex_hs_config_param hs_cfg;
 	u8 hs_activated;
+	u8 hs_activated_manually;
 	u16 hs_activate_wait_q_woken;
 	wait_queue_head_t hs_activate_wait_q;
 	u8 event_body[MAX_EVENT_SIZE];
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 48ea00da1fc9..1e2798dce18f 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -396,6 +396,10 @@ mwifiex_cmd_802_11_hs_cfg(struct mwifiex_private *priv,
 	if (hs_activate) {
 		hs_cfg->action = cpu_to_le16(HS_ACTIVATE);
 		hs_cfg->params.hs_activate.resp_ctrl = cpu_to_le16(RESP_NEEDED);
+
+		adapter->hs_activated_manually = true;
+		mwifiex_dbg(priv->adapter, CMD,
+			    "cmd: Activating host sleep manually\n");
 	} else {
 		hs_cfg->action = cpu_to_le16(HS_CONFIGURE);
 		hs_cfg->params.hs_config.conditions = hscfg_param->conditions;
-- 
2.31.1