[PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

In AP mode every broadcast and multicast data frame is routed to
TX_DESC_QSEL_HIGH, the after-DTIM queue, whether or not anybody is
asleep. The firmware drains that queue at beacon pace, a dozen or so
frames per second measured on RTL8822BU, while one associated client's
mDNS/SSDP chatter alone exceeds that. The excess accumulates inside
the chip until the shared TX page pool is exhausted (measured: 14 of
1803 pages left). From that point every host-sourced frame queues
behind the backlog: authentication responses reach the air seconds
after the client has given up, so no station can associate anymore,
and the beacon reserved-page download fails the BCN_VALID poll
("error beacon valid") because it needs pages from the same pool. The
AP keeps beaconing, so the failure looks like a silent RX stall and
only a reboot recovers.

mac80211 already decides when after-DTIM delivery is needed: it sets
IEEE80211_TX_CTL_SEND_AFTER_DTIM on bmc frames only while at least one
station is actually dozing. Honor that instead of routing
unconditionally: flagged frames keep going through the high queue with
the MORE_DATA and HGQMD handling introduced by commit 076f786a0ae1
("wifi: rtw88: Fix AP mode incorrect DTIM behavior"), everything else
leaves at line rate through the AC queues. This partially reverts the
usb.c hunk of that commit, whose unconditional routing is what lets
the backlog build up.

On a bench AP (USB2, 20 MHz, WPA2, hostapd, a Windows client driven
through disconnect/reconnect cycles): reconnects fail 0/5 on RTL8822BU
and 0/3 on RTL8821CU before this change, and pass 10/10 and 5/5 with
it, with the page pool staying healthy and no beacon errors logged.

Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
v3: drop the Fixes tag (Ping-Ke), reverse xmas order for the
declarations (Ping-Ke).
v2: honor IEEE80211_TX_CTL_SEND_AFTER_DTIM instead of routing bmc
unconditionally to the AC queues.

The flagged path is the code 076f786a0ae1 added and is unchanged by
this patch; I did not have a client entering powersave on this bench
to exercise it explicitly and will follow up with that measurement.

 drivers/net/wireless/realtek/rtw88/usb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
index 64e1c3420..43b9cdb4a 100644
--- a/drivers/net/wireless/realtek/rtw88/usb.c
+++ b/drivers/net/wireless/realtek/rtw88/usb.c
@@ -560,6 +560,7 @@ static int rtw_usb_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
 static u8 rtw_usb_tx_queue_mapping_to_qsel(struct sk_buff *skb)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	__le16 fc = hdr->frame_control;
 	u8 qsel;
 
@@ -567,7 +568,8 @@ static u8 rtw_usb_tx_queue_mapping_to_qsel(struct sk_buff *skb)
 		qsel = TX_DESC_QSEL_MGMT;
 	else if (is_broadcast_ether_addr(hdr->addr1) ||
 		 is_multicast_ether_addr(hdr->addr1))
-		qsel = TX_DESC_QSEL_HIGH;
+		qsel = (info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) ?
+		       TX_DESC_QSEL_HIGH : skb->priority;
 	else if (skb_get_queue_mapping(skb) <= IEEE80211_AC_BK)
 		qsel = skb->priority;
 	else
-- 
2.54.0

Re: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

Hello Ping-Ke,

One more data point that may be useful for the firmware side,
separate from this patch.

While comparing against the vendor driver I noticed a difference in
how MORE_DATA is terminated. rtw88 sets the bit on every high queue
frame (rtw_tx_fill_tx_desc(): more_data = qsel == TX_DESC_QSEL_HIGH),
including the last one. The vendor driver releases the high queue as
an explicit batch and sets mdata = 1 on all frames of the batch
except the final one, which carries 0 (chk_bmc_sleepq_hdl() in
core/rtw_mlme_ext.c of the 88x2bu source).

On both RTL8822BU and RTL8821CU I measured the high queue draining at
roughly 3 frames per DTIM with BIT_TCR_UPDATE_HGQMD set, instead of
the whole buffered burst, so the burst fetch of 076f786a0ae1 does not
seem to engage on these chips. A never-terminated MORE_DATA chain is
my best guess for why, but that is a guess; whether the hardware
needs the 0 to conclude a burst is something only the firmware
documentation can answer.

Thanks,
Mehmet

RE: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@gmail.com> wrote:
> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> 
> Hello Ping-Ke,
> 
> One more data point that may be useful for the firmware side,
> separate from this patch.
> 
> While comparing against the vendor driver I noticed a difference in
> how MORE_DATA is terminated. rtw88 sets the bit on every high queue
> frame (rtw_tx_fill_tx_desc(): more_data = qsel == TX_DESC_QSEL_HIGH),
> including the last one. The vendor driver releases the high queue as
> an explicit batch and sets mdata = 1 on all frames of the batch
> except the final one, which carries 0 (chk_bmc_sleepq_hdl() in
> core/rtw_mlme_ext.c of the 88x2bu source).
> 
> On both RTL8822BU and RTL8821CU I measured the high queue draining at
> roughly 3 frames per DTIM with BIT_TCR_UPDATE_HGQMD set, instead of
> the whole buffered burst, so the burst fetch of 076f786a0ae1 does not
> seem to engage on these chips. A never-terminated MORE_DATA chain is
> my best guess for why, but that is a guess; whether the hardware
> needs the 0 to conclude a burst is something only the firmware
> documentation can answer.

By consulting internal people and experiments, the last MORE_DATA is
unset by hardware. So I think we don't need to align behavior of
vendor driver, unless peeking the next frames before sending. 

Ping-Ke

RE: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@gmail.com> wrote:
> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> 
> In AP mode every broadcast and multicast data frame is routed to
> TX_DESC_QSEL_HIGH, the after-DTIM queue, whether or not anybody is
> asleep. The firmware drains that queue at beacon pace, a dozen or so
> frames per second measured on RTL8822BU, while one associated client's
> mDNS/SSDP chatter alone exceeds that. The excess accumulates inside
> the chip until the shared TX page pool is exhausted (measured: 14 of
> 1803 pages left). From that point every host-sourced frame queues
> behind the backlog: authentication responses reach the air seconds
> after the client has given up, so no station can associate anymore,
> and the beacon reserved-page download fails the BCN_VALID poll
> ("error beacon valid") because it needs pages from the same pool. The
> AP keeps beaconing, so the failure looks like a silent RX stall and
> only a reboot recovers.
> 
> mac80211 already decides when after-DTIM delivery is needed: it sets
> IEEE80211_TX_CTL_SEND_AFTER_DTIM on bmc frames only while at least one
> station is actually dozing. Honor that instead of routing
> unconditionally: flagged frames keep going through the high queue with
> the MORE_DATA and HGQMD handling introduced by commit 076f786a0ae1
> ("wifi: rtw88: Fix AP mode incorrect DTIM behavior"), everything else
> leaves at line rate through the AC queues. This partially reverts the
> usb.c hunk of that commit, whose unconditional routing is what lets
> the backlog build up.
> 
> On a bench AP (USB2, 20 MHz, WPA2, hostapd, a Windows client driven
> through disconnect/reconnect cycles): reconnects fail 0/5 on RTL8822BU
> and 0/3 on RTL8821CU before this change, and pass 10/10 and 5/5 with
> it, with the page pool staying healthy and no beacon errors logged.
> 
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

Re: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Mehmet Fide]

