lkml.org 
[lkml]   [2021]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt
    On 9/14/21 13:48, Jonas Dreßler wrote:
    > It seems that the firmware of the 88W8897 card sometimes ignores or
    > misses when we try to wake it up by writing to 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 didn't send us an AWAKE
    > event.
    >
    > Trying again to read the firmware status register after a short timeout
    > usually makes the firmware wake 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 a device on the
    > network every 0.1 seconds.
    >
    > Cc: stable@vger.kernel.org
    > Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
    > ---
    > drivers/net/wireless/marvell/mwifiex/pcie.c | 33 +++++++++++++++++----
    > 1 file changed, 27 insertions(+), 6 deletions(-)
    >
    > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
    > index 0eff717ac5fa..7fea319e013c 100644
    > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
    > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
    > @@ -661,11 +661,15 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter,
    > "max count reached while accessing sleep cookie\n");
    > }
    >
    > +#define N_WAKEUP_TRIES_SHORT_INTERVAL 15
    > +#define N_WAKEUP_TRIES_LONG_INTERVAL 35
    > +
    > /* This function wakes up the card by reading fw_status register. */
    > 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");
    > @@ -673,12 +677,29 @@ 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 -EIO;
    > + }
    > +
    > + n_tries++;
    > +
    > + if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
    > + usleep_range(400, 700);
    > + else
    > + msleep(10);
    > + } while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
    > + 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);
    >

    So I think I have another solution that might be a lot more elegant, how
    about this:

    try_again:
    n_tries++;

    mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE);

    if (wait_event_interruptible_timeout(adapter->card_wakeup_wait_q,
    READ_ONCE(adapter->int_status) != 0,
    WAKEUP_TRY_AGAIN_TIMEOUT) == 0 &&
    n_tries < MAX_N_WAKEUP_TRIES) {
    goto try_again;
    }


    and then call wake_up_interruptible() in the mwifiex_interrupt_status()
    interrupt handler.

    This solution should make sure we always keep wakeup latency to a minimum
    and can still retry the register write if things didn't work.

    \
     
     \ /
      Last update: 2021-10-03 11:18    [W:3.893 / U:0.484 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site