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

[Ping-Ke Shih]

kimwooseok via B4 Relay <devnull+5mghybrid.khu.ac.kr@xxxxxxxxxx> wrote:
> From: kimwooseok <5mghybrid@xxxxxxxxx>
> 
> rtl8xxxu_start() allocates and submits RX URBs one at a time. If a later
> allocation fails, earlier requests may already be active. After a
> successful submission, that allocation failure can also leave ret set
> to zero. The error path then frees TX resources and disables RX filters
> without draining the earlier RX requests, yet reports startup success.
> 
> Separate pool allocation from submission so an allocation failure can be
> handled before any RX request is active. Introduce rtl8xxxu_alloc_rx_urbs()
> to allocate all 32 wrappers, then rtl8xxxu_start_rx() to submit the
> completed pool. Return ENOMEM for every RX or TX URB pool allocation
> failure so a partial allocation is reported as an error.
> 
> Once submission begins, keep ENOMEM/EAGAIN failures queued for retry.
> For other submission errors, rtl8xxxu_start_rx() frees the unsubmitted
> requests and returns the error. Since earlier submissions may already be
> active at that point, route the outer start failure through the existing
> rtl8xxxu_stop() path. This drains queued work and active requests and
> cleans up RF state and TX resources together. Interrupt URB submission
> failure uses the same cleanup path.
> 
> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)")

Is this patch strong to have a Fix tag?

> Assisted-by: GPT-6 Astra
> Signed-off-by: kimwooseok <5mghybrid@xxxxxxxxx>
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/core.c | 99 ++++++++++++++++++++--------
>  1 file changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> index 795a5ec2f8cd4..1932a9ec1970c 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c

[...]

> @@ -5900,6 +5922,44 @@ static void rtl8xxxu_rx_urb_work(struct work_struct *work)
>         }
>  }
> 
> +static int rtl8xxxu_start_rx(struct rtl8xxxu_priv *priv)

This is very similar to rtl8xxxu_rx_urb_work(). Please merge them.

> +{
> +       struct rtl8xxxu_rx_urb *rx_urb, *tmp;
> +       unsigned long flags;
> +       LIST_HEAD(local);
> +       int ret;
> +
> +       spin_lock_irqsave(&priv->rx_urb_lock, flags);
> +       list_splice_init(&priv->rx_urb_pending_list, &local);
> +       priv->rx_urb_pending_count = 0;
> +       spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
> +
> +       list_for_each_entry_safe(rx_urb, tmp, &local, list) {
> +               list_del_init(&rx_urb->list);
> +               ret = rtl8xxxu_submit_rx_urb(priv, rx_urb);
> +               switch (ret) {
> +               case 0:
> +                       break;
> +               case -ENOMEM:
> +               case -EAGAIN:
> +                       rtl8xxxu_queue_rx_urb(priv, rx_urb);
> +                       break;
> +               default:
> +                       usb_free_urb(&rx_urb->urb);
> +                       goto free_remaining;
> +               }
> +       }
> +
> +       return 0;
> +
> +free_remaining:
> +       list_for_each_entry_safe(rx_urb, tmp, &local, list) {
> +               list_del(&rx_urb->list);
> +               usb_free_urb(&rx_urb->urb);
> +       }
> +       return ret;
> +}
> +
>  /*
>   * The RTL8723BU/RTL8192EU vendor driver use coexistence table type
>   * 0-7 to represent writing different combinations of register values

[..]

> @@ -7478,13 +7526,7 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>         return ret;
> 
>  error_out:
> -       rtl8xxxu_free_tx_resources(priv);
> -       /*
> -        * Disable all data and mgmt frames
> -        */
> -       rtl8xxxu_write16(priv, REG_RXFLTMAP2, 0x0000);
> -       rtl8xxxu_write16(priv, REG_RXFLTMAP0, 0x0000);
> -
> +       rtl8xxxu_stop(hw, false);

By my review, it is fine to call rtl8xxxu_stop(), but in case I missed
something, please test every 'goto error_out' to ensure it doesn't
encounter use-before-initialization. 

>         return ret;
>  }
> 
> @@ -7820,6 +7862,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>         spin_lock_init(&priv->tx_urb_lock);
>         INIT_LIST_HEAD(&priv->rx_urb_pending_list);
>         spin_lock_init(&priv->rx_urb_lock);
> +       priv->shutdown = true;

I see only rtl8xxxu_queue_rx_urb() checks this flag. Any reason we need to
set it true at probe?

>         INIT_WORK(&priv->rx_urb_wq, rtl8xxxu_rx_urb_work);
>         INIT_DELAYED_WORK(&priv->ra_watchdog, rtl8xxxu_watchdog_callback);
>         INIT_DELAYED_WORK(&priv->update_beacon_work, rtl8xxxu_update_beacon_work_callback);
> 
> --
> 2.48.1
>