Hi Ping-Ke, Bitterblue,

thanks for the ack on this one. It has not been applied yet, and before
I post anything on top of it I would like your opinion, because I now
have a case where the same lockup comes back with this patch in place.

What happens
============

The patch made the after-DTIM routing conditional, but nothing bounds
that path. mac80211 sets IEEE80211_TX_CTL_SEND_AFTER_DTIM on every bmc
frame while a single station is dozing, so one client in power save is
enough to put all broadcast and multicast traffic back on the high
queue. The chip drains that queue at beacon pace while the frames
occupy the shared TX page pool, so ordinary chatter such as mDNS
exhausts the pool and with it every other transmit, including the auth
and assoc responses other clients need. The AP still beacons and looks
alive, but nothing can join until the device is rebooted. That is what
our field report looked like.

Measured on an AM62 based AP with an RTL8822BU, one associated client
in power save, and 40 broadcast frames per second generated on the AP
itself. Free page count at 0x240:

  t=0     1803
  t=30s   1148
  t=60s    716
  t=90s     16   <- 0.9 % left
  ...       16   pinned for as long as the traffic runs

and at that point dmesg starts printing "error beacon valid" and
"failed to download drv rsvd page". When the client wakes or the
traffic stops the pool recovers; in the field both conditions persist,
so the AP stays dead.

