Re: [PATCH net-next v15 3/7] r8169: add support for new interrupt mapping

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/ethernet/realtek/r8169_main.c,
  rtl8169_request_irq() registers rtl8169_interrupt_msix for every…
- [Low] Concern: the commit message says the patch adds support for the
  new interrupt mapping, but every new path is gated by tp->irq_nvecs >…

--

r8169: add support for new interrupt mapping

This commit adds the vector-map interrupt registers (IMR_SET_VEC_MAP_REG,
IMR_CLEAR_VEC_MAP_REG, ISR_VEC_MAP_REG). It also adds per-vector MSI-X
handlers for the RX queues, the TX queue and link change, plus separate
RX and TX NAPI poll functions. All of the new paths are gated on
tp->irq_nvecs > 1.

> To support RSS, the number of hardware interrupt bits should match the
> interrupt of software. So we add support for new interrupt mapping here.

[Severity: Low]
This isn't a bug, but every new path in this patch is gated on
tp->irq_nvecs > 1. At this commit rtl_alloc_irq() still does:

    nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);

so none of the new vector-map code can be reached yet.

The next patch in the series, "r8169: enable new interrupt mapping",
changes rtl_alloc_irq() to request get_min_irq_nvecs()..get_max_irq_nvecs()
vectors, and that is what turns this code on.

Could the commit message say that this patch only adds the infrastructure
and that the next patch enables it?

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

[ ... ]

> @@ -5219,18 +5310,28 @@ static int rtl8169_request_irq(struct rtl8169_private *tp)
>  		napi = &tp->rtl8169_napi[i];
>  		irq = pci_irq_vector(pdev, i);
>  
> -		rc = request_irq(irq, rtl8169_interrupt, irqflags,
> -				 dev->name, napi);
> +		if (tp->irq_nvecs > 1) {
> +			/* RX: 0~7, TX: 8*/
> +			if (i <= R8169_MAX_RX_QUEUES)
> +				rc = request_irq(irq, rtl8169_interrupt_msix,
> +						 irqflags, dev->name, napi);

[Severity: Medium]
This installs rtl8169_interrupt_msix() on every vector from 0 to 8, with
&tp->rtl8169_napi[i] as dev_id. However, r8169_init_napi() only calls
netif_napi_add() for the active RX rings and for index
R8169_MAX_RX_QUEUES:

    for (i = 0; i < tp->num_rx_rings; i++) {
        netif_napi_add(tp->dev, &tp->rtl8169_napi[i],
                       rtl8169_poll_msix_rx);

What happens if a message arrives on a vector between num_rx_rings and 7?
Those napi entries are still zeroed from kcalloc(), so
rtl8169_interrupt_msix() would do:

    struct net_device *dev = napi->dev;    /* NULL */
    ...
    tp = netdev_priv(dev);
    message_id = napi - tp->rtl8169_napi;

    rtl8169_disable_hw_interrupt_msix(tp, message_id);

Could this oops in hardirq context when RTL_W32() goes through
tp->mmio_addr? If it didn't fault, it would call napi_schedule() on a
NAPI that was never added.

This can't happen at this commit, because only one vector is allocated.
Later in the series, rtl_alloc_irq() requests 30 to 32 vectors on
RTL_GIGA_MAC_VER_80. num_rx_rings comes from
rounddown_pow_of_two(min(netif_get_num_default_rss_queues(), 8)), which
is 1 in a kdump kernel and below 8 on many machines.

Only the vector-map IMR keeps these vectors quiet, and it isn't cleared
before the handlers are installed:

  - At probe, rtl8169_irq_mask_and_ack() runs before rtl_alloc_irq() sets
    irq_nvecs. It therefore clears IntrMask_8125, not the vector-map IMR.
  - In rtl_open(), rtl8169_request_irq() runs before rtl8169_up() resets
    the vector-map IMR.

Could IMR state left by a previous kernel fire one of these vectors as
soon as request_irq() unmasks it? One example is a crashed kernel before
kdump that had more rings enabled.

Would it make sense to request the MSI-X handler only for
i < tp->num_rx_rings, i == R8169_MAX_RX_QUEUES and
i == MSIX_ID_VEC_MAP_LINKCHG, so the set matches the NAPI setup?
rtl8169_free_one_irq() uses the same i <= R8169_MAX_RX_QUEUES check and
would need the matching change.

> +			else if (i == MSIX_ID_VEC_MAP_LINKCHG)
> +				rc = request_irq(irq, rtl8169_interrupt_other,
> +						 irqflags, dev->name, tp);
> +			else
> +				continue;

[ ... ]

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