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

[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] In MSI-X mode (tp->irq_nvecs > 1, RTL_GIGA_MAC_VER_80 with at
  least 30 vectors), link-change interrupts go to their own vector,…
- [Low] The commit message of the preceding patch, "r8169: move struct
  ethtool_ops", says the ops struct was moved so that 'subsequent…

--

r8169: add get_channel support for ethtool

This commit adds an ethtool get_channels callback to r8169. In
multi-vector mode it reports RX and TX channel counts. Otherwise it
reports a single combined channel.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index dad0947bcc9e7..c32f993c1aaab 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -6734,6 +6734,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 link-change vector as an "other"
channel?

In MSI-X mode the link-change interrupt has its own vector, which
rtl8169_request_irq() requests with a separate handler:

drivers/net/ethernet/realtek/r8169_main.c:rtl8169_request_irq() {
    ...
			else if (i == MSIX_ID_VEC_MAP_LINKCHG)
				rc = request_irq(irq, rtl8169_interrupt_other,
						 irqflags, dev->name, tp);
    ...
}

rtl_set_irq_mask() also enables ISRIMR_LINKCHG in this mode. Since
max_other and other_count are left at zero, "ethtool -l" reports 0
other channels while a dedicated misc vector is in use.

igb, igc and ixgbe (NON_Q_VECTORS) and iavf (NONQ_VECS) report this kind
of vector as max_other = other_count = 1. This patch is the first to
expose the channels UAPI for r8169, so changing these values later would
be visible to userspace.

Would it make sense to set ch->max_other = 1 and ch->other_count = 1
here?

> +	} else {
> +		ch->max_combined = 1;
> +		ch->combined_count = 1;
> +	}
> +}
> +
>  static const struct ethtool_ops rtl8169_ethtool_ops = {
>  	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>  				     ETHTOOL_COALESCE_MAX_FRAMES,
> @@ -6752,6 +6768,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
>  	.nway_reset		= rtl8169_nway_reset,
>  	.get_eee		= rtl8169_get_eee,
>  	.set_eee		= rtl8169_set_eee,
> +	.get_channels		= rtl8169_get_channels,

[Severity: Low]
This isn't a bug, but the preceding patch in this series, "r8169: move
struct ethtool_ops", says:

    The patch moves the rtl8169_ethtool_ops definition further down in
    r8169_main.c so that subsequent additions of rtl8169_get_channels and
    rtl8169_set_channels can be referenced from the ops struct without
    needing forward declarations.

This is the last patch in the series, and it only adds .get_channels. No
rtl8169_set_channels exists in the driver at the end of the series. The
RX ring count is still fixed once in rtl8169_set_rx_ring_num():

	tp->num_rx_rings = rounddown_pow_of_two(min(rss_queue_num,
						    tp->hw_supp_num_rx_queues));

Was set_channels dropped from the series? If so, should the commit
message of "r8169: move struct ethtool_ops" be updated?

>  	.get_link_ksettings	= rtl8169_get_link_ksettings,
>  	.set_link_ksettings	= rtl8169_set_link_ksettings,
>  	.get_ringparam		= rtl8169_get_ringparam,

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