How to reproduce
================

  - any rtw88 USB device as an AP, one client associated
  - put the client into power save (a phone with the screen off will
    do; on Windows set "Wireless Adapter Settings -> Power Saving Mode"
    to maximum and reconnect)
  - from the AP itself send roughly 40 multicast or broadcast packets
    per second, for example in a loop:
      socat -u - UDP4-DATAGRAM:224.0.0.251:5353,ttl=1
  - watch the free page count:
      echo "0x240 4" > /sys/kernel/debug/ieee80211/phyX/rtw88/read_reg
      cat /sys/kernel/debug/ieee80211/phyX/rtw88/read_reg

Do not ping the client while watching. That wakes it, mac80211 flushes
the queue and the pool recovers, which is what hid the problem from me
in the first run.

What I would like to do
=======================

Mirror the vendor driver. It never lets chatter into the hardware high
queue: xmitframe_hiq_filter() in core/rtw_xmit.c admits only ARP, EAPOL
and DHCP (CONFIG_RTW_HIQ_FILTER, default "allow special"), everything
else leaves on its access category queue. With the same rule in rtw88
the pool stays at 1803 in the test above, a DHCP flood still takes the
after-DTIM path, so the filter is selective rather than a blanket
disable, and plain join/ping cycling without power save is unchanged
(10/10 here).

The trade-off is that a dozing station can miss non-special multicast,
since we do not buffer it in software the way the vendor does.

Before I post a patch:

  1. Is such a filter acceptable as the fix, or would you rather see a
     hard bound on how many bmc frames may sit on the high queue, which
     would also cover an ARP or DHCP broadcast storm?

  2. Or both, filter first and bound on top?

  3. Is there a way to learn the DTIM boundary on USB that I have
     missed? With one the driver could hand the chip a single burst per
     DTIM and the pool could not run dry at all.

The patch is ready in either shape and I can post it as soon as you
tell me which one you prefer.

Thanks,
Mehmet

RE: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@gmail.com> wrote:
> Hi Ping-Ke, Bitterblue,
> 
> thanks for the ack on this one. It has not been applied yet, and before
> I post anything on top of it I would like your opinion, because I now
> have a case where the same lockup comes back with this patch in place.
> 
> What happens
> ============
> 
> The patch made the after-DTIM routing conditional, but nothing bounds
> that path. mac80211 sets IEEE80211_TX_CTL_SEND_AFTER_DTIM on every bmc
> frame while a single station is dozing, so one client in power save is
> enough to put all broadcast and multicast traffic back on the high
> queue. The chip drains that queue at beacon pace while the frames
> occupy the shared TX page pool, so ordinary chatter such as mDNS
> exhausts the pool and with it every other transmit, including the auth
> and assoc responses other clients need. The AP still beacons and looks
> alive, but nothing can join until the device is rebooted. That is what
> our field report looked like.
> 
> Measured on an AM62 based AP with an RTL8822BU, one associated client
> in power save, and 40 broadcast frames per second generated on the AP
> itself. Free page count at 0x240:
> 
>   t=0     1803
>   t=30s   1148
>   t=60s    716
>   t=90s     16   <- 0.9 % left
>   ...       16   pinned for as long as the traffic runs

Have you confirmed the broadcast frames ate all of them?

The HIQ packets only send out right after beacon within ATIM
window controlled by REG_ATIMWND (0x055A). Can you try to
enlarge the size to see if it will be different?

> 
> and at that point dmesg starts printing "error beacon valid" and
> "failed to download drv rsvd page". When the client wakes or the
> traffic stops the pool recovers; in the field both conditions persist,
> so the AP stays dead.

If you stop 40 broadcast frames per second, will AP become available?

> 
> How to reproduce
> ================
> 
>   - any rtw88 USB device as an AP, one client associated
>   - put the client into power save (a phone with the screen off will
>     do; on Windows set "Wireless Adapter Settings -> Power Saving Mode"
>     to maximum and reconnect)
>   - from the AP itself send roughly 40 multicast or broadcast packets
>     per second, for example in a loop:
>       socat -u - UDP4-DATAGRAM:224.0.0.251:5353,ttl=1
>   - watch the free page count:
>       echo "0x240 4" > /sys/kernel/debug/ieee80211/phyX/rtw88/read_reg
>       cat /sys/kernel/debug/ieee80211/phyX/rtw88/read_reg
> 
> Do not ping the client while watching. That wakes it, mac80211 flushes
> the queue and the pool recovers, which is what hid the problem from me
> in the first run.
> 
> What I would like to do
> =======================
> 
> Mirror the vendor driver. It never lets chatter into the hardware high
> queue: xmitframe_hiq_filter() in core/rtw_xmit.c admits only ARP, EAPOL
> and DHCP (CONFIG_RTW_HIQ_FILTER, default "allow special"), everything
> else leaves on its access category queue. With the same rule in rtw88
> the pool stays at 1803 in the test above, a DHCP flood still takes the
> after-DTIM path, so the filter is selective rather than a blanket
> disable, and plain join/ping cycling without power save is unchanged
> (10/10 here).
> 
> The trade-off is that a dozing station can miss non-special multicast,
> since we do not buffer it in software the way the vendor does.

