RE: [PATCH rtw-next v2 4/4] wifi: rtl8xxxu: test RX ownership and recovery across failures

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

kimwooseok via B4 Relay <devnull+5mghybrid.khu.ac.kr@kernel.org> wrote:
> From: kimwooseok <5mghybrid@khu.ac.kr>
> 
> Add 11 KUnit cases for the RX allocation, submission, completion and
> retry paths. Cover each retryable completion followed by ENOMEM/EAGAIN,
> batch sizes 1, 8, 9 and 32, skb allocation failure, startup failure
> positions, cancellation and shutdown.
> 
> Run the actual RX helpers and worker with task-scoped stubs for
> allocation and USB submission. Observer references check that the driver
> releases its URB and skb references. A delayed-work case checks that
> one retry request schedules the submission worker.
> 
> Assisted-by: GPT-6 Astra
> Signed-off-by: kimwooseok <5mghybrid@khu.ac.kr>
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/.kunitconfig |  16 +
>  drivers/net/wireless/realtek/rtl8xxxu/Kconfig      |  11 +
>  drivers/net/wireless/realtek/rtl8xxxu/Makefile     |   3 +
>  drivers/net/wireless/realtek/rtl8xxxu/core.c       |  66 ++-
>  drivers/net/wireless/realtek/rtl8xxxu/rx-test.c    | 461 +++++++++++++++++++++
>  drivers/net/wireless/realtek/rtl8xxxu/rx-test.h    |  27 ++
>  6 files changed, 569 insertions(+), 15 deletions(-)
> 

[...]

> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/Makefile
> b/drivers/net/wireless/realtek/rtl8xxxu/Makefile
> index 580a2fa675ee2..a592a81197857 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/Makefile
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/Makefile
> @@ -4,3 +4,6 @@ obj-$(CONFIG_RTL8XXXU)  += rtl8xxxu.o
>  rtl8xxxu-y     := core.o 8192e.o 8723b.o \
>                    8723a.o 8192c.o 8188f.o \
>                    8188e.o 8710b.o 8192f.o
> +
> +obj-$(CONFIG_RTL8XXXU_KUNIT_TEST) += rtl8xxxu-rx-test.o
> +rtl8xxxu-rx-test-y := rx-test.o
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> index 883c9a56f52a4..323411e7f5e3b 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> @@ -17,6 +17,8 @@
>  #include <linux/iopoll.h>

move '#include <kunit/static_stub.h>' here.

