Re: [PATCH net-next v15 4/7] r8169: enable new interrupt mapping

[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 says the patch "enables new interrupt mapping
  for RTL8127", but at this commit the new code can never run.

--

r8169: enable new interrupt mapping

This patch adds a helper that sets INT_CFG0_ENABLE_8125 to turn on the
new interrupt vector mapping for RTL8127. It also moves the rx error,
dropped, length, crc and multicast counters from dev->stats into per rx
ring u64_stats counters, and sums them in rtl8169_get_stats64().

> r8169: enable new interrupt mapping
>
> This patch enables new interrupt mapping for RTL8127 and add error pkts
> counter per ring.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 4656ae108f9c6..20eb7987f03dc 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -4092,6 +4111,9 @@ static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
>  	RTL_W32(tp, RSS_CTRL_8125, 0);
>  	RTL_W16(tp, Q_NUM_CTRL_8125, 0);
>
> +	if (tp->irq_nvecs > 1)
> +		rtl8169_hw_enable_vec_mapping(tp);
> +

[Severity: Low]
Can this branch be taken at this point in the series?

At this commit, rtl_alloc_irq() still asks for exactly one vector:

rtl_alloc_irq() {
    ...
	nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
    ...
}

so tp->irq_nvecs is always 1, and rtl8169_hw_enable_vec_mapping() never
runs.

rtl_setup_rx_params() also still sets:

	tp->num_rx_rings = 1;

so the new per-ring counter loop in rtl8169_get_stats64() only ever
covers ring 0.

The next patch in the series ("r8169: add support and enable rss") adds
get_min_irq_nvecs()/get_max_irq_nvecs() and rtl8169_set_rx_ring_num().
Only after that does the mapping actually get enabled for RTL8127.

This isn't a bug, but could the commit message say that the mapping
enable stays dormant until a later patch allocates more than one vector,
rather than saying this patch enables it?

>  	/* disable UPS */
>  	r8168_mac_ocp_modify(tp, 0xd40a, 0x0010, 0x0000);

[ ... ]

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