Re: [PATCH net-next v15 2/7] r8169: refactor RX path to prepare for multi-queue

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message calls this a 'preparatory refactoring', but it
  does not mention that the source of the RX buffer DMA address changes.
- [Low] The diff contains changes with no functional purpose.

--

r8169: refactor RX path to prepare for multi-queue

This patch adds struct rtl8169_rx_ring. It moves the RX descriptor ring,
the data buffers and the cur_rx index out of rtl8169_private into a
per-queue array that is allocated at probe time. The netdev is now
allocated with devm_alloc_etherdev_mqs() for up to 8 RX queues, and
num_rx_rings stays at 1 for now.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 006c17ee778d1..f24a1e1f435c0 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -74,9 +74,13 @@
>  #define NUM_TX_DESC	256	/* Number of Tx descriptor registers */
>  #define NUM_RX_DESC	256	/* Number of Rx descriptor registers */
>  #define R8169_TX_RING_BYTES	(NUM_TX_DESC * sizeof(struct TxDesc))
> +
>  #define R8169_RX_RING_BYTES	(NUM_RX_DESC * sizeof(struct RxDesc))

[Severity: Low]
This isn't a bug, but this blank line looks like it was left over from
the R8169_RX_RING_BYTES padding change that the v14 changelog says was
reverted. It is still there at the end of the series.

The patch has a few other changes that do nothing. rtl8169_rx_csum() is
re-wrapped with no other change. The later "r8169: add support and
enable rss" patch replaces it anyway:

    static inline void rtl8169_rx_csum(struct sk_buff *skb,
                                       u32 opts1)

The new unwind label in rtl_init_one() is misspelled:

    err_destory_phylink:
        phylink_destroy(tp->phylink);

The misspelled label is also still there at the end of the series.
Should it be err_destroy_phylink?

[ ... ]

> @@ -750,6 +755,14 @@ enum rtl_dash_type {
>  	RTL_DASH_25_BP,
>  };
>  
> +struct rtl8169_rx_ring {
> +	u32 cur_rx;
> +	struct RxDesc *rx_desc_array;
> +	dma_addr_t rx_desc_phy_addr[NUM_RX_DESC];
> +	dma_addr_t rx_phy_addr;

[Severity: Low]
This isn't a bug, but the commit message calls this a preparatory
refactoring. It doesn't say that rtl_rx() and rtl8169_rx_clear() now get
the RX buffer DMA address from a copy kept by the driver, not from the
hardware descriptor.

Before this patch, the address was read back from the descriptor:

    addr = le64_to_cpu(desc->addr);

Now both functions use the new array, which rtl8169_alloc_rx_data()
fills in:

    addr = ring->rx_desc_phy_addr[entry];

This adds 256 dma_addr_t entries per ring, or 2 KB with a 64-bit
dma_addr_t.

The reason for the copy only shows up in the v8 changelog below the ---
line, and that text is dropped when the patch is applied. The reason is
that in the later "r8169: add support and enable rss" patch, the RSS
descriptor format reuses the desc->addr space for rss_info and
rss_result.

Could the commit message mention this change and why it is needed?

The names are also a bit confusing. rx_desc_phy_addr[] holds the DMA
addresses of the RX data pages, not of descriptors. It also sits right
next to rx_phy_addr, which is the base address of the descriptor ring.
The end of the series keeps the same names.

Would a name that refers to the data buffer be clearer here?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928070423.202-1-javen_xu%40realsil.com.cn