RE: [PATCH rtw-next 1/2] wifi: rtw88: usb: bound what the driver feeds the after-DTIM queue

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

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

[...]

> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -562,7 +562,38 @@ static int rtw_usb_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
>         return rtw_usb_write_data(rtwdev, &pkt_info, buf);
>  }
> 
> -static u8 rtw_usb_tx_queue_mapping_to_qsel(struct sk_buff *skb)
> +#define RTW_USB_HIQ_RATE       10
> +#define RTW_USB_HIQ_BURST      16
> +
> +static bool rtw_usb_hiq_take(struct rtw_usb *rtwusb)
> +{
> +       unsigned long flags, delta;
> +       bool ok;
> +       u32 add;
> +
> +       spin_lock_irqsave(&rtwusb->hiq_lock, flags);

nit: a blank line

> +       delta = jiffies - rtwusb->hiq_refill;
> +       if (delta >= HZ / RTW_USB_HIQ_RATE) {
> +               add = delta / (HZ / RTW_USB_HIQ_RATE);

The pattern ' HZ / RTW_USB_HIQ_RATE' repeat many times. Can you just define
the it as a part of macro? (The macro name should change as well.)

> +               if (add >= RTW_USB_HIQ_BURST) {
> +                       rtwusb->hiq_tokens = RTW_USB_HIQ_BURST;
> +                       rtwusb->hiq_refill = jiffies;
> +               } else {
> +                       rtwusb->hiq_tokens = min_t(u32, rtwusb->hiq_tokens + add,
> +                                                  RTW_USB_HIQ_BURST);

Will 'quota' or 'budget' be clearer than 'token'?

> +                       rtwusb->hiq_refill += add * (HZ / RTW_USB_HIQ_RATE);

It looks like you shift refill jiffies according to the tokens you are adding.
How can I understand '+= add * (HZ / RTW_USB_HIQ_RATE)'?

> +               }
> +       }
> +       ok = rtwusb->hiq_tokens > 0;
> +       if (ok)
> +               rtwusb->hiq_tokens--;

nit: a blank line

> +       spin_unlock_irqrestore(&rtwusb->hiq_lock, flags);
> +
> +       return ok;
> +}
> +
> +static u8 rtw_usb_tx_queue_mapping_to_qsel(struct rtw_usb *rtwusb,
> +                                          struct sk_buff *skb)
>  {
>         struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);