Re: [net-next,v12,7/7] r8169: add get_channel support for ethtool

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] rtl8169_get_channels()
  (drivers/net/ethernet/realtek/r8169_main.c:6305-6315) unconditionally…
- [Low] rtl8169_get_channels() sets ch->max_rx =
  tp->hw_supp_num_rx_queues…
- [Low] rtl8169_get_channels() hardcodes ch->max_tx = 1 and ch->tx_count
  = 1 (drivers/net/ethernet/realtek/r8169_main.c:6311 and 6314) instead…

--

r8169: add get_channel support for ethtool

This adds a rtl8169_get_channels() callback to rtl8169_ethtool_ops so that
ethtool -l reports the RX/TX channel layout of the device. The reported
values come from tp->hw_supp_num_rx_queues and tp->num_rx_rings for RX, and
from literal 1 for TX. No .set_channels callback is added.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 11fb31223f492..a76fbf1e50d11 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -6302,6 +6302,18 @@ 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);
> +
> +	ch->max_rx = tp->hw_supp_num_rx_queues;

[Severity: Low]
Is hw_supp_num_rx_queues the right value to advertise as the maximum here?

rtl_setup_rx_params() sets it purely from mac_version, before any interrupt
vectors are allocated:

	case RTL_GIGA_MAC_VER_80:
		tp->hw_supp_num_rx_queues = R8169_MAX_RX_QUEUES;

The number of RX rings the driver will actually arm is decided later, in
rtl8169_set_rx_ring_num():

	if (tp->irq_nvecs >= get_min_irq_nvecs(tp)) {
		unsigned int rss_queue_num = netif_get_num_default_rss_queues();

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

and rtl_alloc_irq() can fall back to a single vector:

	nvecs = pci_alloc_irq_vectors(pdev, get_min_irq_nvecs(tp),
				      get_max_irq_nvecs(tp), flags);

	if (nvecs < 0)
		nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);

In that fallback case num_rx_rings stays 1 for the lifetime of the device,
yet ethtool -l reports a maximum of 8. Counts that are not a power of two
are also not usable in principle, since rtl_set_rss_config() and
rtl8125_set_rx_q_num() both use ilog2(tp->num_rx_rings).

Would it be better to bound the advertised maximum by what the driver can
actually arm, for example rounddown_pow_of_two(min(hw_supp_num_rx_queues,
RX vectors derivable from tp->irq_nvecs))? This matters more once a
.set_channels callback is layered on top, because the ethtool core validates
requested counts against max_rx and would accept counts the hardware paths
cannot use.

> +	ch->max_tx = 1;
> +
> +	ch->rx_count = tp->num_rx_rings;
> +	ch->tx_count = 1;

[Severity: Low]
This isn't a bug, but could max_tx and tx_count use R8169_MAX_TX_QUEUES (and
dev->real_num_tx_queues) instead of the literal 1?

The netdev itself is sized from the constant in rtl_init_one():

	dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*tp),
				      R8169_MAX_TX_QUEUES,
				      R8169_MAX_RX_QUEUES);

The values agree today, so nothing is wrong now. If R8169_MAX_TX_QUEUES is
bumped when multi-TX-queue support lands, would this report keep claiming a
single TX channel while the netdev has more?

> +}

[Severity: Medium]
Should the single-interrupt configuration be reported as one combined
channel rather than as one dedicated RX plus one dedicated TX channel?

max_combined and combined_count are left at 0 here, while max_rx/rx_count
and max_tx/tx_count are always populated. For every chip other than
RTL_GIGA_MAC_VER_80 (and for VER_80 whenever rtl_alloc_irq() falls back to
pci_alloc_irq_vectors(pdev, 1, 1, flags)) there is exactly one IRQ and one
NAPI instance, and it services both directions:

rtl_set_irq_mask()
	tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;

r8169_init_napi() installs rtl8169_poll() in that case, which does:

	rtl_tx(dev, tp, budget);

	/* rtl8169_poll() is used only when there is a single RX ring. */
	work_done = rtl_rx(dev, tp, &tp->rx_ring[0], budget, napi);

The uAPI kernel-doc describes max_combined as a "Set of queues RX, TX or
other", and ethtool(8) defines a channel as an IRQ plus the set of queues
that can trigger it. By that definition the single-vector case looks like
one combined channel, so does ethtool -l tell userspace that two
independently steerable channels exist where only one shared channel does?

The split rx/tx model appears accurate only for the RTL8127 MSI-X case,
where vectors below R8169_MAX_RX_QUEUES run rtl8169_poll_msix_rx() and the
rest run rtl8169_poll_msix_tx(). Since this is a newly exposed read-only
ABI value, changing the classification later, when .set_channels is added,
would change what userspace already sees. Could the commit message explain
the reasoning behind the chosen model?

> +
>  static const struct ethtool_ops rtl8169_ethtool_ops = {

[ ... ]

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