2021-10-18 03:33:44

by Jonas Dreßler

[permalink] [raw]
Subject: [PATCH v2 0/5] A few more cleanups and fixes for mwifiex

v1: https://lore.kernel.org/linux-wireless/[email protected]/T/#t

Changes between v1 and v2:
- Added another commit fixing a bug with host sleep when it was entered
explicitly instead of implicitly.

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


2021-10-18 03:33:53

by Jonas Dreßler

[permalink] [raw]
Subject: [PATCH v2 4/5] mwifiex: Send DELBA requests according to spec

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.

Signed-off-by: Jonas Dreßler <[email protected]>
---
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

2021-10-18 03:35:11

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] mwifiex: Send DELBA requests according to spec

On Saturday 16 October 2021 12:36:55 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.
>
> Signed-off-by: Jonas Dreßler <[email protected]>

Hello! This looks like is fixing mwifiex_send_delba() function which was
added in initial mwifiex commit. So probably it should have following
tag:

Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")

> ---
> 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
>

2021-10-18 03:35:47

by Jonas Dreßler

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] mwifiex: Send DELBA requests according to spec

On 10/16/21 16:28, Pali Rohár wrote:
> On Saturday 16 October 2021 12:36:55 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.
>>
>> Signed-off-by: Jonas Dreßler <[email protected]>
>
> Hello! This looks like is fixing mwifiex_send_delba() function which was
> added in initial mwifiex commit. So probably it should have following
> tag:
>
> Fixes: 5e6e3a92b9a4 ("wireless: mwifiex: initial commit for Marvell mwifiex driver")
>

Hi Pali,

thanks a lot for the quick review, I just addressed this in v3!

>> ---
>> 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
>>