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

[mingming cao <mmc@linux.ibm.com>]

On 8/17/26 6:47 PM, Jakub Kicinski wrote:
> 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?
Yes. At this commit set_channels still only looks at tx_count, so
ethtool -L rx N can pass core validation and return success without
changing RX. Raising max_rx here just widens that window from the
live count to 16. v6 names the intermediate state in the commit
message, which covers your second option.

The reorder is not done: channel reporting stays reviewable on its
own, separate from stats and from the resize. Patch 14 reads
rx_count and calls resize; patch 15 finishes the down-path and caps
max_rx once mq_fallback latches.
> [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.

Yes. While the latch is set, -l can still show MQ max_rx / rx_count
until the next open applies fallback — apply_mq_fallback() runs at
open entry, so get_channels() here still reads multi_queue and the
published count only, not the latch directly. Not taken here:
patch 15 consults the latch by capping max_rx at the live count,
not by understating rx_count. Clamping rx_count would turn the next
TX-only -L into a silent RX shrink.

Thanks,

Mingming