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?
[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.
> + 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?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788102125.git.mmc%40linux.ibm.com
