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

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

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
>
> - [Medium] Intermediate-state ethtool channel ABI mismatch:
>    ibmveth_get_channels() (drivers/net/ethernet/ibm/ibmveth.c:2537-2549)…
> - [Low] Advertised RX maximum not derived from any driver-side capability
>    check: ibmveth_get_channels() (ibmveth.c:2545-2548) reports a flat…
> - [Low] ibmveth_get_channels()
>    (drivers/net/ethernet/ibm/ibmveth.c:2540-2549) now sources…
>
> --
>
> ibmveth: Report MQ-aware RX counts in ethtool get_channels
>
> get_channels() now reports max_rx as IBMVETH_MAX_RX_QUEUES when MQ firmware
> is enabled (1 otherwise) and reports rx_count from the driver's published
> num_rx_queues instead of netdev->real_num_rx_queues.  The paired
> set_channels() is left unchanged in this patch.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index f4fddfa56571d..4f2d956b4c89e 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -2537,11 +2537,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;
> [Severity: Medium]
> Does raising max_rx here make the kernel admit RX channel requests that the
> driver then silently drops on the floor?
> The core only bounds an RX request against the value this callback returns,
> in ethtool_set_channels():
>
> 	if (channels.rx_count > curr.max_rx ||
> 	    channels.tx_count > curr.max_tx ||
>
> and ethnl_set_channels() does the same check on the netlink path.
> ibmveth_set_channels() at this commit only consumes tx_count:
>
> 	unsigned int old = netdev->real_num_tx_queues,
> 		     goal = channels->tx_count;
>
> channels->rx_count is neither read nor rejected.  Since ethtool -L is a
> read-modify-write, tx_count arrives unchanged and the TX grow/shrink loops
> do not run, so "ethtool -L ethN rx 4" on an MQ adapter returns 0, generates
> an ETHTOOL_MSG_CHANNELS_NTF, and changes nothing; a following "ethtool -l"
> still shows the old rx_count.
>
> Before this patch max_rx equalled the live count, so the same request was
> rejected with -EINVAL by the core, which is why this reads as a behaviour
> change rather than a pre-existing gap.
>
> The commit message does acknowledge this:
>
>    "This patch only reports. set_channels() still reads only tx_count, so
>    an RX channel request returns success without changing anything;
>    raising max_rx widens the range of requests that behave that way from
>    the live count to IBMVETH_MAX_RX_QUEUES."
>
> and the later patches do close it: by the end of the series
> ibmveth_set_channels() reads channels->rx_count and calls
> ibmveth_resize_rx_channels(), which validates the range:
>
> 	if (goal_rx < 1 || goal_rx > IBMVETH_MAX_RX_QUEUES) {
> 		...
> 		return -EINVAL;
> 	}
>
> Would it be possible to order the widening of max_rx after the setter
> learns to consume rx_count, so no bisect point reports success for a
> request it ignores?
No, at this commit set_channels() checks
that rx_count matches the live count and
returns -EOPNOTSUPP if userspace attempts to
change RX queues. Read-modify-write TX-only
requests pass through with the live rx_count.
Patch 14 implements live RX resizing.
Patch 15 is the tip ABI: live rx_count,
max_rx capped once mq_fallback, and max_tx
at least the live tx_count.
> [Severity: Low]
> Should this ceiling be derived from a capability check rather than being a
> flat 16?
> The TX side bounds its advertised maximum by the CPU count:
>
> 	static unsigned int ibmveth_real_max_tx_queues(void)
> 	{
> 		unsigned int n_cpu = num_online_cpus();
>
> 		return min(n_cpu, IBMVETH_MAX_QUEUES);
> 	}
>
> while the RX side reports IBMVETH_MAX_RX_QUEUES (16) purely on
> adapter->multi_queue, even though the driver's own MQ default is
> min(num_online_cpus(), IBMVETH_DEFAULT_QUEUES), which is at most 8.
>
> The test also ignores adapter->mq_fallback.  After firmware rejects MQ,
> multi_queue stays true until the next open runs
> ibmveth_apply_mq_fallback():
>
> 	adapter->multi_queue = false;
> 	ibmveth_publish_num_rx_queues(adapter, 1);
> 	/* real_num_rx_queues is set later in open after resources exist. */
>
> so in that window ethtool -l advertises max_rx = 16 for a device the driver
> has already decided will come up single-queue.
>
> The mq_fallback half is handled later in the series, where get_channels()
> becomes:
>
> 	if (adapter->multi_queue && !adapter->mq_fallback)
> 		channels->max_rx = IBMVETH_MAX_RX_QUEUES;
> 	else
> 		channels->max_rx = rx_count;
>
> The flat 16 does match the range the end-of-series setter accepts, so this
> is mostly a question about whether the commit message should say why the RX
> maximum intentionally skips the num_online_cpus() cap that TX applies.
16 is IBMVETH_MAX_RX_QUEUES, the ethtool
ceiling. TX uses the same max cap. The
default is min(cpus, 8). No extra PHYP
capability query this series.
>> +	channels->rx_count = ibmveth_get_num_rx_queues(adapter);
> [Severity: Low]
> Can this disagree with the RX queue count the netdev core enumerates?
>
> The queue-stats side of the same driver deliberately keys on the core's
> count, in ibmveth_get_base_stats():
>
> 	for (i = dev->real_num_rx_queues; i < IBMVETH_MAX_RX_QUEUES; i++) {
>
> with a comment saying the split is bound by real_num_*_queues so it lines up
> with the core's iteration exactly.  After this patch, ethtool -l reports the
> adapter's private count instead.
>
> The two can drift.  ibmveth_apply_mq_fallback() publishes num_rx_queues = 1
> at the very start of open, and netif_set_real_num_rx_queues() is only
> reached much later:
>
> 	rc = ibmveth_register_rx_queues(adapter, mac_address);
> 	if (rc)
> 		goto out_free_buffer_pools;
>
> 	rc = netif_set_real_num_rx_queues(netdev,
> 					  ibmveth_get_num_rx_queues(adapter));
>
> If ibmveth_alloc_filter_list(), ibmveth_alloc_rx_queues(),
> ibmveth_alloc_buffer_pools() or ibmveth_register_rx_queues() fails, open
> returns an error with adapter->num_rx_queues == 1 while
> netdev->real_num_rx_queues still holds the previous value, and
> ibmveth_close() never lowers it.
>
> In that state ethtool -l reports rx_count = 1, the core still enumerates the
> old number of RX queues for netlink per-queue stats, and the per-queue
> ethtool -S strings (which use the adapter count) list only queue 0.
>
> No out-of-bounds access results, since rx_qstats[] is sized
> IBMVETH_MAX_RX_QUEUES and the live/retired split stays non-overlapping, and
> the state self-heals on the next successful open.  This divergence is still
> present at the end of the series, where get_channels() uses the adapter
> count and get_base_stats() uses real_num_rx_queues.  Would keying both on
> the same counter be preferable?
Yes, if this open fails first. real_num is
set after the resources exist; the helper
only publishes the count. The next successful
open calls set_real. get_base_stats stays
keyed on the core's count.

Thanks,
Mingming