Re: [PATCH rtw-next v2] wifi: rtw88: pci: wake the TX queues when the rings are reset

[Abdurrahman Karadag]

> I re-think ring->queue_stopped and restart work. I feel the flow
> should be [...]
> I'll discuss above flow internally, and give back to you.

I traced the three steps against rtw-next. 2.1 looks more necessary than
I expected, 2.2 I need one clarification on, and 3 is v3.

2.1. ieee80211_restart_hw() does stop every queue, but only after the
teardown has run:

  rtw_fw_recovery_work()
      __fw_recovery_work()      <- rtw_enter_ips() on its last line
      ieee80211_restart_hw()    <- queues stopped here

RTW_FLAG_RUNNING is cleared in the rtw_core_stop() that rtw_enter_ips()
calls, so for almost all of the teardown neither flag test blocks a
frame - not the one in rtw_ops_tx(), not the one in
rtw_ops_wake_tx_queue(). And the guards do less than I thought. rtw88
does not set STA_MMPDU_TXQ, so in STA mode mgmt and nulldata fall to
rtw_ops_tx(), which frees the skb as I said last mail, while data goes
through .wake_tx_queue, which frees nothing - those frames stay queued,
and __rtw_tx_work() tests nothing at all.

The reset does unlist them - rtw_sta_remove() and rtw_reset_vif_iter()
call rtw_txq_cleanup() on every sta and vif txq - but those calls sit
just above rtw_enter_ips(), and the flag is still set until
rtw_core_stop() clears it, so .wake_tx_queue can relist a txq in
between; for AC_VO it runs __rtw_tx_work() inline. rtw_core_stop() does
not cancel tx_work, so one queued in that window can still be writing
descriptors and ringing the doorbell while the MAC is powered off.
ieee80211_stop_queues() at the top closes the TXQ path too, because
ieee80211_tx_dequeue() returns NULL on a stopped queue. So I would
keep 2.1.

The one thing I would check in 2.1 is the reason code.
ieee80211_stop_queues() uses IEEE80211_QUEUE_STOP_REASON_DRIVER, which
mac80211 does not unblock by itself - restart_hw() picks SUSPEND
deliberately, with the comment "use this reason, ieee80211_reconfig will
unblock it". And ieee80211_reconfig() can return before
reconfig_complete() is reached: if drv_start() fails it wakes only
SUSPEND and bails. A DRIVER stop taken in 2.1 would be left set on that
path, with step 3 never running.

2.2. There is no ieee80211_wake_queues() on that path today - pci.c has
one, in rtw_pci_io_resume() - so I read "(but not call
ieee80211_wake_queues() if restarting)" two ways: either the reset gets
a wake beside it for the callers that are not a restart, gated on
RTW_FLAG_RESTARTING, or the reset stands alone and step 3 owns the wake.
Which did you mean? The flag fits either way: it is set on the first
line of __fw_recovery_work() and cleared in rtw_reconfig_complete(), so
it is set across the whole teardown, rtw_pci_dma_release() included.

With a wake for the non-restart callers I have no question. With the
reset alone, that path is also reached outside a restart: rtw_enter_ips()
when mac80211 goes idle - which is how the restart itself gets there -
and rtw_ops_stop(), for a plain "ip link set wlan0 down/up" and for
suspend. Step 3 covers only the restart: ieee80211_reconfig() has two
callers, ieee80211_restart_work() and __ieee80211_resume(), so a down/up
never reaches rtw_reconfig_complete() with any reconfig_type at all. And
rtw_leave_ips() -> rtw_core_start() does not wake anything either.

Clearing the flag itself costs nothing, and it is what I offered last
mail - after rtw_pci_dma_release() the indices are rewound and the skbs
freed, so count is 0, and for as long as mac80211 holds a DRIVER stop on
that queue nothing completes, so the wake branch inside
rtw_pci_tx_isr()'s loop cannot run whether the flag is set or not. What
persists is mac80211's DRIVER slot, and that part is self-sustaining:
while it is set neither path reaches rtw_pci_tx_write() -
ieee80211_tx_dequeue() returns NULL for the TXQ frames, and
ieee80211_tx_frags() parks the rest on local->pending[] instead of
calling .tx - so nothing sets queue_stopped again.