Is it possible to declare IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING and
call ieee80211_get_buffered_bc() to get the BC packets to send?

> 
> Before I post a patch:
> 
>   1. Is such a filter acceptable as the fix, or would you rather see a
>      hard bound on how many bmc frames may sit on the high queue, which
>      would also cover an ARP or DHCP broadcast storm?

I think it depends on which kind of packets get lost we can accept. 

The vendor driver considers ARP, EAPOL and DHCP as important ones, because
user experience becomes bad if they get lost, but actually they have
their own re-transmit.

Consider multicast streaming, I guess it has error correction (like FEC),
so maybe it has more tolerant to handle packet loss. (I'm not familiar with
streaming, and no much experience with playing multicast streaming).

> 
>   2. Or both, filter first and bound on top?

With the filter, I think it will not reason bound, because there will not
be much that kind of packets. Maybe, check bound first, and then filter ?

> 
>   3. Is there a way to learn the DTIM boundary on USB that I have
>      missed? With one the driver could hand the chip a single burst per
>      DTIM and the pool could not run dry at all.

I think this is not possible, because USB needs time to transmit packets 
from host to WiFi card, and then it needs to wait for next DTIM. Though
driver can prepare packets earlier, but it will be inaccurate 
(still buffered in hardware HIQ).


As above, we have ATIM window, BC buffer in mac80211, HIQ filter,
and HIQ bound. But I don't have clear picture how to combine them to
deal with this case. Since you have spent time to dig the problem,
you may have more sense and better idea. :)

Re: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

Hi Ping-Ke,

thanks for the detailed answers, and for the ATIM window pointer - I ran
that experiment today. All numbers below are from the same bench setup
as before (RTL8822BU USB2 AP, dtim_period=2, one associated Windows
client in power save, ~40 broadcast/multicast frames per second
generated on the AP, free page count read at 0x240, nothing touching
the client during the runs).

> Have you confirmed the broadcast frames ate all of them?

Yes, three ways. The drain tracks the storm linearly (1803 -> 16 in
~105 s) and only while it runs. With an instrumented build I counted
the high queue draining ~3 frames per DTIM, which at dtim_period=2 is
~15 frames/s against ~40/s coming in. And with a test build that keeps
the same storm off the high queue (routed to the AC queues instead),
the count never leaves 1803 - same traffic, only the queue changed.

> The HIQ packets only send out right after beacon within ATIM
> window controlled by REG_ATIMWND (0x055A). Can you try to
> enlarge the size to see if it will be different?

It makes a dramatic difference. Same storm, only REG_ATIMWND changed:

  0x02 (default)  pool 1803 -> 16 in ~105 s, pinned  (reproduced twice)
  0x04            pool never leaves 1803 over 180 s
  0x08            same, never drops
  0x10            same, never drops  (reproduced twice)

So the default 2 TU window is what limits the drain to ~3 frames per
DTIM, and already 4 TU drains faster than this storm fills.

One more observation worth recording: with the client *actively
pinging* the AP the pool still drains to 16 at the default window.
Windows dynamic power save dozes between packets, so a client that
looks perfectly alive keeps the dozing condition asserted. That matches
our field failure: the link looked healthy and yet the AP died.

> If you stop 40 broadcast frames per second, will AP become available?

