2020-10-29 08:50:43

by Tsuchiya Yuto

[permalink] [raw]
Subject: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead

On Microsoft Surface devices (PCIe-88W8897), the ps_mode causes
connection unstable, especially with 5GHz APs. Then, it eventually causes
fw crash.

This commit disables ps_mode by default instead of enabling it.

Required code is extracted from mwifiex_drv_set_power().

Signed-off-by: Tsuchiya Yuto <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index d3a968ef21ef9..9b7b52fbc9c45 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -2333,14 +2333,19 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
return -1;

if (priv->bss_type != MWIFIEX_BSS_TYPE_UAP) {
- /* Enable IEEE PS by default */
- priv->adapter->ps_mode = MWIFIEX_802_11_POWER_MODE_PSP;
+ /* Disable IEEE PS by default */
+ priv->adapter->ps_mode = MWIFIEX_802_11_POWER_MODE_CAM;
ret = mwifiex_send_cmd(priv,
HostCmd_CMD_802_11_PS_MODE_ENH,
- EN_AUTO_PS, BITMAP_STA_PS, NULL,
+ DIS_AUTO_PS, BITMAP_STA_PS, NULL,
true);
if (ret)
return -1;
+ ret = mwifiex_send_cmd(priv,
+ HostCmd_CMD_802_11_PS_MODE_ENH,
+ GET_PS, 0, NULL, false);
+ if (ret)
+ return -1;
}

