On 9/22/21 2:50 PM, Jonas Dreßler wrote:
On 9/20/21 7:48 PM, Brian Norris wrote:
On Sat, Sep 18, 2021 at 12:37 AM Jonas Dreßler <verdre@xxxxxxx> wrote:
Thanks for the pointer to that commit Brian, it turns out this is
actually the change that causes the "Firmware wakeup failed" issues that
I'm trying to fix with the second patch here.
Huh. That's interesting, although I guess it makes some sense given
your theory of "dropped writes". FWIW, this strategy (post a single
write, then wait for wakeup) is the same used by some other
chips/drivers too (e.g., ath10k/pci), although in those cases card
wakeup is much much faster. But if the bus was dropping writes
somehow, those strategies would fail too.
Also my approach is a lot messier than just reverting
062e008a6e83e7c4da7df0a9c6aefdbc849e2bb3 and also appears to be blocking
even longer...
For the record, in case you're talking about my data ("blocking even
longer"): I was only testing patch 1. Patch 2 isn't really relevant to
my particular systems (Rockchip RK3399 + Marvell 8997/PCIe), because
(a) I'm pretty sure my system isn't "dropping" any reads or writes
(b) all my delay is in the read-back; the Rockchip PCIe bus is waiting
indefinitely for the card to wake up, instead of timing out and
reporting all-1's like many x86 systems appear to do (I've tested
this).
So, the 6ms delay is entirely sitting in the ioread32(), not a delay
loop.
I haven't yet tried your version 2 (which avoids the blocking read to
wake up; good!), but it sounds like in theory it could solve your
problem while avoiding 6ms delays for me. I intend to test your v2
this week.
With "blocking even longer" I meant that (on my system) the delay-loop
blocks even longer than waking up the card via mwifiex_read_reg() (both
are in the orders of milliseconds). And given that in certain cases the
card wakeup (or a write getting through to the card, I have no idea) can
take extremely long, I'd feel more confident going with the
mwifiex_read_reg() method to wake up the card.
Anyway, you know what's even weirder with all this: I've been testing
the first commit of patch v2 (so just the single read-back instead of
the big hammer) together with 062e008a6e83e7c4da7df0a9c6aefdbc849e2bb3
reverted for a good week now and haven't seen any wakeup failure yet.
Otoh I'm fairly sure the big hammer with reading back every write wasn't
enough to fix the wakeup failures, otherwise I wouldn't even have
started working on the second commit.
So that would mean there's a difference between writing and then reading
back vs only reading to wake up the card: Only the latter fixes the
wakeup failures.
Does anyone have an idea what could be the reason for the posted write
not going through, or could that also be a potential firmware bug in the
chip?
I have no clue about that. That does sound downright horrible, but so
are many things when dealing with this family of hardware/firmware.
I'm not sure how to prove out whether this is a host bus problem, or
an endpoint/firmware problem, other than perhaps trying the same
module/firmware on another system, if that's possible.
Anyway, to reiterate: I'm not fundamentally opposed to v2 (pending a
test run here), even if it is a bit ugly and perhaps not 100%
understood.
I'm not 100% sure about all this yet, I think I'm gonna try to confirm
my older findings once again now and then we'll see. FTR, would you be
fine with using the mwifiex_read_reg() method to wake up the card and
somehow quirking your system to use write_reg()?
Brian
Okay, so I finally managed to find my exact reproducer for the bug again:
1) Make sure wifi powersaving is enabled (iw dev wlp1s0 set power_save on)
2) Connect to any wifi network (makes firmware go into wifi powersaving
mode, not deep sleep)
3) Make sure bluetooth is turned off (to ensure the firmware actually
enters powersave mode and doesn't keep the radio active doing bluetooth
stuff)
4) To confirm that wifi powersaving is entered ping a device on the LAN,
pings should be a few ms higher than without powersaving
5) Run "while true; do iwconfig; sleep 0.0001; done", this wakes and
suspends the firmware extremely often
6) Wait until things explode, for me it consistently takes <5 minutes
Using this reproducer I was able to clear things up a bit:
- There still are wakeup failures when using (only) mwifiex_read_reg()
to wake the card, so there's no weird difference between waking up using
read vs write+read-back
- Just calling mwifiex_write_reg() once and then blocking until the card
wakes up using my delay-loop doesn't fix the issue, it's actually
writing multiple times that fixes the issue
These observations sound a lot like writes (and even reads) are actually
being dropped, don't they?