3. That is v3 and the part I have measured. Luka Gejak's point from the
v3 thread sits here: rtw_reconfig_complete() also runs after a plain
resume, with RECONFIG_TYPE_SUSPEND. If the wake does go away there,
covering both types would close the resume case; the down/up case needs
something else, for the reason above. v3 stands on the list in the
meantime, and I am holding the respin until you come back with the flow,
so your decision and Luka's points land in the same version.

Your other two questions were in the bug thread; bringing them here as
you asked.

> could you describe how you resolve the stuck (by the method of your
> previous patch, maybe v1)?

Nothing the driver did on its own. Over the five events, power save off:

  rewrite the TXBD host index   0/3   read index never moved
  ip neigh flush                0/4
  iwd reconnect, new PTK        2/4   cleared 08-28 20:32, 09-06 03:17
  ip link set wlan0 down/up     1/2   cleared 09-14 17:48
  reboot                              always

The ladder stops at the first step that works, so the denominators are
not comparable, and the down/up row carries the caveat I sent on 3
October: on 09-14 19:09 the reconnect did fail - the AP answered status
30, "rejected association temporarily" - and the timeout at the down/up
step was my ladder giving up after 25 s rather than the outcome. The
journal has the interface up 3 s after the down/up, and the station
reassociated, with a fresh lease and no reboot, some 90 s after that -
but through iwd's own autoconnect, which it had fallen back to at the
status 30, eleven seconds before the down/up. So I cannot credit the
down/up with it either, and I have left the row at what the probe
measured. The doorbell rewrite is the patch I withdrew.

What the five do separate is narrower: nothing short of tearing the link
down ever cleared one. The two steps that never worked leave the
association alone, and every step that did clear one tore the
association or the interface down and rebuilt it - the down/up through
the driver's own rtw_ops_stop() -> rtw_core_stop() path. I cannot rank
the reconnect against the down/up, because the ladder gave the reconnect
the earlier attempts.

rtw_fw_recovery() also tears down and rebuilds, through the same
rtw_core_stop() -> rtw_power_off() -> rtw_hci_stop() -> rtw_pci_stop()
-> rtw_pci_dma_release() path. In the controlled stall I sent on 3
October it did reinitialise the ring and still left the BE stop reason
at 0x1, with nothing back in the 90 s I watched, while the same script
with patch 1 cleared the reason and had traffic back within 2 s. That is
the one measurement I have that bears on a workaround, and it is
narrower than it looks: the restart also cleared the REG_TXPAUSE I had
set, and in the field TXPAUSE was 0x00 already.

One thing I should flag before you read the dumps. My ladder pokes
debugfs fw_crash anyway, so nine of the captures print
"rtw_fw_recovery() tetikleniyor" - Turkish for "being triggered" - six
on 27 September, two on 14 October, and one where the knob was not
writable. Eight of them follow it with "fw_crash ->
ieee80211_restart_hw: 100% packet loss". Both strings are labels my
script prints for that rung, not anything the driver reported.
rtw_debugfs_set_fw_crash() never calls rtw_fw_recovery(): it leaves deep
LPS, sets RTW_FLAG_RESTART_TRIGGERING and writes REG_HRCV_MSG, and
recovery is entered only through the C2H path I described on 3 October,
which matches your point that the knob only takes effect on 8822C. So
nothing was triggered and nothing was tested at that rung. Section 4 of
the six September dumps also reads fw_crash as 1 before that dump's own
write, because the file prints RTW_FLAG_RESTART_TRIGGERING and only
__fw_recovery_work() clears it; all six are the same boot. The two
October ones are later boots and read 0.

> I suggest to turn it off for all experiments

Agreed. It is off at all five, and no dump since 01:00 on 1 September
records it as on - partly by construction, though: the watchdog forces
it off at the top of the same 20 s pass that takes the dump, and it has
had to do that about a hundred times since, because an iface down/up
returns it to the kernel default, as I said in the bug thread.