>  #include "regs.h"
>  #include "rtl8xxxu.h"
> +#include "rx-test.h"
> +#include <kunit/static_stub.h>
> 
>  #define DRIVER_NAME "rtl8xxxu"
> 
> @@ -60,8 +62,7 @@ MODULE_PARM_DESC(dma_agg_pages, "Set DMA aggregation pages (range 1-127, 0 to di
>  #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);
> +
> 
>  static struct ieee80211_rate rtl8xxxu_rates[] = {
>         { .bitrate = 10, .hw_value = DESC_RATE_1M, .flags = 0 },
> @@ -5817,7 +5818,36 @@ void jaguar2_rx_parse_phystats(struct rtl8xxxu_priv *priv,
>         }
>  }
> 
> -static void rtl8xxxu_free_rx_resources(struct rtl8xxxu_priv *priv)
> +VISIBLE_IF_KUNIT struct rtl8xxxu_rx_urb *rtl8xxxu_alloc_rx_urb(void)
> +{
> +       KUNIT_STATIC_STUB_REDIRECT(rtl8xxxu_alloc_rx_urb);

an empty line, and also apply to following funtions.

> +       return kmalloc_obj(struct rtl8xxxu_rx_urb);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_alloc_rx_urb);
> +
> +VISIBLE_IF_KUNIT struct sk_buff *rtl8xxxu_alloc_rx_skb(unsigned int size)
> +{
> +       KUNIT_STATIC_STUB_REDIRECT(rtl8xxxu_alloc_rx_skb, size);
> +       return __netdev_alloc_skb(NULL, size, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_alloc_rx_skb);
> +
> +VISIBLE_IF_KUNIT int rtl8xxxu_rx_usb_submit(struct urb *urb, gfp_t flags)
> +{
> +       KUNIT_STATIC_STUB_REDIRECT(rtl8xxxu_rx_usb_submit, urb, flags);
> +       return usb_submit_urb(urb, flags);
> +}
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_rx_usb_submit);
> +
> +VISIBLE_IF_KUNIT void rtl8xxxu_schedule_rx_retry(struct rtl8xxxu_priv *priv)
> +{
> +       KUNIT_STATIC_STUB_REDIRECT(rtl8xxxu_schedule_rx_retry, priv);
> +       queue_delayed_work(system_wq, &priv->rx_urb_retry_wq,
> +                          msecs_to_jiffies(RTL8XXXU_RX_URB_RETRY_DELAY_MS));
> +}
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_schedule_rx_retry);
> +
> +VISIBLE_IF_KUNIT void rtl8xxxu_free_rx_resources(struct rtl8xxxu_priv *priv)
>  {
>         struct rtl8xxxu_rx_urb *rx_urb, *tmp;
>         unsigned long flags;
> @@ -5838,15 +5868,16 @@ static void rtl8xxxu_free_rx_resources(struct rtl8xxxu_priv *priv)
> 
>         spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_free_rx_resources);
> 
> -static int rtl8xxxu_alloc_rx_urbs(struct rtl8xxxu_priv *priv)
> +VISIBLE_IF_KUNIT 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);
> +               rx_urb = rtl8xxxu_alloc_rx_urb();
>                 if (!rx_urb)
>                         return -ENOMEM;
> 
> @@ -5859,6 +5890,7 @@ static int rtl8xxxu_alloc_rx_urbs(struct rtl8xxxu_priv *priv)
> 
>         return 0;
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_alloc_rx_urbs);
> 
>  static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv,
>                                   struct rtl8xxxu_rx_urb *rx_urb)
> @@ -5887,7 +5919,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv,
>         spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
>  }
> 
> -static void rtl8xxxu_rx_urb_retry_work(struct work_struct *work)
> +VISIBLE_IF_KUNIT void rtl8xxxu_rx_urb_retry_work(struct work_struct *work)
>  {
>         struct rtl8xxxu_priv *priv = container_of(to_delayed_work(work),
>                                                 struct rtl8xxxu_priv,
> @@ -5907,6 +5939,7 @@ static void rtl8xxxu_rx_urb_retry_work(struct work_struct *work)
> 
>         spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_rx_urb_retry_work);
> 
>  static void rtl8xxxu_queue_rx_urb_retry(struct rtl8xxxu_priv *priv,
>                                         struct rtl8xxxu_rx_urb *rx_urb)
> @@ -5919,8 +5952,7 @@ static void rtl8xxxu_queue_rx_urb_retry(struct rtl8xxxu_priv *priv,
>                 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));
> +               rtl8xxxu_schedule_rx_retry(priv);
>         } else {
>                 usb_free_urb(&rx_urb->urb);
>         }
> @@ -5928,7 +5960,7 @@ static void rtl8xxxu_queue_rx_urb_retry(struct rtl8xxxu_priv *priv,
>         spin_unlock_irqrestore(&priv->rx_urb_lock, flags);
>  }
> 
> -static void rtl8xxxu_rx_urb_work(struct work_struct *work)
> +VISIBLE_IF_KUNIT void rtl8xxxu_rx_urb_work(struct work_struct *work)
>  {
>         struct rtl8xxxu_priv *priv;
>         struct rtl8xxxu_rx_urb *rx_urb, *tmp;
> @@ -5968,8 +6000,9 @@ static void rtl8xxxu_rx_urb_work(struct work_struct *work)
>                 }
>         }
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_rx_urb_work);
> 
> -static int rtl8xxxu_start_rx(struct rtl8xxxu_priv *priv)
> +VISIBLE_IF_KUNIT int rtl8xxxu_start_rx(struct rtl8xxxu_priv *priv)
>  {
>         struct rtl8xxxu_rx_urb *rx_urb, *tmp;
>         unsigned long flags;
> @@ -6006,6 +6039,7 @@ static int rtl8xxxu_start_rx(struct rtl8xxxu_priv *priv)
>         }
>         return ret;
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_start_rx);
> 
>  /*
>   * The RTL8723BU/RTL8192EU vendor driver use coexistence table type
> @@ -6646,7 +6680,7 @@ int rtl8xxxu_parse_rxdesc24(struct rtl8xxxu_priv *priv, struct sk_buff *skb)
>         return RX_TYPE_DATA_PKT;
>  }
> 
> -static void rtl8xxxu_rx_complete(struct urb *urb)
> +VISIBLE_IF_KUNIT void rtl8xxxu_rx_complete(struct urb *urb)
>  {
>         struct rtl8xxxu_rx_urb *rx_urb =
>                 container_of(urb, struct rtl8xxxu_rx_urb, urb);
> @@ -6686,9 +6720,10 @@ static void rtl8xxxu_rx_complete(struct urb *urb)
>         usb_free_urb(urb);
>         dev_kfree_skb(skb);
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_rx_complete);
> 
> -static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
> -                                 struct rtl8xxxu_rx_urb *rx_urb)
> +VISIBLE_IF_KUNIT int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
> +                                           struct rtl8xxxu_rx_urb *rx_urb)
>  {
>         struct rtl8xxxu_fileops *fops = priv->fops;
>         struct sk_buff *skb;
> @@ -6704,7 +6739,7 @@ static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
>                 skb_size = IEEE80211_MAX_FRAME_LEN + rx_desc_sz;
>         }
> 
> -       skb = __netdev_alloc_skb(NULL, skb_size, GFP_KERNEL);
> +       skb = rtl8xxxu_alloc_rx_skb(skb_size);
>         if (!skb)
>                 return -ENOMEM;
> 
> @@ -6712,7 +6747,7 @@ static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
>         usb_fill_bulk_urb(&rx_urb->urb, priv->udev, priv->pipe_in, skb->data,
>                           skb_size, rtl8xxxu_rx_complete, skb);
>         usb_anchor_urb(&rx_urb->urb, &priv->rx_anchor);
> -       ret = usb_submit_urb(&rx_urb->urb, GFP_ATOMIC);
> +       ret = rtl8xxxu_rx_usb_submit(&rx_urb->urb, GFP_ATOMIC);
>         if (ret) {
>                 usb_unanchor_urb(&rx_urb->urb);
>                 dev_kfree_skb(skb);
> @@ -6720,6 +6755,7 @@ static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
>         }
>         return ret;
>  }
> +EXPORT_SYMBOL_IF_KUNIT(rtl8xxxu_submit_rx_urb);
> 
>  static void rtl8xxxu_int_complete(struct urb *urb)
>  {
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rx-test.c

Prefer test-rx.c. 

Regarding sometime we add test cases for TX, the file name can
be test-tx.c

> b/drivers/net/wireless/realtek/rtl8xxxu/rx-test.c
> new file mode 100644
> index 0000000000000..aedddcb049cdf
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rx-test.c
> @@ -0,0 +1,461 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <kunit/static_stub.h>
> +#include <kunit/test.h>
> +#include <linux/usb.h>
> +
> +#include "rtl8xxxu.h"
> +#include "rx-test.h"
> +
> +#define RX_TEST_URBS 32
> +#define RX_TEST_SKBS 512
> +
> +struct rx_test {

I'm not familiar to kunittest, so I don't quit understand how it uses its
own context. I'll review this patch in detail in the future, and currently
I only point out some surface opinons.

> +       struct rtl8xxxu_priv priv;
> +       struct ieee80211_hw hw;
> +       struct rtl8xxxu_fileops fops;
> +       struct usb_device udev;
> +       struct rtl8xxxu_rx_urb *urbs[RX_TEST_URBS];
> +       struct sk_buff *skbs[RX_TEST_SKBS];
> +       unsigned int allocated;
> +       unsigned int buffers;
> +       unsigned int alloc_calls;
> +       unsigned int alloc_fail_at;
> +       unsigned int submit_calls;
> +       unsigned int submit_fail_at;
> +       int submit_error;
> +       bool fail_skb;
> +       bool retry_pending;
> +       unsigned int retry_arms;
> +       atomic_t normal_runs;
> +};
> +
> +static struct rx_test *rx_current(void)
> +{
> +       return kunit_get_current_test()->priv;
> +}
> +
> +static struct rtl8xxxu_rx_urb *rx_alloc_object(void)
> +{
> +       struct rx_test *ctx = rx_current();

maybe function name can point out 'ctx', like rx_current_test_ctx().

> +       struct rtl8xxxu_rx_urb *rx;
> +
> +       ctx->alloc_calls++;
> +       if (ctx->alloc_calls == ctx->alloc_fail_at)
> +               return NULL;

empty line 

> +       rx = kmalloc_obj(struct rtl8xxxu_rx_urb);
> +       if (rx)
> +               ctx->urbs[ctx->allocated++] = rx;

empty line. (please review this file yourself to add proper empty lines)

> +       return rx;
> +}
> +

[...]

> +static struct rx_test *rx_init(struct kunit *test, bool fake_timer)
> +{
> +       struct rx_test *ctx;
> +
> +       ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return NULL;
> +       test->priv = ctx;
> +       ctx->hw.priv = &ctx->priv;
> +       ctx->priv.hw = &ctx->hw;
> +       ctx->priv.udev = &ctx->udev;
> +       ctx->priv.fops = &ctx->fops;
> +       ctx->fops.rx_desc_size = sizeof(struct rtl8xxxu_rxdesc16);
> +       spin_lock_init(&ctx->priv.rx_urb_lock);
> +       INIT_LIST_HEAD(&ctx->priv.rx_urb_pending_list);
> +       INIT_LIST_HEAD(&ctx->priv.rx_urb_retry_list);
> +       init_usb_anchor(&ctx->priv.rx_anchor);
> +       INIT_WORK(&ctx->priv.rx_urb_wq, rx_observe_work);
> +       INIT_DELAYED_WORK(&ctx->priv.rx_urb_retry_wq,
> +                         rtl8xxxu_rx_urb_retry_work);
> +       atomic_set(&ctx->normal_runs, 0);
> +
> +       kunit_activate_static_stub(test, rtl8xxxu_alloc_rx_urb, rx_alloc_object);

Add prefix to sub, like:

rx_alloc_object  ->  fake_rtl8xxxu_alloc_rx_urb, or
                 ->  stub_rtl8xxxu_alloc_rx_urb

> +       kunit_activate_static_stub(test, rtl8xxxu_alloc_rx_skb, rx_alloc_buffer);
> +       kunit_activate_static_stub(test, rtl8xxxu_rx_usb_submit, rx_submit);
> +       if (fake_timer)
> +               kunit_activate_static_stub(test, rtl8xxxu_schedule_rx_retry,
> +                                          rx_schedule_retry);
> +       return ctx;
> +}
> +

[...]

> +
> +static void rx_start_fatal_failure(struct kunit *test)
> +{
> +       static const unsigned int positions[] = { 1, 8, 32 };

A personal random thought: if the values can be random, maybe create
another test item with random values for unpredicted corner cases.
(not only for this test item)

> +       struct rx_test *ctx;
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(positions); i++) {
> +               ctx = rx_init(test, true);
> +               KUNIT_ASSERT_NOT_NULL(test, ctx);
> +               KUNIT_ASSERT_EQ(test, rx_pool(ctx, 32), 0);
> +               ctx->submit_fail_at = positions[i];
> +               ctx->submit_error = -ENODEV;
> +               KUNIT_EXPECT_EQ(test, rtl8xxxu_start_rx(&ctx->priv), -ENODEV);
> +               KUNIT_EXPECT_EQ(test, ctx->submit_calls, positions[i]);
> +               KUNIT_EXPECT_EQ(test, ctx->priv.rx_urb_pending_count, 0);
> +               KUNIT_EXPECT_FALSE(test, ctx->retry_pending);
> +               rx_finish(test, ctx);
> +       }
> +}
> +