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

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@xxxxxxxxx> wrote:
> From: Mehmet Fide <mehmet.fide@xxxxxxxxxxxxxxxxxx>
> 
> 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.
> 
> Fixes: 076f786a0ae1 ("wifi: rtw88: Fix AP mode incorrect DTIM behavior")

As you said in v1, this is to fix different problem. Is it too strong to
point it as a Fixes?

> Signed-off-by: Mehmet Fide <mehmet.fide@xxxxxxxxxxxxxxxxxx>
> ---
> 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..c0990c125 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -559,6 +559,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_tx_info *info = IEEE80211_SKB_CB(skb);

In reverse X'mas order.

>         struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>         __le16 fc = hdr->frame_control;
>         u8 qsel;