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

[netdev-bot+sashiko]

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

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

- [High] Concurrent execution of lockless `rtl_tx()` due to redundant Tx
  NAPIs being scheduled simultaneously by `netpoll`.
- [Medium] NAPI completion unconditionally unmasks hardware interrupts
  for vectors that were explicitly left masked, risking unexpected…
- [Low] Concern (assessed as a FALSE POSITIVE): every new code path added
  by this patch is gated on tp->irq_nvecs > 1, but at this commit…
- [Low] Concern (assessed as a FALSE POSITIVE): rtl_set_irq_mask()
  unmasks ISRIMR_LINKCHG (BIT(29)) and ISRIMR_TOK_Q0 (BIT(8)) for any…
- [Low] Concern (assessed as a FALSE POSITIVE): the new comment above
  r8169_init_napi ("RTL8127 MSI-X vector layout: Vectors 0 .. (RxQs -…
- [Low] Concern (assessed as a FALSE POSITIVE): with per-queue Rx NAPIs,
  rtl_rx() could run on several CPUs at once while still doing plain…

--

r8169: add support for new interrupt mapping

This routes the 8125-style ack/mask/unmask helpers through the new
per-vector mapping registers (IMR_CLEAR_VEC_MAP_REG, ISR_VEC_MAP_REG,
IMR_SET_VEC_MAP_REG) when more than one IRQ vector is allocated. It adds
per-vector MSI-X handlers plus separate Rx and Tx poll routines, a
link-change handler on message id 29, and adjusts the NAPI allocation,
free and netpoll loops for the new layout.

No regressions to report from this review pass. The candidate concerns
that came up were checked against the code and did not hold up; they are
noted inline below purely for the record.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 9311a0cab4eb..ca6be1e38408 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -5047,8 +5124,26 @@ static int rtl8169_request_irq(struct rtl8169_private *tp)
>  
>  	for (i = 0; i < tp->irq_nvecs; i++) {
>  		napi = &tp->rtl8169_napi[i];
> -		rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
> -				     NULL, napi, "%s-%d", dev->name, i);
> +		if (tp->irq_nvecs > 1) {
> +			if (i < R8169_MAX_QUEUES)
> +				rc = pci_request_irq(tp->pci_dev, i,
> +						     rtl8169_interrupt_msix,
> +						     NULL, napi, "%s-%d",
> +						     dev->name, i);
> +			else if (i == MSIX_ID_VEC_MAP_LINKCHG)
> +				rc = pci_request_irq(tp->pci_dev, i,
> +						     rtl8169_interrupt_other,
> +						     NULL, tp, "%s-%d",
> +						     dev->name, i);
> +			else
> +				continue;
> +		} else {

Checked and not an issue: at this commit rtl_alloc_irq() still calls
pci_alloc_irq_vectors(pdev, 1, 1, flags), so tp->irq_nvecs is always 1
and none of the new branches run yet. The follow-up patch
"r8169: enable new interrupt mapping" switches the allocation to
get_min_irq_nvecs()/get_max_irq_nvecs(). Splitting "add support" from
"enable" this way is fine and leaves no broken intermediate state.

[ ... ]

> @@ -5508,10 +5612,16 @@ static const struct net_device_ops rtl_netdev_ops = {
>  
>  static void rtl_set_irq_mask(struct rtl8169_private *tp)
>  {
> -	tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
> +	if (tp->irq_nvecs > 1) {
> +		tp->irq_mask = ISRIMR_LINKCHG | ISRIMR_TOK_Q0;
> +		for (int i = 0; i < tp->num_rx_rings; i++)
> +			tp->irq_mask |= ISRIMR_ROK_Q0 << i;

Checked and not an issue: unmasking ISRIMR_TOK_Q0 (bit 8) and
ISRIMR_LINKCHG (bit 29) for any tp->irq_nvecs > 1 would only be a
problem for a vector count in the 2..29 range, since rtl8169_request_irq()
installs the Tx NAPI handler at vector 8 and the link handler at vector
29. That range cannot occur: get_min_irq_nvecs() returns 30 for
RTL_GIGA_MAC_VER_80 and 1 otherwise, and rtl_alloc_irq() falls back to
exactly one vector, so tp->irq_nvecs ends up in {1, 30, 31, 32}.

[ ... ]

> @@ -5796,10 +5906,64 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp)
>  	return false;
>  }
>  
> +static int rtl8169_poll_msix_rx(struct napi_struct *napi, int budget)
> +{
> +	struct net_device *dev = napi->dev;
> +	struct rtl8169_private *tp;
> +	int work_done = 0;
> +	int message_id;
> +
> +	tp = netdev_priv(dev);
> +	message_id = napi - tp->rtl8169_napi;
> +
> +	if (message_id < tp->num_rx_rings)
> +		work_done += rtl_rx(dev, tp, &tp->rx_ring[message_id],
> +				    budget, napi);

[Severity: Medium]
The two new poll routines re-enable the hardware vector
unconditionally on completion, which loses the masking decision that
rtl_set_irq_mask() just made. With irq_nvecs > 1, rtl_set_irq_mask()
only unmasks ISRIMR_ROK_Q0 << i for i < tp->num_rx_rings, plus
ISRIMR_TOK_Q0 and ISRIMR_LINKCHG; the bits for the unused Rx message
ids (num_rx_rings .. R8169_MAX_RX_QUEUES - 1) and for the redundant Tx
ids (R8169_MAX_RX_QUEUES + 1 .. R8169_MAX_QUEUES - 1) are deliberately
left clear. But r8169_init_napi() still adds a NAPI for every one of
those message ids, and rtl8169_poll_msix_rx() / rtl8169_poll_msix_tx()
write BIT(message_id) into IMR_SET_VEC_MAP_REG whenever
napi_complete_done() returns true, with no reference to tp->irq_mask.

Those NAPI instances are reachable: rtl8169_netpoll() walks all
min(tp->irq_nvecs, R8169_MAX_QUEUES) entries and calls
rtl8169_interrupt_msix() on each, which schedules the NAPI, and the
poll then unmasks the vector. After a single netpoll pass the hardware
mask no longer matches what the driver thinks it programmed, and
vectors the driver never intended to arm are live. This is dormant in
this commit because rtl_alloc_irq() still asks for exactly one vector,
but it goes live with the follow-up "r8169: enable new interrupt
mapping".

What is the intended fix here? Gating the unmask on
tp->irq_mask & BIT(message_id) would keep the two in sync, but it may
be cleaner to only create NAPI instances for the message ids that are
actually in use, which would also drop the message_id <
tp->num_rx_rings test in rtl8169_poll_msix_rx().

Checked and not an issue: the worry was that per-queue Rx NAPIs let
rtl_rx() run concurrently while still doing plain increments of the
shared dev->stats fields. At this commit tp->num_rx_rings is
unconditionally 1 (the only assignment is in rtl_setup_rx_params()), so
only ring 0 is polled, and the follow-up patch converts those counters to
per-ring u64_stats with one writer per ring before RSS is enabled.

[ ... ]

> +/* RTL8127 MSI-X vector layout:
> + * Vectors 0 .. (RxQs - 1)		: Rx Queues
> + * Vectors RxQs .. (RxQs + TxQs - 1)	: Tx Queues
> + * NAPI is only allocated for data path
> + */
>  static void r8169_init_napi(struct rtl8169_private *tp)
>  {
> -	for (int i = 0; i < tp->irq_nvecs; i++)
> -		netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
> +	int napi_num = min(tp->irq_nvecs, R8169_MAX_QUEUES);
> +
> +	for (int i = 0; i < napi_num; i++) {
> +		int (*poll_fn)(struct napi_struct *, int) = rtl8169_poll;
> +
> +		if (tp->irq_nvecs > 1) {
> +			if (i < R8169_MAX_RX_QUEUES)
> +				poll_fn = rtl8169_poll_msix_rx;
> +			else
> +				poll_fn = rtl8169_poll_msix_tx;
> +		}
> +		netif_napi_add(tp->dev, &tp->rtl8169_napi[i], poll_fn);
> +	}
>  }

[Severity: High]
Every index from R8169_MAX_RX_QUEUES up to R8169_MAX_QUEUES - 1 gets
the same poll_fn here, so eight NAPI instances all end up running
rtl8169_poll_msix_tx(), and that function calls rtl_tx() on the one
shared Tx ring unconditionally - it does not derive a ring index from
napi - tp->rtl8169_napi at all, as the comment you added acknowledges.

rtl_tx() is lockless by design: it reads tp->dirty_tx, walks
tp->TxDescArray / tp->tx_skb, calls rtl8169_unmap_tx_skb() and
napi_consume_skb() and then does WRITE_ONCE(tp->dirty_tx, dirty_tx).
Its only serialization is that a single NAPI instance owns the ring.
With eight NAPIs bound to the same reclaim path that guarantee is
gone. rtl8169_netpoll() schedules all min(tp->irq_nvecs,
R8169_MAX_QUEUES) instances in one go, so several of them can be
running rtl_tx() on different CPUs, each reclaiming the same entries:
that is a double dma_unmap_single() and a double napi_consume_skb() on
the same skb, plus a torn tp->dirty_tx update and a bogus
netif_subqueue_completed_wake() accounting. The Tx vector interrupt
itself can add another concurrent caller on top of that.

Since only a single Tx ring exists, the extra Tx NAPIs look like they
have nothing to do. Would it not be better to create exactly one Tx
NAPI (and size napi_num accordingly) instead of padding out to
R8169_MAX_QUEUES? If the padding is needed for the message-id to NAPI
index arithmetic in rtl8169_interrupt_msix(), please say so, but then
the surplus indices must not be given a poll function that touches the
Tx ring.

Checked and not an issue: reading RxQs in the comment as the chip's fixed
Rx message id count (8, matching R8169_MAX_RX_QUEUES and
tp->hw_supp_num_rx_queues for RTL_GIGA_MAC_VER_80) makes it agree with
both the R8169_MAX_RX_QUEUES split here and ISRIMR_TOK_Q0 being BIT(8).

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