Re: [PATCH net-next v14 3/7] r8169: add support for 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] DISCARDED (series-resolved): dead code — every new `tp->irq_nvecs
  > 1` branch added by this patch is unreachable at this commit because…
- [Low] DISCARDED (series-resolved): claim that a partial MSI-X grant
  with 2 <= irq_nvecs <= 29 arms hardware sources (TOK bit 8, LINKCHG…
- [Low] rtl8169_poll_msix_tx() calls napi_complete_done()
  unconditionally, including when the core invokes the poll callback…
- [Low] The new Tx-only NAPI instance (index R8169_MAX_RX_QUEUES, poll
  routine rtl8169_poll_msix_tx()) is registered with netif_napi_add()…
- [Low] In multi-vector mode rtl8169_netpoll() invokes
  rtl8169_interrupt_msix() for every data-path NAPI slot…

--

r8169: add support for new interrupt mapping

This patch adds the new per-vector interrupt mapping registers
(IMR_CLEAR_VEC_MAP_REG, ISR_VEC_MAP_REG, IMR_SET_VEC_MAP_REG) and new
MSI-X handlers plus per-vector Rx/Tx NAPI poll routines. The old
single-vector paths are kept for the tp->irq_nvecs <= 1 case. A few
questions below about the new poll and netpoll paths.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 7a2e7ce56e48..c647b4327ff7 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -5396,9 +5491,16 @@ static int rtl8169_close(struct net_device *dev)
>  static void rtl8169_netpoll(struct net_device *dev)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
> +	int napi_num;
> +
> +	napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
>  
> -	for (int i = 0; i < tp->irq_nvecs; i++)
> -		rtl8169_interrupt(0, &tp->rtl8169_napi[i]);
> +	for (int i = 0; i < napi_num; i++) {
> +		if (tp->irq_nvecs > 1)
> +			rtl8169_interrupt_msix(0, &tp->rtl8169_napi[i]);
> +		else
> +			rtl8169_interrupt(0, &tp->rtl8169_napi[i]);
> +	}
>  }

[Severity: Low]
Should this loop be bounded by tp->num_rx_rings rather than by
R8169_MAX_RX_QUEUES + 1?

In multi-vector mode this walks all nine data-path slots regardless of how
many Rx rings are actually configured. For a slot whose index is >=
tp->num_rx_rings, rtl8169_poll_msix_rx() skips rtl_rx() entirely and then
still re-arms the vector:

	if (message_id < tp->num_rx_rings)
		work_done += rtl_rx(dev, tp, &tp->rx_ring[message_id],
				    budget, napi);

	if (work_done < budget && napi_complete_done(napi, work_done))
		rtl8169_enable_hw_interrupt_msix(tp, message_id);

rtl_set_irq_mask() deliberately only sets the ROK bits for the active rings:

	for (int i = 0; i < tp->num_rx_rings; i++)
		tp->irq_mask |= ISRIMR_ROK_Q0 << i;

so after one netpoll pass, does the hardware mask in IMR_SET_VEC_MAP_REG end
up containing ROK bits that tp->irq_mask excludes? tp->num_rx_rings is
normally smaller than 8 (it is derived from
netif_get_num_default_rss_queues() rounded down to a power of two), while
nine NAPI instances are always created in multi-vector mode, so slots 1..8
can be inactive on a typical machine.

> @@ -5654,10 +5758,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;
> +	} else {
> +		tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
>  
> -	if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
> -		tp->irq_mask |= SYSErr | RxFIFOOver;
> +		if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
> +			tp->irq_mask |= SYSErr | RxFIFOOver;
> +	}
>  }

[ ... ]

> @@ -6203,10 +6313,63 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
>  	return 0;
>  }
>  
> +static int rtl8169_poll_msix_rx(struct napi_struct *napi, int budget)
> +{

[ ... ]

> +static int rtl8169_poll_msix_tx(struct napi_struct *napi, int budget)
> +{
> +	struct net_device *dev = napi->dev;
> +	struct rtl8169_private *tp;
> +
> +	tp = netdev_priv(dev);
> +
> +	/* Currently r8169 only supports a single Tx ring.
> +	 * Therefore, we don't need a per-ring Tx processing loop here.
> +	 */
> +	rtl_tx(dev, tp, budget);
> +
> +	if (napi_complete_done(napi, 0))
> +		rtl8169_enable_hw_interrupt_msix(tp, (int)(napi - tp->rtl8169_napi));
> +
> +	return 0;
> +}

[Severity: Low]
Should this call to napi_complete_done() be skipped when budget is 0?

Documentation/networking/napi.rst states:

   If the ``budget`` is 0 napi_complete_done() should never be called.

and netpoll does invoke the poll callback with a zero budget:

net/core/netpoll.c:poll_one_napi() {
	/* We explicitly pass the polling call a budget of 0 to
	 * indicate that we are clearing the Tx path only.
	 */
	work = napi->poll(napi, 0);
}

Today the effect appears to be absorbed by napi_complete_done() itself,
which returns early when NAPI_STATE_NPSVC is set (the flag poll_one_napi()
sets before the zero-budget call), so the Tx vector is not re-armed behind
the core's back. Would the documented form be preferable here:

	if (budget && napi_complete_done(napi, 0))
		rtl8169_enable_hw_interrupt_msix(tp, ...);

> +/* RTL8127 MSI-X vector layout:
> + * Vectors 0 .. (MAX_RXQ - 1)			: Rx Queues
> + * Vectors MAX_RXQ .. (MAX_RXQ + MAX_TXQ - 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_RX_QUEUES + 1);
> +
> +	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: Low]
Would netif_napi_add_tx() be the right registration helper for the slot at
index R8169_MAX_RX_QUEUES, since rtl8169_poll_msix_tx() only calls rtl_tx()
and never touches an Rx ring?

The kernel-doc for netif_napi_add_tx() in include/linux/netdevice.h says it
"should be used from drivers using NAPI to exclusively poll a TX queue", and
netif_napi_add_tx_weight() additionally does:

	set_bit(NAPI_STATE_NO_BUSY_POLL, &napi->state);

With plain netif_napi_add(), does this Tx-only instance end up in napi_hash[]
and advertised as busy-poll capable?

>  		netif_napi_set_irq(&tp->rtl8169_napi[i],
>  				   pci_irq_vector(tp->pci_dev, i));
>  	}

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