| Message ID | 20210830123704.221494-3-verdre@v0yd.nl |
|---|---|
| State | New |
| Headers | show |
| Series | mwifiex: Work around firmware bugs on 88W8897 chip | expand |
On Mon, Aug 30, 2021 at 3:39 PM Jonas Dreßler <verdre@v0yd.nl> wrote: > > It seems that the firmware of the 88W8897 card sometimes ignores or > misses when we try to wake it up by reading the firmware status > register. This leads to the firmware wakeup timeout expiring and the > driver resetting the card because we assume the firmware has hung up or > crashed (unfortunately that's not unlikely with this card). > > Turns out that most of the time the firmware actually didn't hang up, > but simply "missed" our wakeup request and doesn't send us an AWAKE didn't > event. > > Trying again to read the firmware status register after a short timeout > usually makes the firmware wake we up as expected, so add a small retry wake up > loop to mwifiex_pm_wakeup_card() that looks at the interrupt status to > check whether the card woke up. > > The number of tries and timeout lengths for this were determined > experimentally: The firmware usually takes about 500 us to wake up > after we attempt to read the status register. In some cases where the > firmware is very busy (for example while doing a bluetooth scan) it > might even miss our requests for multiple milliseconds, which is why > after 15 tries the waiting time gets increased to 10 ms. The maximum > number of tries it took to wake the firmware when testing this was > around 20, so a maximum number of 50 tries should give us plenty of > safety margin. > > A good reproducer for this issue is letting the firmware sleep and wake > up in very short intervals, for example by pinging an device on the a device > network every 0.1 seconds. ... > + /* Access the fw_status register to wake up the device. > + * Since the 88W8897 firmware sometimes appears to ignore or miss > + * that wakeup request, we continue trying until we receive an > + * interrupt from the card. > + */ > + do { > + if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) { > + mwifiex_dbg(adapter, ERROR, > + "Writing fw_status register failed\n"); > + return -1; > + } > + > + n_tries++; > + > + if (n_tries <= 15) > + usleep_range(400, 700); > + else > + msleep(10); > + } while (n_tries <= 50 && READ_ONCE(adapter->int_status) == 0); NIH read_poll_timeout() from iopoll.h.
On Mon, Aug 30, 2021 at 3:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Aug 30, 2021 at 3:39 PM Jonas Dreßler <verdre@v0yd.nl> wrote: ... > > + do { > > + if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) { > > + mwifiex_dbg(adapter, ERROR, > > + "Writing fw_status register failed\n"); > > + return -1; Please, use proper code. -EIO or so. > > + } > > + > > + n_tries++; > > + > > + if (n_tries <= 15) > > + usleep_range(400, 700); > > + else > > + msleep(10); > > + } while (n_tries <= 50 && READ_ONCE(adapter->int_status) == 0); Can you use definitions for 15 and 50? > NIH read_poll_timeout() from iopoll.h. On the second thought it might be not optimal to use it (requires anyway an additional function and doesn't provide an abortion on error). Just see above.
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index bfd6e135ed99..14742bdb96ef 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -658,6 +658,7 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter) { struct pcie_service_card *card = adapter->card; const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; + int n_tries = 0; mwifiex_dbg(adapter, EVENT, "event: Wakeup device...\n"); @@ -665,12 +666,28 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter) if (reg->sleep_cookie) mwifiex_pcie_dev_wakeup_delay(adapter); - /* Accessing fw_status register will wakeup device */ - if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) { - mwifiex_dbg(adapter, ERROR, - "Writing fw_status register failed\n"); - return -1; - } + /* Access the fw_status register to wake up the device. + * Since the 88W8897 firmware sometimes appears to ignore or miss + * that wakeup request, we continue trying until we receive an + * interrupt from the card. + */ + do { + if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) { + mwifiex_dbg(adapter, ERROR, + "Writing fw_status register failed\n"); + return -1; + } + + n_tries++; + + if (n_tries <= 15) + usleep_range(400, 700); + else + msleep(10); + } while (n_tries <= 50 && READ_ONCE(adapter->int_status) == 0); + + mwifiex_dbg(adapter, EVENT, + "event: Tried %d times until firmware woke up\n", n_tries); if (reg->sleep_cookie) { mwifiex_pcie_dev_wakeup_delay(adapter);
It seems that the firmware of the 88W8897 card sometimes ignores or misses when we try to wake it up by reading the firmware status register. This leads to the firmware wakeup timeout expiring and the driver resetting the card because we assume the firmware has hung up or crashed (unfortunately that's not unlikely with this card). Turns out that most of the time the firmware actually didn't hang up, but simply "missed" our wakeup request and doesn't send us an AWAKE event. Trying again to read the firmware status register after a short timeout usually makes the firmware wake we up as expected, so add a small retry loop to mwifiex_pm_wakeup_card() that looks at the interrupt status to check whether the card woke up. The number of tries and timeout lengths for this were determined experimentally: The firmware usually takes about 500 us to wake up after we attempt to read the status register. In some cases where the firmware is very busy (for example while doing a bluetooth scan) it might even miss our requests for multiple milliseconds, which is why after 15 tries the waiting time gets increased to 10 ms. The maximum number of tries it took to wake the firmware when testing this was around 20, so a maximum number of 50 tries should give us plenty of safety margin. A good reproducer for this issue is letting the firmware sleep and wake up in very short intervals, for example by pinging an device on the network every 0.1 seconds. Signed-off-by: Jonas Dreßler <verdre@v0yd.nl> --- drivers/net/wireless/marvell/mwifiex/pcie.c | 29 ++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-)