[PATCH 0/3] mwifiex: disable ps_mode by default for stability

[PATCH 0/3] mwifiex: disable ps_mode by default for stability

[Tsuchiya Yuto]

Hello all,

On Microsoft Surface devices (PCIe-88W8897), we are observing stability
issues when ps_mode (IEEE power_save) is enabled, then eventually causes
firmware crash. Especially on 5GHz APs, the connection is completely
unstable and almost unusable.

I think the most desirable change is to fix the ps_mode itself. But is
seems to be hard work [1], I'm afraid we have to go this way.

Therefore, the first patch of this series disables the ps_mode by default
instead of enabling it on driver init. I'm not sure if explicitly
disabling it is really required or not. I don't have access to the details
of this chip. Let me know if it's enough to just remove the code that
enables ps_mode.

The Second patch adds a new module parameter named "allow_ps_mode". Since
other wifi drivers just disable power_save by default by module parameter
like this, I also added this.

The third patch adds a message when ps_mode will be changed. Useful when
diagnosing connection issues.

Thanks,
Tsuchiya Yuto

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

Tsuchiya Yuto (3):
  mwifiex: disable ps_mode explicitly by default instead
  mwifiex: add allow_ps_mode module parameter
  mwifiex: print message when changing ps_mode

 .../net/wireless/marvell/mwifiex/cfg80211.c   | 23 +++++++++++++++++++
 .../net/wireless/marvell/mwifiex/sta_cmd.c    | 11 ++++++---
 2 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.29.1

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

[Tsuchiya Yuto]

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 <kitakar@gmail.com>
---
 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

[PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Tsuchiya Yuto]

To make the ps_mode (power_save) control easier, this commit adds a new
module parameter allow_ps_mode and set it false (disallowed) by default.

When this parameter is set to false, changing the power_save mode will
be disallowed like the following:

    $ sudo iw dev mlan0 set power_save on
    command failed: Operation not permitted (-1)

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index a6b9dc6700b14..943bc1e8ceaee 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -25,6 +25,11 @@
 static char *reg_alpha2;
 module_param(reg_alpha2, charp, 0);
 
+static bool allow_ps_mode;
+module_param(allow_ps_mode, bool, 0644);
+MODULE_PARM_DESC(allow_ps_mode,
+		 "allow WiFi power management to be enabled. (default: disallowed)");
+
 static const struct ieee80211_iface_limit mwifiex_ap_sta_limits[] = {
 	{
 		.max = MWIFIEX_MAX_BSS_NUM,
@@ -435,6 +440,17 @@ mwifiex_cfg80211_set_power_mgmt(struct wiphy *wiphy,
 
 	ps_mode = enabled;
 
+	/* Allow ps_mode to be enabled only when allow_ps_mode is true */
+	if (ps_mode && !allow_ps_mode) {
+		mwifiex_dbg(priv->adapter, MSG,
+			    "Enabling ps_mode disallowed by modparam\n");
+
+		/* Return -EPERM to inform userspace tools that setting
+		 * power_save to be enabled is not permitted.
+		 */
+		return -EPERM;
+	}
+
 	return mwifiex_drv_set_power(priv, &ps_mode);
 }
 
-- 
2.29.1

[PATCH 3/3] mwifiex: print message when changing ps_mode

[Tsuchiya Yuto]

Users may want to know the ps_mode state change (e.g., diagnosing
connection issues). This commit adds the print when changing ps_mode.

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 943bc1e8ceaee..a2eb8df8d3854 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -451,6 +451,13 @@ mwifiex_cfg80211_set_power_mgmt(struct wiphy *wiphy,
 		return -EPERM;
 	}
 
+	if (ps_mode)
+		mwifiex_dbg(priv->adapter, MSG,
+			    "Enabling ps_mode, disable if unstable.\n");
+	else
+		mwifiex_dbg(priv->adapter, MSG,
+			    "Disabling ps_mode.\n");
+
 	return mwifiex_drv_set_power(priv, &ps_mode);
 }
 
-- 
2.29.1

Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Brian Norris]

On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
>
> To make the ps_mode (power_save) control easier, this commit adds a new
> module parameter allow_ps_mode and set it false (disallowed) by default.

This sounds like a bad idea, as it breaks all the existing users who
expect this feature to be allowed. Seems like you should flip the
defaults. Without some better justification, NACK.

Also, I can't find the other 2 patches in this alleged series. Maybe
they're still making it through the mailing lists and archives.

Brian

> When this parameter is set to false, changing the power_save mode will
> be disallowed like the following:
>
>     $ sudo iw dev mlan0 set power_save on
>     command failed: Operation not permitted (-1)
>
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>

Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Willem de Bruijn]

On Wed, Oct 28, 2020 at 9:13 PM Brian Norris <briannorris@chromium.org> wrote:
>
> On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> >
> > To make the ps_mode (power_save) control easier, this commit adds a new
> > module parameter allow_ps_mode and set it false (disallowed) by default.

This sounds like some form of access control, not something that makes
power control "easier"? What exactly is the use case.

Also, module params in networking devices are discouraged.

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

[Brian Norris]

