Thanks for the review!
On 9/3/26 11:10 AM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 3
- [Medium] Advertised RX channel range does not match the enforced range
once `mq_fallback` latches.
- [Low] In `ibmveth_set_channels()` the comment above the RX rollback
describes code that no longer exists: it claims…
- [Low] The new up-path TX-failure rollback in `ibmveth_set_channels()`
calls `ibmveth_resize_rx_queues_incremental(adapter, old_rx,…
- [Low] `adapter->mq_fallback` is written with a plain store from
softirq/NAPI context — `ibmveth_replenish_task()` does…
--
ibmveth: Complete set_channels down-path and mq_fallback max_rx cap
This completes the ethtool -L path in ibmveth: the down-path publishes the
desired RX count without allocating, the up-path resizes RX then TX and
rolls RX back if TX cannot reach the requested count, and get_channels()
now caps max_rx at the live rx_count once mq_fallback is set.
ibmveth_poll_controller() also returns early when the adapter is closed.
diff --git a/drivers/net/ethernet/ibm/ibmveth.c
b/drivers/net/ethernet/ibm/ibmveth.c
index 5aef8a1f2c23d..4cd00ff3d43e4 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -3156,15 +3156,24 @@ static void ibmveth_get_channels(struct net_device
*netdev,
struct ethtool_channels *channels)
{
struct ibmveth_adapter *adapter = netdev_priv(netdev);
+ unsigned int rx_count = ibmveth_get_num_rx_queues(adapter);
channels->max_tx = ibmveth_real_max_tx_queues();
channels->tx_count = netdev->real_num_tx_queues;
- if (adapter->multi_queue)
+ /*
+ * Always report the live RX count. ethtool -L is read-modify-
+ * write, so a TX-only request echoes rx_count back at us; an
+ * understated value would be applied as a silent RX shrink.
+ * mq_fallback instead caps max_rx at the live count, which
+ * blocks growth in the core without misreporting what is
+ * currently configured.
+ */
+ channels->rx_count = rx_count;
+ if (adapter->multi_queue && !adapter->mq_fallback)
channels->max_rx = IBMVETH_MAX_RX_QUEUES;
else
- channels->max_rx = 1;
- channels->rx_count = ibmveth_get_num_rx_queues(adapter);
+ channels->max_rx = rx_count;
}
[Severity: Medium]
Once mq_fallback is set, does the advertised max_rx still match what the
driver will accept?
Here max_rx becomes the live rx_count, which can be 8 or 16. The core only
bounds the request by that value, in ethtool_set_channels():
if (channels.rx_count > curr.max_rx ||
channels.tx_count > curr.max_tx ||
So every value in 1..rx_count reaches the driver. But
ibmveth_resize_rx_channels() refuses any changed count above 1 in that
state, after the no-op shortcut:
if (goal_rx == old_rx)
return 0;
...
if (goal_rx > 1 && (!adapter->multi_queue || adapter->mq_fallback)) {
netdev_err(netdev,
"Cannot resize to %u RX queues: multi-queue mode not
supported by firmware\n",
goal_rx);
return -EOPNOTSUPP;
}
With mq_fallback set and a live count of 8, ethtool -l reports max_rx 8 while
ethtool -L ethX rx 2 through rx 7 all fail with -EOPNOTSUPP; only 1 and the
current count are accepted.
This state looks persistent rather than transient, since
ibmveth_register_rx_queues() latches the flag and then fails the open:
if (rc) {
/* Firmware MQ gone: fall back to SQ on next open. */
if (rc == -EOPNOTSUPP)
adapter->mq_fallback = true;
goto err_unregister;
}
adapter->opened stays false and num_rx_queues keeps its old value above 1
until the next successful open.
Would it be clearer to advertise max_rx = 1 in the fallback state and keep
the no-op shortcut for the read-modify-write case? The kernel-doc for
ibmveth_resize_rx_channels() ("Rejects rx > 1 without MQ firmware
(-EOPNOTSUPP)") also does not mention that the no-op shortcut precedes the
gate.
get_channels reports the live count and
caps max_rx at that count once mq_fallback.
Advertising max_rx = 1 while rx_count is
still live fails the core (rx_count >
max_rx) and blocks a TX-only ethtool -L.
Clamping rx_count would shrink RX. rx > 1
is -EOPNOTSUPP except the current-count
no-op.
[Severity: Low]
Is the read of adapter->mq_fallback here synchronized against its writer?
The flag is stored from softirq/NAPI context in ibmveth_replenish_task(),
after the replenish_lock has already been dropped:
spin_unlock_irqrestore(&rxq->replenish_lock, flags);
...
adapter->mq_fallback = true;
schedule_work(&adapter->work);
The new reader added here, and the capability gate in
ibmveth_resize_rx_channels():
if (goal_rx > 1 && (!adapter->multi_queue || adapter->mq_fallback)) {
run under RTNL / the netdev ops lock, which does not exclude the softirq
writer. There is no lock, no READ_ONCE()/WRITE_ONCE() and no acquire/release
pairing on this field, while the sibling field num_rx_queues in the same
struct is deliberately published with:
smp_store_release(&adapter->num_rx_queues, num);
A stale false read here would advertise max_rx = IBMVETH_MAX_RX_QUEUES right
after firmware refused MQ buffer adds, and a stale read in the gate would let
the driver allocate and replenish RX queues on that firmware, re-latching
mq_fallback and queueing another reset. Would WRITE_ONCE()/READ_ONCE() on
mq_fallback, or folding it into the existing publish/consume helpers, be
appropriate?
No — RTNL does not exclude the NAPI store, so
this is an unannotated race. v7 keeps it as a
benign sticky latch: a stale false is one extra
MQ attempt, then the flag latches again.
WRITE_ONCE on the store without READ_ONCE on
apply / get_channels / set_channels is only half
a pass, so I left it plain. Happy to annotate
the store and all three readers together if you
would prefer that spelled out.
@@ -3233,28 +3242,83 @@ static int ibmveth_set_channels(struct net_device
*netdev,
struct ethtool_channels *channels)
{
struct ibmveth_adapter *adapter = netdev_priv(netdev);
- unsigned int old = netdev->real_num_tx_queues,
- goal = channels->tx_count;
+ unsigned int old_rx = ibmveth_get_num_rx_queues(adapter);
+ unsigned int goal_rx = channels->rx_count;
+ unsigned int old_tx = netdev->real_num_tx_queues;
+ unsigned int goal_tx = channels->tx_count;
+ unsigned int want_tx = goal_tx;
+ bool rx_changed = false;
int rc, i;
- /* Validate RX (and resize when opened) before the down-path
- * early return so MQ/range errors are reported here. Publishing
- * the desired RX count and CMO while down is the next patch.
- */
- rc = ibmveth_resize_rx_channels(adapter, channels->rx_count);
+ if (goal_tx < 1 || goal_tx > ibmveth_real_max_tx_queues()) {
+ netdev_err(netdev,
+ "Invalid TX queue count %u (must be 1-%u)\n",
+ goal_tx, ibmveth_real_max_tx_queues());
+ return -EINVAL;
+ }
+
+ /* RX range / MQ checks live in ibmveth_resize_rx_channels(). */
+ rc = ibmveth_resize_rx_channels(adapter, goal_rx);
if (rc)
return rc;
[ ... down-path publish/rollback block snipped ... ]
- return rc;
+ if (netdev->real_num_tx_queues != want_tx) {
+ if (rx_changed) {
+ /*
+ * Only meaningful once RX is live. num_slots is
+ * embedded in the adapter and outlives the DMA ring,
+ * so reading it at function entry is safe but can
+ * return a stale geometry from before the resize.
+ */
+ int rxq_entries = adapter->rx_queue[0].num_slots;
[Severity: Low]
This isn't a bug, but does this comment still describe the code below it?
It says num_slots is read "at function entry" and "can return a stale
geometry from before the resize", yet the read it annotates happens right
here, inside the if (rx_changed) block, after ibmveth_resize_rx_channels()
has already changed the RX geometry. Function entry only reads
num_rx_queues, real_num_tx_queues, channels->rx_count and channels->tx_count.
The comment looks like a leftover from the v5 layout mentioned in the
changelog, and now warns about a hazard that would only exist if the read
were moved back to function entry.
The comment was leftover from the earlier
layout. The read is after the RX resize.
v7 replaces it to match that restore.
+ int rb;
+
+ rb = ibmveth_resize_rx_queues_incremental(adapter,
+ old_rx,
+ rxq_entries);
+ if (rb)
+ netdev_err(netdev,
+ "Failed to roll back RX queues to %u
after TX failure: %d\n",
+ old_rx, rb);
+ }
+ return rc ? rc : -ENOMEM;
+ }
[Severity: Low]
Can this rollback re-attempt a queue geometry that the forward path would
have refused?
The rollback calls ibmveth_resize_rx_queues_incremental() directly, so the
multi_queue / mq_fallback gate in ibmveth_resize_rx_channels() is not
consulted. That gate only rejects goal_rx > 1:
if (goal_rx > 1 && (!adapter->multi_queue || adapter->mq_fallback)) {
...
return -EOPNOTSUPP;
}
Consider adapter->opened true, mq_fallback already latched from softirq in
ibmveth_replenish_task():
adapter->mq_fallback = true;
schedule_work(&adapter->work);
and a live RX count of 8. A request of rx 1 with a larger tx passes the gate
(goal_rx is 1), RX shrinks 8 -> 1 and rx_changed becomes true. If the TX
step then fails in ibmveth_allocate_tx_ltb() or
netif_set_real_num_tx_queues(), the rollback runs the scale-up path back to
old_rx = 8 on firmware that has already refused MQ buffer adds, so
H_REG_LOGICAL_LAN_QUEUE / replenish hit the same H_FUNCTION, mq_fallback is
re-latched and another schedule_work(&adapter->work) reset is queued from an
ethtool error path.
Would it be better to route the rollback through
ibmveth_resize_rx_channels(), or to skip it when mq_fallback is set and leave
RX at 1?
[ ... ]
The rollback restores the count we just
left. If firmware already refused MQ, that
scale-up can fail and latch again. !opened
is the documented down path and does not
allocate.
Thanks,
Mingming
