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

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

[Abdurrahman Karadag]

When a TX ring fills, rtw_pci_tx_write() stops the mac80211 queue and
records that in ring->queue_stopped, which is cleared only by the
completion loop in rtw_pci_tx_isr(). Resetting the rings rewinds the
indices, so nothing that was in flight can complete and run that loop,
and the queue stays stopped over a ring that is now empty.
ieee80211_restart_hw() does not release that stop either, because
ieee80211_reconfig() wakes only IEEE80211_QUEUE_STOP_REASON_SUSPEND.

Release those queues where the reset happens. The stop side is the one
that matters: rtw_fw_recovery() reaches rtw_pci_dma_release() through
rtw_enter_ips() before it calls ieee80211_restart_hw(), so that is
where a queue stopped before the crash is still stopped. By the time
rtw_pci_setup() runs the flag is already clear.

Wake all the queues, but only if at least one ring had filled. This
cannot tell one DRIVER stop from another: rtw_pci_io_err_detected() and
the hardware scan paths in fw.c stop every queue with the same
IEEE80211_QUEUE_STOP_REASON_DRIVER, which mac80211 keeps as a single
non-refcounted slot per queue. Gating on ring->queue_stopped at least
keeps the resets that stopped nothing out of it.

On an RTL8821CE, a BE ring held until rtw_pci_tx_write() stopped its
queue did not come back from the restart. The ring itself was reset -
0x3a8 read 0x00000000 and REG_TXPAUSE was clear again - but BE's
mac80211 stop reason still read 0x1 three minutes later and TX never
resumed. With this patch it reads 0x0 and TX resumes within 2 s. The
ring was filled by writing REG_TXPAUSE 0x0f from debugfs under load,
and rtw_fw_recovery() was called from a debug build.

Signed-off-by: Abdurrahman Karadag <abdurrahmankaradag19@gmail.com>
---
v2:
 - wake with ieee80211_wake_queues() instead of recording and replaying
   per-ring queue mappings (Ping-Ke Shih)
 - drop the Fixes tag: the bug only bites once a recovery path exists,
   which the blamed commit predates (Ping-Ke Shih)
 - cut the commit message down to why and how (Ping-Ke Shih)
 - say which of the two reset call sites fires during recovery, and
   that the measured stall was induced from debugfs
 - patch 2 of the v1 series is dropped rather than resent, while the
   cause of the stall is still open