On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto <kitakar@gmail.com> 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 <kitakar@gmail.com>

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

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

[Andy Shevchenko]

On Thu, Oct 29, 2020 at 8:29 PM Brian Norris <briannorris@chromium.org> wrote:
> On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto <kitakar@gmail.com> 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

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

[Brian Norris]

On Thu, Oct 29, 2020 at 11:37 AM Andy Shevchenko
<andy.shevchenko@gmail.com> 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

Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Tsuchiya Yuto]

On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote:
> On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> > 
> > To make the ps_mode (power_save) control easier, this commit adds a new
> > module parameter allow_ps_mode and set it false (disallowed) by default.
> 
> This sounds like a bad idea, as it breaks all the existing users who
> expect this feature to be allowed. Seems like you should flip the
> defaults. Without some better justification, NACK.

Thanks for the review! I wanted to open a discussion widely and wanted
to ask from the upstream developers the direction of how this stability
issue should be resolved.

I added the link to the Bugzilla in the cover-letter (that should have
arrived on the mailing list now), but I should have added this to every
commit as well:

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109681

This stability issue exists for a long time. I also submitted there the
required kernel log and device_dump more than three months ago. However,
unfortunately, it's not fixed yet. So, I have to send a series like this.

If we know that the power_save feature is broken (on some devices), I
think it should be fixed in either firmware or driver for the affected
devices. It makes no sense to keep enabling the broken features by
default.

Because userspace tools sometimes try to enable power_save anyway
regardless of default driver settings (expecting it's not broken, but
in fact it's broken), the module parameter like this is required in
addition to the first patch of this series. The commit 8298383c2cd5
("ath9k: Do not support PowerSave by default") also does the same thing
for this purpose.

On the other hand, I agree that I don't want to break the existing users.
As you mentioned in the reply to the first patch, I can set the default
value of this parameter depending on the chip id (88W8897) or DMI matching.

> Also, I can't find the other 2 patches in this alleged series. Maybe
> they're still making it through the mailing lists and archives.

Yes, there seems to be a problem with the mailing list at the time.
All the other patches I sent have arrived by now.

> Brian
> 
> > When this parameter is set to false, changing the power_save mode will
> > be disallowed like the following:
> > 
> >     $ sudo iw dev mlan0 set power_save on
> >     command failed: Operation not permitted (-1)
> > 
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>

Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Tsuchiya Yuto]

On Thu, 2020-10-29 at 13:04 -0400, Willem de Bruijn wrote:
> On Wed, Oct 28, 2020 at 9:13 PM Brian Norris <briannorris@chromium.org> wrote:
> > 
> > On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> > > 
> > > To make the ps_mode (power_save) control easier, this commit adds a new
> > > module parameter allow_ps_mode and set it false (disallowed) by default.
> 
> This sounds like some form of access control, not something that makes
> power control "easier"? What exactly is the use case.

Thanks for the review!

As I replied to Brian, userspace tools sometimes try to enable power_save
anyway regardless of default driver settings (expecting it's not broken,
but in fact it's broken), the module parameter like this is required.

So, the commit message is misleading. What will be "easier" is letting
userspace tools know power_save should be off, not the procedure of
toggling ps_mode state in the driver.

> Also, module params in networking devices are discouraged.

Even though it should be avoided, some upstream drivers provide a module
parameter like this to let users enable it if needed (since they disable
power_save by default because of stability on some devices) likw this
commit 0172b0292649 ("iwlagn: add power_save module parameter").

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

[Tsuchiya Yuto]

On Thu, 2020-10-29 at 11:25 -0700, Brian Norris wrote:
> On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto <kitakar@gmail.com> 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 <kitakar@gmail.com>
> 
> 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.

Thanks for the review! I didn't know about the flag, much appreciated.
By setting the flag to false explicitly, indeed userspace doesn't try
to enable power_save now at least for this short amount of time. I wonder
if I can drop the second patch (adding module parameter) now. But I still
want to make sure that power_save won't be enabled by userspace tools by
default.

Regarding quirks, I also don't want to break existing users. So, of course
I can try to use the quirk framework if we really can't fix the firmware.

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

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.

> As-is, NAK.
> 
> Brian

Re: [PATCH 0/3] mwifiex: disable ps_mode by default for stability

[Tsuchiya Yuto]

On Wed, 2020-10-28 at 17:21 +0200, Andy Shevchenko wrote:
> On Wed, Oct 28, 2020 at 11:24:30PM +0900, Tsuchiya Yuto wrote:
> > Hello all,
> > 
> > On Microsoft Surface devices (PCIe-88W8897), we are observing stability
> > issues when ps_mode (IEEE power_save) is enabled, then eventually causes
> > firmware crash. Especially on 5GHz APs, the connection is completely
> > unstable and almost unusable.
> > 
> > I think the most desirable change is to fix the ps_mode itself. But is
> > seems to be hard work [1], I'm afraid we have to go this way.
> > 
> > Therefore, the first patch of this series disables the ps_mode by default
> > instead of enabling it on driver init. I'm not sure if explicitly
> > disabling it is really required or not. I don't have access to the details
> > of this chip. Let me know if it's enough to just remove the code that
> > enables ps_mode.
> > 
> > The Second patch adds a new module parameter named "allow_ps_mode". Since
> > other wifi drivers just disable power_save by default by module parameter
> > like this, I also added this.
> > 
> > The third patch adds a message when ps_mode will be changed. Useful when
> > diagnosing connection issues.
> 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=109681
> 
> Can you attach this to the actual patch as BugLink: tag?
> 

Thanks! Indeed I should have added this... I wrote it in the replies.
If I send the v2 version of this series, I'll add it to them.

Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Andy Shevchenko]