if (drcs) {
--
2.29.1


2020-10-29 18:29:16

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead

On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto <[email protected]> wrote:
>
> On Microsoft Surface devices (PCIe-88W8897), the ps_mode causes
> connection unstable, especially with 5GHz APs. Then, it eventually causes
> fw crash.
>
> This commit disables ps_mode by default instead of enabling it.
>
> Required code is extracted from mwifiex_drv_set_power().
>
> Signed-off-by: Tsuchiya Yuto <[email protected]>

You should read up on WIPHY_FLAG_PS_ON_BY_DEFAULT and
CONFIG_CFG80211_DEFAULT_PS, and set/respect those appropriately (hint:
mwifiex sets WIPHY_FLAG_PS_ON_BY_DEFAULT, and your patch makes this a
lie). Also, this seems like a quirk that you haven't properly worked
out -- if you're working on a quirk framework in your other series,
you should just key into that.

For the record, Chrome OS supports plenty of mwifiex systems with 8897
(SDIO only) and 8997 (PCIe), with PS enabled, and you're hurting
those. Your problem sounds to be exclusively a problem with the PCIe
8897 firmware.

As-is, NAK.

Brian

2020-10-29 18:38:27

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead

On Thu, Oct 29, 2020 at 8:29 PM Brian Norris <[email protected]> wrote:
> On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto <[email protected]> wrote:

...

> For the record, Chrome OS supports plenty of mwifiex systems with 8897
> (SDIO only) and 8997 (PCIe), with PS enabled, and you're hurting
> those. Your problem sounds to be exclusively a problem with the PCIe
> 8897 firmware.

And this feeling (that it's a FW issue) what I have. But the problem
here, that Marvell didn't fix and probably won't fix their FW...

Just wondering if Google (and MS in their turn) use different
firmwares to what we have available in Linux.

--
With Best Regards,
Andy Shevchenko

2020-10-29 18:57:49

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead

On Thu, Oct 29, 2020 at 11:37 AM Andy Shevchenko
<[email protected]> wrote:
> And this feeling (that it's a FW issue) what I have. But the problem
> here, that Marvell didn't fix and probably won't fix their FW...

Sure, I wouldn't hold your breath. So some of these tactics (disabling
PS, etc.) may be valid, but you have to do them smartly, acknowledging
that there are other (more stable) firmwares and chips in use for this
same driver.

> Just wondering if Google (and MS in their turn) use different
> firmwares to what we have available in Linux.

No clue about MS. But Chrom{e,ium} OS generally publishes all this
stuff where possible. You can see what we use here:

https://chromium.googlesource.com/chromiumos/third_party/linux-firmware/+/HEAD/mrvl/
https://chromium.googlesource.com/chromiumos/third_party/marvell/+/HEAD/

We try to stay somewhat in sync / parallel with "upstream"
linux-firmware, and strongly encourage vendors to send the same
binaries upstream when they hand them to us, but there are exceptions
and oversights (e.g., old products might have used a different
firmware branch).

Notably, I'll repeat: we (Chrome OS) don't actually support the PCIe
variant of 8897, so the report in question ("PCIe-88W8897") has no
equivalent in a supported Chrome OS system (even if there are binaries
in the links above, we don't use them). I would not be surprised if
there are an enormous number of firmware bugs there, as there were
initially for PCIe-88W8997 (which we do support).

Brian

2020-11-20 21:05:31

by Brian Norris

[permalink] [raw]
Subject: Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead

On Fri, Oct 30, 2020 at 1:04 AM Tsuchiya Yuto <[email protected]> wrote:
> On Thu, 2020-10-29 at 11:25 -0700, Brian Norris wrote:
> > For the record, Chrome OS supports plenty of mwifiex systems with 8897
> > (SDIO only) and 8997 (PCIe), with PS enabled, and you're hurting
> > those. Your problem sounds to be exclusively a problem with the PCIe
> > 8897 firmware.
>
> Actually, I already know that some Chromebooks use these mwifiex cards
> (but not out PCIe-88W8897) because I personally like chromiumos. I'm
> always wondering what is the difference. If the difference is firmware,
> our PCIe-88W8897 firmware should really be fixed instead of this stupid
> series.

PCIe is a very different beast. (For one, it uses DMA and
memory-mapped registers, where SDIO has neither.) It was a very
difficult slog to get PCIe/8997 working reliably for the few
Chromebooks that shipped it, and lots of that work is in firmware. I
would not be surprised if the PCIe-related changes Marvell made for
8997 never fed back into their PCIe-8897 firmware. Or maybe they only
ever launched PCIe-8897 for Windows, and the Windows driver included
workarounds that were never published to their Linux driver. But now
I'm just speculating.

> Yes, I'm sorry that I know this series is just a stupid one but I have to
> send this anyway because this stability issue has not been fixed for a
> long time. I should have added this buglink to every commit as well:
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109681
>
> If the firmware can't be fixed, I'm afraid I have to go this way. It makes
> no sense to keep enabling power_save for the affected devices if we know
> it's broken.

Condolences and sympathy, seriously. You likely have little chance of
getting the firmware fixed, so without new information (e.g,. other
workarounds?), this is the probably the right way to go.

Brian

2020-11-27 08:40:52

by Tsuchiya Yuto

[permalink] [raw]
Subject: Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default instead

On Fri, 2020-11-20 at 13:04 -0800, Brian Norris wrote:
> On Fri, Oct 30, 2020 at 1:04 AM Tsuchiya Yuto <[email protected]> wrote:
> > On Thu, 2020-10-29 at 11:25 -0700, Brian Norris wrote:
> > > For the record, Chrome OS supports plenty of mwifiex systems with 8897
> > > (SDIO only) and 8997 (PCIe), with PS enabled, and you're hurting
> > > those. Your problem sounds to be exclusively a problem with the PCIe
> > > 8897 firmware.
> >
> > Actually, I already know that some Chromebooks use these mwifiex cards
> > (but not out PCIe-88W8897) because I personally like chromiumos. I'm
> > always wondering what is the difference. If the difference is firmware,
> > our PCIe-88W8897 firmware should really be fixed instead of this stupid
> > series.
>
> PCIe is a very different beast. (For one, it uses DMA and
> memory-mapped registers, where SDIO has neither.) It was a very
> difficult slog to get PCIe/8997 working reliably for the few
> Chromebooks that shipped it, and lots of that work is in firmware. I
> would not be surprised if the PCIe-related changes Marvell made for
> 8997 never fed back into their PCIe-8897 firmware. Or maybe they only
> ever launched PCIe-8897 for Windows, and the Windows driver included
> workarounds that were never published to their Linux driver. But now
> I'm just speculating.

Thanks. Yeah, this is indeed hard work. Actually, I (and maybe also other
users) am already thankful that there is wifi driver/firmware available
on Linux :) and it'll be greater if we can fix ps_mode-related issues.

> > Yes, I'm sorry that I know this series is just a stupid one but I have to
> > send this anyway because this stability issue has not been fixed for a
> > long time. I should have added this buglink to every commit as well:
> >
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109681
> >
> > If the firmware can't be fixed, I'm afraid I have to go this way. It makes
> > no sense to keep enabling power_save for the affected devices if we know
> > it's broken.
>
> Condolences and sympathy, seriously. You likely have little chance of
> getting the firmware fixed, so without new information (e.g,. other
> workarounds?), this is the probably the right way to go.

Thank you for the pointer!

There are two issues regarding ps_mode:
1) fw crashes with "Firmware wakeup failed"
(I haven't mentioned in this series, but ps_mode also causes fw crashes)
2) connection instability (like large ping delay or even ping not reaching)

If anyone is ever interested in dmesg log with debug_mask=0xffffffff and
device_dump, I posted them to the Bugzilla [1] before.

Regarding the #2, although this is even not a workaround but I found
scanning APs will fix this. So, when I encounter this issue, I keep
scanning APs like "watch -n10 sudo iw dev ${dev_name} scan". So, it
seems that scanning APs will somehow wake wifi up? In other words, wifi
is sleeping when it shouldn't? or wifi somehow failed to wake up when
it should?

Regarding #1, we don't have any ideas yet. There is a guess that memory
leak will occur in the fw every time wifi goes into sleep, but don't know.

We even don't have the exact reproducers for both #1 and #2. What we
know so far is that, enabling ps_mode causes these issues.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=109681#c130

> Brian