v1: https://lore.kernel.org/linux-wireless/20261002231013.11792-1-abdurrahmankaradag19@gmail.com/

 drivers/net/wireless/realtek/rtw88/pci.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 66d2e5f..4d3524e 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -475,7 +475,29 @@ static void rtw_pci_reset_buf_desc(struct rtw_dev *rtwdev)
 
 static void rtw_pci_reset_trx_ring(struct rtw_dev *rtwdev)
 {
+	struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+	enum rtw_tx_queue_type queue;
+	bool wake = false;
+
 	rtw_pci_reset_buf_desc(rtwdev);
+
+	/*
+	 * The indices are back at zero, so nothing that was in flight can
+	 * complete and reach the wake in rtw_pci_tx_isr(); a queue stopped
+	 * when its ring filled would stay stopped for good. Only wake when
+	 * that happened: the stop reason is shared with the AER and scan
+	 * paths, which wake the queues again themselves.
+	 */
+	for (queue = 0; queue < RTK_MAX_TX_QUEUE_NUM; queue++) {
+		if (!rtwpci->tx_rings[queue].queue_stopped)
+			continue;
+
+		rtwpci->tx_rings[queue].queue_stopped = false;
+		wake = true;
+	}
+
+	if (wake)
+		ieee80211_wake_queues(rtwdev->hw);
 }
 
 static void rtw_pci_enable_interrupt(struct rtw_dev *rtwdev,
-- 
2.55.0

base-commit: 83de3a16c7b37064a59305040ba9b0f93b832794

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

[Ping-Ke Shih]

Abdurrahman Karadag <abdurrahmankaradag19@gmail.com> wrote:
> When a TX ring fills, rtw_pci_tx_write() stops the mac80211 queue and
> records that in ring->queue_stopped, which is cleared only by the
> completion loop in rtw_pci_tx_isr(). Resetting the rings rewinds the
> indices, so nothing that was in flight can complete and run that loop,
> and the queue stays stopped over a ring that is now empty.
> ieee80211_restart_hw() does not release that stop either, because
> ieee80211_reconfig() wakes only IEEE80211_QUEUE_STOP_REASON_SUSPEND.

The main reason why we need this patch is ieee80211_restart_hw(), so
please point out this in subject. 

> 
> Release those queues where the reset happens. The stop side is the one
> that matters: rtw_fw_recovery() reaches rtw_pci_dma_release() through
> rtw_enter_ips() before it calls ieee80211_restart_hw(), so that is
> where a queue stopped before the crash is still stopped. By the time
> rtw_pci_setup() runs the flag is already clear.
> 
> Wake all the queues, but only if at least one ring had filled. This
> cannot tell one DRIVER stop from another: rtw_pci_io_err_detected() and
> the hardware scan paths in fw.c stop every queue with the same
> IEEE80211_QUEUE_STOP_REASON_DRIVER, which mac80211 keeps as a single
> non-refcounted slot per queue. Gating on ring->queue_stopped at least
> keeps the resets that stopped nothing out of it.
> 
> On an RTL8821CE, a BE ring held until rtw_pci_tx_write() stopped its
> queue did not come back from the restart. The ring itself was reset -
> 0x3a8 read 0x00000000 and REG_TXPAUSE was clear again - but BE's
> mac80211 stop reason still read 0x1 three minutes later and TX never
> resumed. With this patch it reads 0x0 and TX resumes within 2 s. The
> ring was filled by writing REG_TXPAUSE 0x0f from debugfs under load,
> and rtw_fw_recovery() was called from a debug build.

This isn't related to this patch, but it is still digging, so no need.

As I see, the commit message can be very simple -- wake up queues for
ieee80211_restart_hw() case.

(For the analysis in another mail, I'll read it later.)

> 
> Signed-off-by: Abdurrahman Karadag <abdurrahmankaradag19@gmail.com>
> ---
> v2:
>  - wake with ieee80211_wake_queues() instead of recording and replaying
>    per-ring queue mappings (Ping-Ke Shih)
>  - drop the Fixes tag: the bug only bites once a recovery path exists,
>    which the blamed commit predates (Ping-Ke Shih)
>  - cut the commit message down to why and how (Ping-Ke Shih)
>  - say which of the two reset call sites fires during recovery, and
>    that the measured stall was induced from debugfs
>  - patch 2 of the v1 series is dropped rather than resent, while the
>    cause of the stall is still open
> 
> v1: https://lore.kernel.org/linux-wireless/20261002231013.11792-1-abdurrahmankaradag19@gmail.com/
> 
>  drivers/net/wireless/realtek/rtw88/pci.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index 66d2e5f..4d3524e 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -475,7 +475,29 @@ static void rtw_pci_reset_buf_desc(struct rtw_dev *rtwdev)
> 
>  static void rtw_pci_reset_trx_ring(struct rtw_dev *rtwdev)

I'm thinking if here is too early to wake up queues. Should we do this in
rtw_reconfig_complete()?

Or we check flag RTW_FLAG_RESTARTING of rtwdev->flags to wake up queues.
This will be clear why we need to wake up. The comment isn't necessary
by the way.

>  {
> +       struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> +       enum rtw_tx_queue_type queue;
> +       bool wake = false;
> +
>         rtw_pci_reset_buf_desc(rtwdev);
> +
> +       /*
> +        * The indices are back at zero, so nothing that was in flight can
> +        * complete and reach the wake in rtw_pci_tx_isr(); a queue stopped
> +        * when its ring filled would stay stopped for good. Only wake when
> +        * that happened: the stop reason is shared with the AER and scan
> +        * paths, which wake the queues again themselves.
> +        */
> +       for (queue = 0; queue < RTK_MAX_TX_QUEUE_NUM; queue++) {
> +               if (!rtwpci->tx_rings[queue].queue_stopped)
> +                       continue;
> +
> +               rtwpci->tx_rings[queue].queue_stopped = false;
> +               wake = true;
> +       }
> +
> +       if (wake)
> +               ieee80211_wake_queues(rtwdev->hw);
>  }
> 
>  static void rtw_pci_enable_interrupt(struct rtw_dev *rtwdev,
> --
> 2.55.0
> 
> base-commit: 83de3a16c7b37064a59305040ba9b0f93b832794

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

[Abdurrahman Karadag]

> I'm thinking if here is too early to wake up queues. Should we do this in
> rtw_reconfig_complete()?

It is too early, and rtw_reconfig_complete() is the right place. v3 does
that.

On the reset path rtw_core_stop() has already cleared RTW_FLAG_RUNNING by
the time rtw_pci_reset_trx_ring() runs, so anything mac80211 hands the
driver between that wake and the end of the restart is freed again by
rtw_ops_tx(). reconfig_complete() is called from the wake_up label in
ieee80211_reconfig(), where the comment reads "If this is for hw restart
things are still running". It also makes the fix bus-independent instead
of PCI-only.

I measured it at the new hook before sending, with two builds of current
rtw-next that differ by nothing but this patch, using mac80211's own
tracepoints instead of a register read. Under load the BE queue is
stopped by the ring-full path, then rtw_fw_recovery() is called:

  without the patch
    511.499278  stop_queue  queue:2 reason:0      DRIVER, left dangling
    511.501625  stop_queue  queue:0-3 reason:4    SUSPEND, the restart
    511.957404  drv_reconfig_complete reconfig_type:0
    511.957411  wake_queue  queue:0-3 reason:4    only SUSPEND is woken
    AC_BE stays 0x00000001, 100% loss

  with the patch
    475.930915  drv_reconfig_complete reconfig_type:0
    475.930925  wake_queue  queue:2 reason:0      10 us later
    475.930926  wake_queue  queue:0-3 reason:4
    AC_BE reads 0x00000000, 0% loss

So the wake lands after the restart has completed and before mac80211
wakes the SUSPEND reason itself. No TXPAUSE write and no injection this
time - the ring fills on its own under load, which is also why I am happy
to leave the measurement out of the commit message.

One consequence I should flag, since v2 covered it and v3 does not. A
plain "ip link set wlan0 down/up" also reaches rtw_pci_reset_trx_ring(),
and mac80211 never clears a DRIVER stop on interface down - nothing
outside the stop/wake helpers touches local->q_stop_reasons[]. So with v3
a queue stopped before a down/up stays stopped. Is the restart case the
one you want fixed, or should a down/up release it too?

I also left ring->queue_stopped alone. It stays set until the first
completion after the restart, which clears it, and rtw_pci_tx_write()
does not read it before stopping a queue, so a stale value costs nothing.
I can clear it in rtw_pci_reset_trx_ring() if you would rather the driver
state did not claim a stop that is gone.

> As I see, the commit message can be very simple -- wake up queues for
> ieee80211_restart_hw() case.

Done, and the subject names it now. The measurement paragraph is out -
you are right that the stall belongs elsewhere.

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

[Ping-Ke Shih]

Abdurrahman Karadag <abdurrahmankaradag19@gmail.com> wrote:
> I also left ring->queue_stopped alone. It stays set until the first
> completion after the restart, which clears it, and rtw_pci_tx_write()
> does not read it before stopping a queue, so a stale value costs nothing.
> I can clear it in rtw_pci_reset_trx_ring() if you would rather the driver
> state did not claim a stop that is gone.

I re-think ring->queue_stopped and restart work. I feel the flow should be

1. fw/hw asked to restart -> rtw_fw_recovery_work() -> __fw_recovery_work()
2. __fw_recovery_work()
2.1. I think here should call ieee80211_stop_queues() to stop all TX queues
2.2. __fw_recovery_work() resets most driver states, but doesn't reset ring->queue_stopped.
     I think we can reset it (but not call ieee80211_wake_queues() if restarting) by
     rtw_pci_stop() -> rtw_pci_dma_release()
3. rtw_reconfig_complete()
   call ieee80211_wake_queues() as you did in v3.

I'll discuss above flow internally, and give back to you. 

Ping-Ke

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.

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

[Ping-Ke Shih]

Abdurrahman Karadag <abdurrahmankaradag19@gmail.com> wrote:
> > 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 discussed internally, and we suggest to add ieee80211_wake_queues() and
clear queue_stopped flags in rtw_pci_start().

This can resolve two cases -- restart_hw and interface down/up while
queue_stopped is flagging. Just like initialize or reset to variables. 

No need 3 steps I mentioned. (So I'd not reply your trace.)

> 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

So there is no solution for now. Since 3th and 4th events will stop/start
pci, if the cause to fail to recovery is flagged queue_stopped, our above
idea will be helpful. 

Another test is to remove/insert driver ko. It will reset queue states.

(I suppose you did these tests without changing any behavior as was).

[...]

> 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 need your help to summarize the dumps. I don't have much time to analyze 
them. Sorry.