Re: [PATCH net-next v14 7/7] r8169: add get_channel support for ethtool

[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] rtl8169_get_channels()
  (drivers/net/ethernet/realtek/r8169_main.c:6716-6729) does not report…

--

r8169: add get_channel support for ethtool

Adds a .get_channels ethtool callback to the r8169 driver. In multi-vector
(MSI-X) mode it reports the RX and TX channel maximums and counts, and in
single-vector mode it reports one combined channel.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 3be6f6e708f2..7a61c2ecc7d1 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -6713,6 +6713,22 @@ static void r8169_init_napi(struct rtl8169_private *tp)
>  	}
>  }
>  
> +static void rtl8169_get_channels(struct net_device *dev,
> +				 struct ethtool_channels *ch)
> +{
> +	struct rtl8169_private *tp = netdev_priv(dev);
> +
> +	if (tp->irq_nvecs > 1) {
> +		ch->max_rx = tp->hw_supp_num_rx_queues;
> +		ch->max_tx = 1;
> +		ch->rx_count = tp->num_rx_rings;
> +		ch->tx_count = 1;

[Severity: Low]
Should this branch also report the dedicated link-change vector via
max_other/other_count?

In multi-vector mode rtl8169_request_irq() installs a separate handler on a
vector that is neither an RX nor the TX queue vector:

	if (tp->irq_nvecs > 1) {
		if (i <= R8169_MAX_RX_QUEUES)
			rc = request_irq(irq, rtl8169_interrupt_msix,
					 irqflags, dev->name, napi);
		else if (i == MSIX_ID_VEC_MAP_LINKCHG)
			rc = request_irq(irq, rtl8169_interrupt_other,
					 irqflags, dev->name, tp);

and rtl_set_irq_mask() arms that source in the same mode:

	if (tp->irq_nvecs > 1) {
		tp->irq_mask = ISRIMR_LINKCHG | ISRIMR_TOK_Q0;

Since ch is zero-initialized by the caller and channels_fill_reply() in
net/ethtool/channels.c only emits the OTHER attributes when max_other is
non-zero:

	    (channels->max_other &&
	     (nla_put_u32(skb, ETHTOOL_A_CHANNELS_OTHER_MAX,
			  channels->max_other) ||
	      nla_put_u32(skb, ETHTOOL_A_CHANNELS_OTHER_COUNT,
			  channels->other_count))) ||

ethtool -l reports no "other" channel even though that interrupt is active.
ixgbe_get_channels() reports the equivalent non-queue vector:

	/* report info for other vector */
	if (adapter->flags & IXGBE_FLAG_MSIX_ENABLED) {
		ch->max_other = NON_Q_VECTORS;
		ch->other_count = NON_Q_VECTORS;
	}

Would setting ch->max_other = ch->other_count = 1 in the tp->irq_nvecs > 1
branch be more accurate here? The single-vector branch looks fine as is,
since link-change events share the one combined interrupt.

> +	} else {
> +		ch->max_combined = 1;
> +		ch->combined_count = 1;
> +	}
> +}

[ ... ]

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