RE: [PATCH rtw-next v3 2/3] wifi: rtl8xxxu: unwind incomplete receive startup

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

Kim Wooseok via B4 Relay <devnull+5mghybrid.khu.ac.kr@kernel.org> wrote:
> From: Kim Wooseok <5mghybrid@khu.ac.kr>
> 
> rtl8xxxu_start() allocates and submits RX URBs one at a time. If a later
> allocation fails, the error path frees the TX pool but leaves earlier RX
> requests active. A partial TX or RX allocation failure can also leave
> ret at zero, so start reports success despite the incomplete setup.
> 
> Allocate the RX pool before submitting any of it, and return ENOMEM
> whenever a pool allocation fails. Pass start errors through
> rtl8xxxu_stop(), including interrupt URB submission failures, so the
> existing stop path cancels work and releases both active requests and
> partially allocated pools.
> 
> Share the submission loop with the RX worker. Both callers keep requests
> that fail with ENOMEM/EAGAIN. For a fatal error during startup, free the
> rest of the unsubmitted batch and return the error so start can unwind.
> In the worker, free only the failed request and continue with the rest
> of the batch.
> 
> Keep the allocation loop in a small helper so the setup and error paths
> in start remain easy to follow.
> 
> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)")
> Assisted-by: GPT-6 Astra
> Signed-off-by: Kim Wooseok <5mghybrid@khu.ac.kr>

Please fix some minor comments, and take my reviewed-by to next version.

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

> ---
>  drivers/net/wireless/realtek/rtl8xxxu/core.c | 96
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------
>  1 file changed, 54 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> index 795a5ec2f8cd4..5ae3dbf5034ac 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> @@ -58,6 +58,7 @@ MODULE_PARM_DESC(dma_agg_pages, "Set DMA aggregation pages (range 1-127, 0 to di
>  #define RTL8XXXU_TX_URB_LOW_WATER      25
>  #define RTL8XXXU_TX_URB_HIGH_WATER     32
> 
> +static void rtl8xxxu_stop(struct ieee80211_hw *hw, bool suspend);
>  static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
>                                   struct rtl8xxxu_rx_urb *rx_urb);
> 
> @@ -5832,6 +5833,27 @@ static void rtl8xxxu_free_rx_resources(struct rtl8xxxu_priv *priv)
>         spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
>  }
> 
> +static int rtl8xxxu_alloc_rx_urbs(struct rtl8xxxu_priv *priv)
> +{
> +       struct rtl8xxxu_rx_urb *rx_urb;
> +       int i;
> +
> +       /* No RX work is active until the complete pool has been allocated. */
> +       for (i = 0; i < RTL8XXXU_RX_URBS; i++) {
> +               rx_urb = kmalloc_obj(struct rtl8xxxu_rx_urb);
> +               if (!rx_urb)
> +                       return -ENOMEM;
> +
> +               usb_init_urb(&rx_urb->urb);
> +               INIT_LIST_HEAD(&rx_urb->list);
> +               rx_urb->hw = priv->hw;

nit: add a blank line

> +               list_add_tail(&rx_urb->list, &priv->rx_urb_pending_list);
> +               priv->rx_urb_pending_count++;
> +       }
> +
> +       return 0;
> +}
> +
>  static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv,
>                                   struct rtl8xxxu_rx_urb *rx_urb)
>  {

[...]

> +
> +static void rtl8xxxu_rx_urb_work(struct work_struct *work)
> +{
> +       struct rtl8xxxu_priv *priv;
> +
> +       priv = container_of(work, struct rtl8xxxu_priv, rx_urb_wq);

Just assign this at declaration. 

> +       rtl8xxxu_submit_rx_urbs(priv, false);
>  }
> 
>  /*
> @@ -7408,7 +7438,6 @@ static void rtl8xxxu_watchdog_callback(struct work_struct *work)
>  static int rtl8xxxu_start(struct ieee80211_hw *hw)
>  {
>         struct rtl8xxxu_priv *priv = hw->priv;
> -       struct rtl8xxxu_rx_urb *rx_urb;
>         struct rtl8xxxu_tx_urb *tx_urb;
>         unsigned long flags;
>         int ret, i;
> @@ -7423,14 +7452,13 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>         if (priv->usb_interrupts) {
>                 ret = rtl8xxxu_submit_int_urb(hw);
>                 if (ret)
> -                       goto exit;
> +                       goto error_out;

I have a request in v2 that please make sure all error path can
properly handled by rtl8xxxu_stop(). Can you confirm and ack me?
Or I missed?

>         }
> 
>         for (i = 0; i < RTL8XXXU_TX_URBS; i++) {
>                 tx_urb = kmalloc_obj(struct rtl8xxxu_tx_urb);
>                 if (!tx_urb) {
> -                       if (!i)
> -                               ret = -ENOMEM;
> +                       ret = -ENOMEM;
> 
>                         goto error_out;
>                 }