[PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Jonas Dreßler]

The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
hardware issue where the card wakes up from deep sleep randomly and very
often, somewhat depending on the card activity, maybe the hardware has a
floating wakeup pin or something.

Those continuous wakeups prevent the card from entering host sleep when
the computer suspends. And because the host won't answer to events from
the card anymore while it's suspended, the firmwares internal
powersaving state machine seems to get confused and the card can't sleep
anymore at all after that.

Since we can't work around that hardware bug in the firmware, let's
get the hardware revision string from the firmware and match it with
known bad revisions. Then disable auto deep sleep for those revisions,
which makes sure we no longer get those spurious wakeups.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/main.c      | 14 ++++++++++++++
 drivers/net/wireless/marvell/mwifiex/main.h      |  1 +
 .../net/wireless/marvell/mwifiex/sta_cmdresp.c   | 16 ++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 19b996c6a260..5ab2ad4c7006 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -226,6 +226,19 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
+static void maybe_quirk_fw_disable_ds(struct mwifiex_adapter *adapter)
+{
+	struct mwifiex_private *priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA);
+	struct mwifiex_ver_ext ver_ext;
+
+	set_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &adapter->work_flags);
+
+	memset(&ver_ext, 0, sizeof(ver_ext));
+	ver_ext.version_str_sel = 1;
+	mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
+			 HostCmd_ACT_GEN_GET, 0, &ver_ext, false);
+}
+
 /*
  * The main process.
  *
@@ -356,6 +369,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 			if (adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE) {
 				adapter->hw_status = MWIFIEX_HW_STATUS_READY;
 				mwifiex_init_fw_complete(adapter);
+				maybe_quirk_fw_disable_ds(adapter);
 			}
 		}
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 90012cbcfd15..1e829d84b1f6 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -524,6 +524,7 @@ enum mwifiex_adapter_work_flags {
 	MWIFIEX_IS_SUSPENDED,
 	MWIFIEX_IS_HS_CONFIGURED,
 	MWIFIEX_IS_HS_ENABLING,
+	MWIFIEX_IS_REQUESTING_FW_VEREXT,
 };
 
 struct mwifiex_band_config {
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 6b5d35d9e69f..8e49ebca1847 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -708,6 +708,22 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
 {
 	struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
 
+	if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
+		if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)", 128) == 0) {
+			struct mwifiex_ds_auto_ds auto_ds = {
+				.auto_ds = DEEP_SLEEP_OFF,
+			};
+
+			mwifiex_dbg(priv->adapter, MSG,
+				    "Bad HW revision detected, disabling deep sleep\n");
+
+			mwifiex_send_cmd(priv, HostCmd_CMD_802_11_PS_MODE_ENH,
+					 DIS_AUTO_PS, BITMAP_AUTO_DS, &auto_ds, false);
+		}
+
+		return 0;
+	}
+
 	if (version_ext) {
 		version_ext->version_str_sel = ver_ext->version_str_sel;
 		memcpy(version_ext->version_str, ver_ext->version_str,
-- 
2.31.1

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Andy Shevchenko]

On Thu, Oct 28, 2021 at 10:38 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
> hardware issue where the card wakes up from deep sleep randomly and very
> often, somewhat depending on the card activity, maybe the hardware has a
> floating wakeup pin or something.
>
> Those continuous wakeups prevent the card from entering host sleep when
> the computer suspends. And because the host won't answer to events from
> the card anymore while it's suspended, the firmwares internal
> powersaving state machine seems to get confused and the card can't sleep

power saving

> anymore at all after that.
>
> Since we can't work around that hardware bug in the firmware, let's
> get the hardware revision string from the firmware and match it with
> known bad revisions. Then disable auto deep sleep for those revisions,
> which makes sure we no longer get those spurious wakeups.

...

> +static void maybe_quirk_fw_disable_ds(struct mwifiex_adapter *adapter)
> +{
> +       struct mwifiex_private *priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA);
> +       struct mwifiex_ver_ext ver_ext;

> +       set_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &adapter->work_flags);

This does not bring atomicity to this function.
You need test_and_set_bit().

Otherwise the bit may very well be cleared already here. And function
may enter here again.

If this state machine is protected by lock or so, then why not use
__set_bit() to show this clearly?

> +       memset(&ver_ext, 0, sizeof(ver_ext));
> +       ver_ext.version_str_sel = 1;
> +       mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
> +                        HostCmd_ACT_GEN_GET, 0, &ver_ext, false);
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Denis Kirjanov]

10/28/21 10:37 AM, Jonas Dreßler пишет:
> The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
> hardware issue where the card wakes up from deep sleep randomly and very
> often, somewhat depending on the card activity, maybe the hardware has a
> floating wakeup pin or something.
> 
> Those continuous wakeups prevent the card from entering host sleep when
> the computer suspends. And because the host won't answer to events from
> the card anymore while it's suspended, the firmwares internal
> powersaving state machine seems to get confused and the card can't sleep
> anymore at all after that.
> 
> Since we can't work around that hardware bug in the firmware, let's
> get the hardware revision string from the firmware and match it with
> known bad revisions. Then disable auto deep sleep for those revisions,
> which makes sure we no longer get those spurious wakeups.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>   drivers/net/wireless/marvell/mwifiex/main.c      | 14 ++++++++++++++
>   drivers/net/wireless/marvell/mwifiex/main.h      |  1 +
>   .../net/wireless/marvell/mwifiex/sta_cmdresp.c   | 16 ++++++++++++++++
>   3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 19b996c6a260..5ab2ad4c7006 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -226,6 +226,19 @@ static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
>   	return 0;
>   }
>   
> +static void maybe_quirk_fw_disable_ds(struct mwifiex_adapter *adapter)
> +{
> +	struct mwifiex_private *priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA);
> +	struct mwifiex_ver_ext ver_ext;
> +
> +	set_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &adapter->work_flags);
> +
> +	memset(&ver_ext, 0, sizeof(ver_ext));
> +	ver_ext.version_str_sel = 1;
> +	mwifiex_send_cmd(priv, HostCmd_CMD_VERSION_EXT,
> +			 HostCmd_ACT_GEN_GET, 0, &ver_ext, false);
> +}
> +
>   /*
>    * The main process.
>    *
> @@ -356,6 +369,7 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
>   			if (adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE) {
>   				adapter->hw_status = MWIFIEX_HW_STATUS_READY;
>   				mwifiex_init_fw_complete(adapter);
> +				maybe_quirk_fw_disable_ds(adapter);
>   			}
>   		}
>   
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 90012cbcfd15..1e829d84b1f6 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -524,6 +524,7 @@ enum mwifiex_adapter_work_flags {
>   	MWIFIEX_IS_SUSPENDED,
>   	MWIFIEX_IS_HS_CONFIGURED,
>   	MWIFIEX_IS_HS_ENABLING,
> +	MWIFIEX_IS_REQUESTING_FW_VEREXT,
>   };
>   
>   struct mwifiex_band_config {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> index 6b5d35d9e69f..8e49ebca1847 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -708,6 +708,22 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>   {
>   	struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
>   
> +	if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
> +		if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)", 128) == 0) {
> +			struct mwifiex_ds_auto_ds auto_ds = {
> +				.auto_ds = DEEP_SLEEP_OFF,
> +			};
> +
> +			mwifiex_dbg(priv->adapter, MSG,
> +				    "Bad HW revision detected, disabling deep sleep\n");
> +
> +			mwifiex_send_cmd(priv, HostCmd_CMD_802_11_PS_MODE_ENH,
> +					 DIS_AUTO_PS, BITMAP_AUTO_DS, &auto_ds, false);
> +		}
> +
> +		return 0;
> +	}

mwifiex_send_cmd() may return an error

> +
>   	if (version_ext) {
>   		version_ext->version_str_sel = ver_ext->version_str_sel;
>   		memcpy(version_ext->version_str, ver_ext->version_str,
> 

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Brian Norris]

On Thu, Oct 28, 2021 at 12:37 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
> hardware issue where the card wakes up from deep sleep randomly and very
> often, somewhat depending on the card activity, maybe the hardware has a
> floating wakeup pin or something.

What makes you think it's associated with the particular "hardware
revision 20"? Have you used multiple revisions on the same platform
and found that only certain ones fail in this way? Otherwise, your
theory in the last part of your sentence sounds like a platform issue,
where you might do a DMI match instead.

> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
> @@ -708,6 +708,22 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>  {
>         struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
>
> +       if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
> +               if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)", 128) == 0) {

Rather than memorize the 128-size array here, maybe use
sizeof(ver_ext->version_str) ?

Brian

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Jonas Dreßler]

On 10/28/21 23:27, Brian Norris wrote:
> On Thu, Oct 28, 2021 at 12:37 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> The 88W8897 PCIe+USB card in the hardware revision 20 apparently has a
>> hardware issue where the card wakes up from deep sleep randomly and very
>> often, somewhat depending on the card activity, maybe the hardware has a
>> floating wakeup pin or something.
> 
> What makes you think it's associated with the particular "hardware
> revision 20"? Have you used multiple revisions on the same platform
> and found that only certain ones fail in this way? Otherwise, your
> theory in the last part of your sentence sounds like a platform issue,
> where you might do a DMI match instead.

The issue only appeared for some community members using Surface devices,
happening on the Surface Book 2 of one person, but not on the Surface Book
2 of another person. When investigating we were poking around in the dark
for a long time and almost gave up until we found that those two devices
had different hardware revisions of the same wifi card installed (ChipRev
20 vs 21).

So it seems pretty clear that with revision 21 they fixed some hardware
bug that causes those spurious wakeups.

FWIW, obviously a proper workaround for this would have to be implemented
in the firmware.

> 
>> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>> @@ -708,6 +708,22 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>>   {
>>          struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
>>
>> +       if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
>> +               if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)", 128) == 0) {
> 
> Rather than memorize the 128-size array here, maybe use
> sizeof(ver_ext->version_str) ?

Sounds like a good idea, yeah.

> 
> Brian
> 

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Jonas Dreßler]

Also, just in case anyone at NXP is still following this thread, for the
sake of completeness here's a list of all the firmware bugs we've discovered
when investigating this wifi card:

- Firmware can crash after setting TX ring write pointer while ASPM L1 or
L1 substate is active (exact substate is platform dependent). Workaround
"mwifiex: Read a PCI register after writing the TX ring write pointer"

- Firmware sometimes doesn't wake up and send an interrupt after
reading/writing a PCI register. Workaround "mwifiex: Try waking the firmware
until we get an interrupt"

- Firmware doesn't properly implement PCIe LTR (appears to send a single
report during fw startup), making the system unable to enter deeper
powersaving states. Workaround "mwifiex: Add quirk resetting the PCI bridge
on MS Surface devices"

- On hardware revision 20 the card randomly wakes up from deep sleep, most
likely a hardware bug, the firmware should work around that. Workaround
"mwifiex: Add quirk to disable deep sleep with certain hardware revision"

- BTCOEX events from firmware are not sent consistently when BT gets
active/inactive and we end up throttling wifi speeds for no reason.
Workaround "Ignore BTCOEX events from the firmware"

- Firmwares BT LE active and passive scan feature is ignoring the "Filter
duplicates" property, leading to tons of USB interrupts from the card,
preventing the system from powersaving. No workaround except not pairing
any LE devices or disabling BT LE.

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Jonas Dreßler]

On 11/3/21 13:25, Jonas Dreßler wrote:
> 
>>
>>> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>>> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
>>> @@ -708,6 +708,22 @@ static int mwifiex_ret_ver_ext(struct mwifiex_private *priv,
>>>   {
>>>          struct host_cmd_ds_version_ext *ver_ext = &resp->params.verext;
>>>
>>> +       if (test_and_clear_bit(MWIFIEX_IS_REQUESTING_FW_VEREXT, &priv->adapter->work_flags)) {
>>> +               if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)", 128) == 0) {
>>
>> Rather than memorize the 128-size array here, maybe use
>> sizeof(ver_ext->version_str) ?
> 
> Sounds like a good idea, yeah.

Nevermind, the reason I did this was for consistency in the
function, right underneath in the same function it also assumes
a fixed size of 128 characters, so I'd rather use the same
length.

>		memcpy(version_ext->version_str, ver_ext->version_str,
>		       sizeof(char) * 128);
>		memcpy(priv->version_str, ver_ext->version_str, 128);

Might be a good idea to #define it as MWIFIEX_VERSION_STR_LENGTH
in fw.h though...

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Andy Shevchenko]

On Wed, Nov 03, 2021 at 02:37:53PM +0100, Jonas Dreßler wrote:
> On 11/3/21 13:25, Jonas Dreßler wrote:

...

> > > > +               if (strncmp(ver_ext->version_str, "ChipRev:20, BB:9b(10.00), RF:40(21)", 128) == 0) {
> > > 
> > > Rather than memorize the 128-size array here, maybe use
> > > sizeof(ver_ext->version_str) ?
> > 
> > Sounds like a good idea, yeah.
> 
> Nevermind, the reason I did this was for consistency in the
> function, right underneath in the same function it also assumes
> a fixed size of 128 characters, so I'd rather use the same
> length.
> 
> > 		memcpy(version_ext->version_str, ver_ext->version_str,
> > 		       sizeof(char) * 128);

Besides sizeof(char)...

> > 		memcpy(priv->version_str, ver_ext->version_str, 128);
> 
> Might be a good idea to #define it as MWIFIEX_VERSION_STR_LENGTH
> in fw.h though...

...I think you simply need a precursor patch that changes this
to sizeof() / #define approach.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mwifiex: Add quirk to disable deep sleep with certain hardware revision

[Brian Norris]

On Wed, Nov 03, 2021 at 01:25:44PM +0100, Jonas Dreßler wrote:
> The issue only appeared for some community members using Surface devices,
> happening on the Surface Book 2 of one person, but not on the Surface Book
> 2 of another person. When investigating we were poking around in the dark
> for a long time and almost gave up until we found that those two devices
> had different hardware revisions of the same wifi card installed (ChipRev
> 20 vs 21).
> 
> So it seems pretty clear that with revision 21 they fixed some hardware
> bug that causes those spurious wakeups.

Seems reasonable, thanks for the thorough handling!

> FWIW, obviously a proper workaround for this would have to be implemented
> in the firmware.

Yeah, but you only get those if you're a paying customer apparently :(

I wonder if the original OEM got firmware fixes, but because they don't
use Linux, nobody bothered to roll those into the linux-firmware repo.

Brian