RE: [PATCH rtw-next] wifi: rtw88: sdio: limit the high queue to DTIM delivery

[Ping-Ke Shih <pkshih@realtek.com>]

Luka Gejak <luka.gejak@linux.dev> wrote:
> In AP mode every broadcast and multicast (bmc) frame is sent through the
> high queue, which the hardware only drains after a DTIM beacon. Only a
> few frames leave per DTIM, so steady multicast traffic builds a backlog
> that holds the TX pages the other queues need. Unicast traffic then
> stalls for seconds and clients can no longer renew their DHCP lease.
> 
> USB had the same problem and was fixed by commit c710c7c2e038 ("wifi:
> rtw88: usb: route bmc frames via the high queue only for DTIM delivery")
> and the two commits that followed it. Use the same rule for SDIO. A bmc
> frame goes to the high queue only when mac80211 asks for DTIM delivery
> because a station is dozing, the frame is ARP, EAPOL or DHCP, and the
> budget allows it. All other bmc frames go out on their access category
> queue.

I think it'd be good to add:

Tested on rtl8723bs with atk19 client.

> 
> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
> ---
>   Tested on rtl8723bs with atk19 client. 

(mention this in commit message)

> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index dc2fd0f8f9ff..3cdffac5b5b0 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -6,9 +6,11 @@
>   *   Copyright(c) 2018-2019  Realtek Corporation
>   */
> 
> +#include <linux/ip.h>

When moving rtw_sdio_bmc_needs_dtim() to tx.h, no need to include this, 
nor does usb.c.

>  #include <linux/module.h>
>  #include <linux/mmc/host.h>
>  #include <linux/mmc/sdio_func.h>
> +#include <linux/udp.h>
>  #include "main.h"
>  #include "mac.h"
>  #include "debug.h"
> @@ -26,6 +28,8 @@
>  #define RTW_SDIO_TX_FIFO_HIWATER                       16
>  #define RTW_SDIO_TX_FIFO_LOWATER                       8
>  #define RTW_SDIO_TX_RETRY_DELAY                        msecs_to_jiffies(1)
> +#define RTW_SDIO_HIQ_REFILL_INTERVAL                   (HZ / 10)
> +#define RTW_SDIO_HIQ_BUDGET_MAX                                16
> 
>  static bool rtw_sdio_is_bus_addr(u32 addr)
>  {
> @@ -1299,6 +1303,64 @@ static void rtw_sdio_8723bs_wake_tx_queue(struct rtw_dev *rtwdev,
>         ieee80211_wake_queue(rtwdev->hw, q_map);
>  }
> 
> +static bool rtw_sdio_bmc_needs_dtim(struct sk_buff *skb)
> +{
> +       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +       struct udphdr *udphdr;
> +
> +       if (info->control.flags & IEEE80211_TX_CTRL_PORT_CTRL_PROTO)
> +               return true;
> +
> +       if (skb->protocol == htons(ETH_P_ARP))
> +               return true;
> +
> +       if (skb->protocol == htons(ETH_P_IP) &&
> +           ip_hdr(skb)->protocol == IPPROTO_UDP) {
> +               udphdr = udp_hdr(skb);
> +
> +               return udphdr->dest == htons(67) || udphdr->dest == htons(68);
> +       }
> +
> +       return false;
> +}

This is totally the same as USB. To share with USB, move it to tx.c 
as an export function or to tx.h as an inline function.

> +
> +static bool rtw_sdio_hiq_take_budget(struct rtw_sdio *rtwsdio)
> +{
> +       unsigned long flags, elapsed, add;
> +       bool ok;
> +
> +       spin_lock_irqsave(&rtwsdio->hiq_lock, flags);
> +
> +       elapsed = jiffies - rtwsdio->hiq_refill;
> +       add = elapsed / RTW_SDIO_HIQ_REFILL_INTERVAL;
> +       if (add) {
> +               rtwsdio->hiq_budget = min_t(unsigned long,
> +                                           rtwsdio->hiq_budget + add,
> +                                           RTW_SDIO_HIQ_BUDGET_MAX);
> +
> +               /* keep the unfinished part of the interval for the next unit */
> +               rtwsdio->hiq_refill = jiffies -
> +                                     elapsed % RTW_SDIO_HIQ_REFILL_INTERVAL;
> +       }
> +
> +       ok = rtwsdio->hiq_budget > 0;
> +       if (ok)
> +               rtwsdio->hiq_budget--;
> +
> +       spin_unlock_irqrestore(&rtwsdio->hiq_lock, flags);
> +
> +       return ok;
> +}

This is also the same as USB. Please consider how to share the logic.