On Fri, Oct 30, 2020 at 04:58:33PM +0900, Tsuchiya Yuto wrote:
> On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote:

...

> On the other hand, I agree that I don't want to break the existing users.
> As you mentioned in the reply to the first patch, I can set the default
> value of this parameter depending on the chip id (88W8897) or DMI matching.

Since it's a PCIe device you already have ID table where you may add a
driver_data with what ever quirks are needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] mwifiex: add allow_ps_mode module parameter

[Tsuchiya Yuto]

On Fri, 2020-10-30 at 13:02 +0200, Andy Shevchenko wrote:
> On Fri, Oct 30, 2020 at 04:58:33PM +0900, Tsuchiya Yuto wrote:
> > On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote:
> 
> ...
> 
> > On the other hand, I agree that I don't want to break the existing users.
> > As you mentioned in the reply to the first patch, I can set the default
> > value of this parameter depending on the chip id (88W8897) or DMI matching.
> 
> Since it's a PCIe device you already have ID table where you may add a
> driver_data with what ever quirks are needed.

Sorry that my comment was misleading. I meant using the quirk framework
(that is based on DMI matching) I sent in another series. This applies
to the other replies from me.

However, thanks to your comment, I remembered that currently, the quirk
framework can be used only within pcie.c file. For example, the quirk
initialization is currently done in pcie.c file. The mwifiex driver is
divided into interface-specific modules (PCIe, SDIO, USB) (e.g.,
mwifiex_pcie module for PCIe interface) + common module (mwifiex module).

So, I need to extend the quirk framework so that it can be used by the
mwifiex module globally.

I'll make a v2 version of this series with using the updated quirk
framework so that it won't change behaviors for existing users.

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

[Brian Norris]

On Fri, Oct 30, 2020 at 1:04 AM Tsuchiya Yuto <kitakar@gmail.com> 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

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

[Tsuchiya Yuto]

On Fri, 2020-11-20 at 13:04 -0800, Brian Norris wrote:
> On Fri, Oct 30, 2020 at 1:04 AM Tsuchiya Yuto <kitakar@gmail.com> 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

Re: [PATCH 0/3] mwifiex: disable ps_mode by default for stability

[Pali Rohár]

Hello!

Please CC me in future for mwifiex discussion :-)

On Wednesday 28 October 2020 23:24:30 Tsuchiya Yuto wrote:
> Hello all,
> 
> On Microsoft Surface devices (PCIe-88W8897), we are observing stability
> issues when ps_mode (IEEE power_save) is enabled, then eventually causes
> firmware crash. Especially on 5GHz APs, the connection is completely
> unstable and almost unusable.
> 
> I think the most desirable change is to fix the ps_mode itself. But is
> seems to be hard work [1], I'm afraid we have to go this way.
> 
> Therefore, the first patch of this series disables the ps_mode by default
> instead of enabling it on driver init. I'm not sure if explicitly
> disabling it is really required or not. I don't have access to the details
> of this chip. Let me know if it's enough to just remove the code that
> enables ps_mode.
> 
> The Second patch adds a new module parameter named "allow_ps_mode". Since
> other wifi drivers just disable power_save by default by module parameter
> like this, I also added this.
> 
> The third patch adds a message when ps_mode will be changed. Useful when
> diagnosing connection issues.

There are more issues with power save API and implementation in mwifiex.

See my email for more details:
https://lore.kernel.org/linux-wireless/20200609111544.v7u5ort3yk4s7coy@pali/T/#u

These patches would just break power save API and reporting status to
userspace even more due to WIPHY_FLAG_PS_ON_BY_DEFAULT and
CONFIG_CFG80211_DEFAULT_PS options.

I would suggest to first fix issues mentioned in my email and then start
providing a way how to blacklist or whitelist power save feature
depending on firmware or card/chip version.

From my experience I know that e.g. 88W8997 cards have lot of bugs in
their firmware and I'm not aware that bugs are going to be fixed... So
we really need workarounds, like disabling power save mode to have cards
usable.

> Thanks,
> Tsuchiya Yuto
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=109681
> 
> Tsuchiya Yuto (3):
>   mwifiex: disable ps_mode explicitly by default instead
>   mwifiex: add allow_ps_mode module parameter
>   mwifiex: print message when changing ps_mode
> 
>  .../net/wireless/marvell/mwifiex/cfg80211.c   | 23 +++++++++++++++++++
>  .../net/wireless/marvell/mwifiex/sta_cmd.c    | 11 ++++++---
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> -- 
> 2.29.1
>