Re: [PATCH net-next v5 12/15] ibmveth: Report MQ-aware RX counts in ethtool get_channels

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ibmveth: Report MQ-aware RX counts in ethtool get_channels

The patch changes ibmveth_get_channels() to report max_rx as
IBMVETH_MAX_RX_QUEUES when the adapter has multi-queue firmware support
(else 1), and to report rx_count from the driver's published
num_rx_queues instead of netdev->real_num_rx_queues.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 3202b657c9b8f..5d4ca4b3d3d07 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -2522,11 +2522,16 @@ static void ibmveth_get_ethtool_stats(struct net_device *dev,
>  static void ibmveth_get_channels(struct net_device *netdev,
>  				 struct ethtool_channels *channels)
>  {
> +	struct ibmveth_adapter *adapter = netdev_priv(netdev);
> +
>  	channels->max_tx = ibmveth_real_max_tx_queues();
>  	channels->tx_count = netdev->real_num_tx_queues;
>  
> -	channels->max_rx = netdev->real_num_rx_queues;
> -	channels->rx_count = netdev->real_num_rx_queues;
> +	if (adapter->multi_queue)
> +		channels->max_rx = IBMVETH_MAX_RX_QUEUES;
> +	else
> +		channels->max_rx = 1;
> +	channels->rx_count = ibmveth_get_num_rx_queues(adapter);
>  }

[Severity: Medium]
At this point in the series, does raising max_rx here make RX channel
requests silently succeed without changing anything?

ibmveth_set_channels() only derives its work from tx_count:

	unsigned int old = netdev->real_num_tx_queues,
		     goal = channels->tx_count;
	...
	if (!(netdev->flags & IFF_UP))
		return netif_set_real_num_tx_queues(netdev, goal);

channels->rx_count is never read there.  The core validation in
ethtool_set_channels() only bounds the request by the driver-reported
maximum:

	if (channels.rx_count > curr.max_rx ||
	    channels.tx_count > curr.max_tx ||

so with max_rx now 16, "ethtool -L ethX rx N" for any N in 1..16 that
differs from the current RX count passes validation, enters
ibmveth_set_channels() with tx_count unchanged, runs no LTB loop
iterations, and returns 0.  The netlink path in ethnl_set_channels()
applies the same max_rx-only ceiling and maps the 0 return to success.

Before this change max_rx equalled real_num_rx_queues, so an increase was
rejected with -EINVAL by the core; afterwards it is accepted and ignored,
and "ethtool -l" still shows the old rx_count.

For the record, this appears to be resolved later in the same series: the
final patch "ibmveth: Wire ethtool set_channels to MQ RX queue resize"
makes ibmveth_set_channels() read channels->rx_count and apply it via
ibmveth_resize_rx_channels() with range validation of
1..IBMVETH_MAX_RX_QUEUES, so the get/set pair agrees at the end of the
series.  Would it be worth ordering that patch before this one, or
mentioning the intermediate state in the changelog?

[Severity: Low]
A second, smaller point about the same lines: max_rx is derived only from
adapter->multi_queue and does not consult adapter->mq_fallback.  After
firmware refuses multi-queue, ibmveth_apply_mq_fallback() defers the
downgrade to the next open:

	if (!adapter->mq_fallback)
		return;

	adapter->mq_fallback = false;
	...
	adapter->multi_queue = 0;
	ibmveth_publish_num_rx_queues(adapter, 1);

so between the firmware rejection and the next open, "ethtool -l" keeps
reporting max_rx = 16 and rx_count = 8 while the adapter is going to run
single-queue.  Should get_channels() also take mq_fallback into account?

Also, at this patch the reported maximum of IBMVETH_MAX_RX_QUEUES (16) is
above anything the driver publishes, since ibmveth_probe() caps the RX
queue count:

		adapter->multi_queue = 1;
		ibmveth_publish_num_rx_queues(adapter,
					      min(num_online_cpus(),
						  IBMVETH_DEFAULT_QUEUES));

The end-of-series patch "ibmveth: Wire ethtool set_channels to MQ RX
queue resize" does make 1..IBMVETH_MAX_RX_QUEUES genuinely settable via
ibmveth_resize_rx_channels(), so this is only about the intermediate
state and the stale reporting while mq_fallback is latched.