RE: [PATCH rtw-next v2 3/4] wifi: rtl8xxxu: preserve RX requests across recoverable transfer errors

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

kimwooseok via B4 Relay <devnull+5mghybrid.khu.ac.kr@kernel.org> wrote:
> From: kimwooseok <5mghybrid@khu.ac.kr>
> 
> rtl8xxxu normally reuses 32 RX URBs, scheduling the submission worker when
> more than eight completed requests have accumulated on the pending list.
> Completion errors free URBs instead. A finite error burst can therefore
> leave eight or fewer requests, which cannot reach that threshold after
> they all complete. With no request in flight and no worker pending or
> running, RX stays stopped even after the errors cease.
> 
> To prevent these errors from shrinking the pool below the number
> needed for normal resubmission, retain URBs after EPROTO, EILSEQ, ETIME,
> EOVERFLOW, ECOMM and ENOSR completions. EHCI can report ENOSR for
> IN data-buffer
> errors, and FHCI maps RX buffer overrun to ECOMM. Free the failed
> transfer's skb and keep its URB on a separate retry list.
> 
> Keeping the URBs is only part of the fix: the driver must also submit
> them again without waiting for nine requests to accumulate. When the
> first failed request enters the retry list, schedule delayed work for
> 100 ms. Further failures join that list while the work is pending.
> When it runs, move the collected requests to normal pending and schedule
> the submission worker even if only one request is waiting. Keeping
> failed requests separate until then prevents normal completions from
> triggering an immediate retry; successful RX keeps its existing batching.
> 
> A retry can itself fail with ENOMEM/EAGAIN. Returning that request to
> normal pending would bring back the same threshold problem, so route
> temporary submission failures from both start and the RX worker through
> the delayed retry list as well.
> 
> Serialize retry insertion and scheduling with shutdown so late
> completions cannot schedule fresh retries during stop. Cancel retry work
> first, then wait for submission work before killing active URBs, so a
> running worker cannot submit a request after the active requests have
> been drained.
> 
> Cancellation and removal keep their release behavior. EPIPE endpoint-halt
> recovery remains outside this change because it requires quiescing
> requests and distinguishing recovery cancellation from shutdown.
> 
> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)")

Same question. Is this patch strong enough to need a Fixes?

> Assisted-by: GPT-6 Astra
> Signed-off-by: kimwooseok <5mghybrid@khu.ac.kr>
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/core.c     | 71 ++++++++++++++++++++++--
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h |  3 +
>  2 files changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> index 1932a9ec1970c..883c9a56f52a4 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c

[...]

> +static void rtl8xxxu_rx_urb_retry_work(struct work_struct *work)
> +{
> +       struct rtl8xxxu_priv *priv = container_of(to_delayed_work(work),
> +                                               struct rtl8xxxu_priv,
> +                                               rx_urb_retry_wq);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->rx_urb_lock, flags);
> +
> +       if (!priv->shutdown && priv->rx_urb_retry_count) {
> +               list_splice_tail_init(&priv->rx_urb_retry_list,
> +                                     &priv->rx_urb_pending_list);
> +               priv->rx_urb_pending_count += priv->rx_urb_retry_count;
> +               priv->rx_urb_retry_count = 0;
> +               /* An error must not leave a small batch waiting indefinitely. */
> +               schedule_work(&priv->rx_urb_wq);
> +       }
> +
> +       spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
> +}
> +
> +static void rtl8xxxu_queue_rx_urb_retry(struct rtl8xxxu_priv *priv,
> +                                       struct rtl8xxxu_rx_urb *rx_urb)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->rx_urb_lock, flags);
> +
> +       if (!priv->shutdown) {
> +               list_add_tail(&rx_urb->list, &priv->rx_urb_retry_list);
> +               priv->rx_urb_retry_count++;
> +               /* Keep normal completions from bypassing the error backoff. */
> +               queue_delayed_work(system_wq, &priv->rx_urb_retry_wq,
> +                                  msecs_to_jiffies(RTL8XXXU_RX_URB_RETRY_DELAY_MS));
> +       } else {
> +               usb_free_urb(&rx_urb->urb);
> +       }
> +
> +       spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
> +}
> +

These two are very similar to existing codes, and ...

>  static void rtl8xxxu_rx_urb_work(struct work_struct *work)
>  {
>         struct rtl8xxxu_priv *priv;

[...]

> @@ -6618,7 +6665,20 @@ static void rtl8xxxu_rx_complete(struct urb *urb)
>                 rtl8xxxu_queue_rx_urb(priv, rx_urb);
>         } else {
>                 dev_dbg(dev, "%s: status %i\n", __func__, urb->status);
> -               goto cleanup;
> +               switch (urb->status) {
> +               case -EPROTO:
> +               case -EILSEQ:
> +               case -ETIME:
> +               case -EOVERFLOW:
> +               case -ECOMM:
> +               case -ENOSR:
> +                       dev_kfree_skb(skb);
> +                       urb->context = NULL;
> +                       rtl8xxxu_queue_rx_urb_retry(priv, rx_urb);

Why can't it just call rtl8xxxu_queue_rx_urb()?

The key point is that you want an additional delay? If so, maybe we can
flag for these cases and reuse rtl8xxxu_queue_rx_urb().

> +                       return;
> +               default:
> +                       goto cleanup;
> +               }
>         }
>         return;
>