Yes. In both runs the pool was back to 1803 about 60 s after the storm
stopped, and the reconnects between the runs above all succeeded. In
the field neither condition stops (the chatter is mDNS/SSDP from the
clients' own segment), which is why it presents as a permanent lockup.

> I think this is not possible, because USB needs time to transmit
> packets from host to WiFi card, and then it needs to wait for next
> DTIM.

Understood, I have dropped that idea.

> Is it possible to declare IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING and
> call ieee80211_get_buffered_bc() to get the BC packets to send?

I looked at how the existing users time the release. ath9k_htc can do
it on USB only because its firmware sends an SWBA event at beacon time
(WMI_SWBA_EVENTID) and the driver pulls the buffered frames from that
handler; rt2500usb has no such event and explicitly refuses to set the
flag for that reason (see the comment in rt2500usb_probe_hw_mode).

Interestingly, the firmware seems to already have the needed event: the
vendor driver enables a beacon-early C2H report through a bit in the
SET_PWR_MODE H2C (SET_H2CCMD_PWRMODE_PARM_BCN_EARLY_C2H_RPT, C2H id
0x1E), though it only uses it for TDLS channel switching, i.e. in a
station power-save context. Do you know whether that report also works
in AP mode on the USB chips, and fires early enough to pace
ieee80211_get_buffered_bc()? If it does, this becomes the clean
long-term solution and I would be happy to prototype it.

For what it is worth, the vendor driver does not use any beacon event
for bmc delivery on USB either: it parks at most one filtered burst in
the high queue, lets the hardware pace it out after the DTIM beacon,
and refills only when the queue reads back empty - in other words, its
real protection is a hard bound on high queue occupancy, which is what
patch 1/2 below brings to rtw88.

> Maybe, check bound first, and then filter ?

Agreed. Here is how I would combine the four knobs:

  - bound (patch 1/2): hard cap on how many bmc frames may sit on the
    high queue (token bucket refilled at what the default window
    drains, overflow goes out on the AC queues awake-style). This is
    the guarantee: the pool stays healthy under any storm, including
    the ARP/DHCP bursts the filter admits.
  - filter (patch 2/2): admit only ARP, EAPOL and DHCP to the high
    queue, matching the vendor driver's default. Ordinary chatter never
    reaches the beacon-paced path, so the bound rarely engages.
  - ATIM window: given the measurements, a moderate raise (0x04
    already drains this storm, 0x10 gives headroom) would add drain
    capacity as a complement. I left it out of the series for now
    because the queue stays unbounded either way and a wider window
    costs every PS station awake time after each DTIM - I assume that
    is why the vendor driver keeps it small and filters instead. If
    the firmware is fine with a larger window on these chips I can add
    it as a third patch; is 0x055A safe to raise across the USB chips?
  - mac80211 BC buffering: the long-term correct PS delivery, gated on
    a beacon-time event as above; follow-up work, not part of this
    series.

If the plan looks right I will send the two patches (based on rtw-next
with the acked v3 applied) in the next days.

Best regards,
Mehmet

Re: [PATCH rtw-next v3] wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@gmail.com> wrote:

> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> 
> In AP mode every broadcast and multicast data frame is routed to
> TX_DESC_QSEL_HIGH, the after-DTIM queue, whether or not anybody is
> asleep. The firmware drains that queue at beacon pace, a dozen or so
> frames per second measured on RTL8822BU, while one associated client's
> mDNS/SSDP chatter alone exceeds that. The excess accumulates inside
> the chip until the shared TX page pool is exhausted (measured: 14 of
> 1803 pages left). From that point every host-sourced frame queues
> behind the backlog: authentication responses reach the air seconds
> after the client has given up, so no station can associate anymore,
> and the beacon reserved-page download fails the BCN_VALID poll
> ("error beacon valid") because it needs pages from the same pool. The
> AP keeps beaconing, so the failure looks like a silent RX stall and
> only a reboot recovers.
> 
> mac80211 already decides when after-DTIM delivery is needed: it sets
> IEEE80211_TX_CTL_SEND_AFTER_DTIM on bmc frames only while at least one
> station is actually dozing. Honor that instead of routing
> unconditionally: flagged frames keep going through the high queue with
> the MORE_DATA and HGQMD handling introduced by commit 076f786a0ae1
> ("wifi: rtw88: Fix AP mode incorrect DTIM behavior"), everything else
> leaves at line rate through the AC queues. This partially reverts the
> usb.c hunk of that commit, whose unconditional routing is what lets
> the backlog build up.
> 
> On a bench AP (USB2, 20 MHz, WPA2, hostapd, a Windows client driven
> through disconnect/reconnect cycles): reconnects fail 0/5 on RTL8822BU
> and 0/3 on RTL8821CU before this change, and pass 10/10 and 5/5 with
> it, with the page pool staying healthy and no beacon errors logged.
> 
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
> Acked-by: Ping-Ke Shih <pkshih@realtek.com>

1 patch(es) applied to rtw-next branch of rtw.git, thanks.

c710c7c2e038 wifi: rtw88: usb: route bmc frames via the high queue only for DTIM delivery

---
https://github.com/pkshih/rtw.git