Re: [PATCH net-next v14 1/7] r8169: prepare for multi irqs and napi

[netdev-bot+sashiko]

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] Commit message claims "there is no functional change", but the
  patch introduces two user-visible changes in…

--

r8169: prepare for multi irqs and napi

This converts the single struct napi_struct in struct rtl8169_private into a
dynamically allocated array sized by the number of IRQ vectors, and moves
IRQ request/free into helpers that iterate over that array.  irq_nvecs and
num_rx_rings stay at 1, so this is described as a preparation step for RSS.

>     Currently, irq_nvecs and num_rx_rings are still hard-coded
>     to 1, so there is no functional change.

[Severity: Low]
Is the "no functional change" statement accurate?  There look to be two
observable changes for the currently supported single-vector case.

The first is the probe banner in rtl_init_one(), which gains a field that
always reads "(1 total)" while irq_nvecs is hard-coded to 1.

The second is the new netif_napi_set_irq() call in r8169_init_napi().
netif_napi_add_weight_locked() leaves napi->irq at -1, and
netdev_nl_napi_fill_one() only emits the attribute when it is non-negative:

net/core/netdev-genl.c:netdev_nl_napi_fill_one() {
	...
	if (napi->irq >= 0 && nla_put_u32(rsp, NETDEV_A_NAPI_IRQ, napi->irq))
		goto nla_put_failure;
	...
}

So NETDEV_A_NAPI_IRQ goes from absent to present in NETDEV_CMD_NAPI_GET
replies for this driver.

Could the commit message mention both, or could the banner change be moved
to the patch that actually enables more than one vector?

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 5415ff62a286..a8214c48a775 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -6005,6 +6087,15 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
>  	return 0;
>  }
>  
> +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);
> +		netif_napi_set_irq(&tp->rtl8169_napi[i],
> +				   pci_irq_vector(tp->pci_dev, i));
> +	}
> +}
> +

[ ... ]

> @@ -6215,8 +6317,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			tp->leds = rtl8168_init_leds(dev);
>  	}
>  
> -	netdev_info(dev, "%s, %pM, %sXID %x, IRQ %d\n",
> -		    chip->name, dev->dev_addr, ext_xid_str, xid, tp->irq);
> +	netdev_info(dev, "%s, %pM, %sXID %x, IRQ %d (%u total)\n",
> +		    chip->name, dev->dev_addr, ext_xid_str, xid,
> +		    pci_irq_vector(pdev, 0), tp->irq_nvecs);

[ ... ]

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