| Message ID | 20211028073729.24408-1-verdre@v0yd.nl (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mwifiex: Add quirk to disable deep sleep with certain hardware revision | expand |
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); > +}
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, >
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
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 >
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.
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...
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.
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
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,
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(+)