Re: [PATCH net-next v5 15/15] ibmveth: Wire ethtool set_channels to MQ RX queue resize

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: Wire ethtool set_channels to MQ RX queue resize

This completes the RX side of ibmveth_set_channels(): while the device is
down the desired RX count is stashed, and while it is up RX is resized
first and the TX LTB stop/alloc/set_real_num_tx/free/wake path runs
afterwards.  It also adds up-front TX range validation and an RX rollback
when TX cannot reach the requested count.

A few questions on the new paths below.

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but the bound used here is
CPU-count dependent while the reported current value is not, so the two can
disagree.

ibmveth_get_channels() fills the reply from two different sources:

	channels->max_tx = ibmveth_real_max_tx_queues();
	channels->tx_count = netdev->real_num_tx_queues;

and ibmveth_real_max_tx_queues() is min(num_online_cpus(),
IBMVETH_MAX_QUEUES).  After configuring N TX queues and then offlining CPUs
so that num_online_cpus() < N, the reply advertises tx_count > max_tx.

Both ethtool entry points reject a request whose tx_count exceeds the
driver-advertised max_tx:

net/ethtool/ioctl.c:ethtool_set_channels() {
	...
	if (channels.rx_count > curr.max_rx ||
	    channels.tx_count > curr.max_tx || ...)
		return -EINVAL;
	...
}

so even an RX-only change (which resubmits the current tx_count) fails with
-EINVAL until the CPUs come back online.  Should max_tx be latched from the
configured count instead, or should the new range check be against
IBMVETH_MAX_QUEUES?

+
+	/* RX range / MQ checks live in ibmveth_resize_rx_channels(). */
+	rc = ibmveth_resize_rx_channels(adapter, goal_rx);
 	if (rc)
 		return rc;
 
-	if (!adapter->opened)
-		return netif_set_real_num_tx_queues(netdev, goal);
+	/* If RX resources are not live (never opened, or close+open failed
+	 * while IFF_UP stayed set), only stash desired queue counts.
+	 */
+	if (!adapter->opened) {
+		/* Apply TX first so a failure leaves RX stash unchanged. */
+		rc = netif_set_real_num_tx_queues(netdev, goal_tx);
+		if (rc)
+			return rc;
+
+		/* Stash desired RX count; open() publishes it via
+		 * netif_set_real_num_rx_queues() after queue registration.
+		 * Refresh CMO now so open() can map the larger footprint;
+		 * open itself does not call vio_cmo_set_dev_desired.
+		 */

[Severity: Low]
Is this comment describing what the code does?  The block right below
publishes immediately rather than deferring to open():
ibmveth_publish_num_rx_queues() updates adapter->num_rx_queues and
netif_set_real_num_rx_queues() updates netdev->real_num_rx_queues, which
creates or removes the /sys/class/net/*/queues/rx-N entries while the
interface is down.  ibmveth_open() only re-applies the same value later:

	rc = netif_set_real_num_rx_queues(netdev,
					  ibmveth_get_num_rx_queues(adapter));

The commit message says the same thing:

    - When down: set TX queues first, then stash the desired RX count
      in adapter->num_rx_queues for the next open() (open publishes via
      netif_set_real_num_rx_queues).

Could the comment and the changelog mention that real_num_rx_queues, the
rx-N sysfs entries and (below) the CMO entitlement all change immediately
on a down interface?

+		if (goal_rx != ibmveth_get_num_rx_queues(adapter)) {
+			ibmveth_publish_num_rx_queues(adapter, goal_rx);

[Severity: Medium]
Can publishing a count with no allocated queues behind it let a lockless
reader take an uninitialized spinlock?

adapter->num_rx_queues is the only validity bound the RX readers use, and
rx_queue[i].replenish_lock is only spin_lock_init()'ed for queues that are
actually allocated (ibmveth_alloc_rx_queues() and
ibmveth_alloc_single_rx_queue()).

ibmveth_replenish_task() checks nothing else before locking:

	if (queue_index >= ibmveth_get_num_rx_queues(adapter)) {
		...
		return;
	}
	...
	spin_lock_irqsave(&rxq->replenish_lock, flags);

and ibmveth_poll_controller() has no adapter->opened check:

	unsigned int num = ibmveth_get_num_rx_queues(adapter);

	for (i = 0; i < num; i++)
		ibmveth_replenish_task(adapter, i);

That is reachable in the state this branch targets: pool_store and
change_mtu call ibmveth_close()+ibmveth_open() directly, so IFF_UP can stay
set (netif_running() true, netpoll still attached) with adapter->opened
false.  An "ethtool -L ethX rx N" with N above any previously allocated
count then publishes N here, and the window stays open for as long as the
device is down.

ibmveth_publish_num_rx_queues() itself states the invariant:

	/* Per-queue state must be visible before readers observe num. */
	smp_wmb();
	WRITE_ONCE(adapter->num_rx_queues, num);

Does this call site break it, given the per-queue state does not exist yet?

+			rc = netif_set_real_num_rx_queues(netdev, goal_rx);
+			if (rc) {
+				ibmveth_publish_num_rx_queues(adapter, old_rx);
+				return rc;
+			}

[Severity: Medium]
On this failure path, netdev->real_num_tx_queues was already moved to
goal_tx by the netif_set_real_num_tx_queues() call above, and only the RX
publish is undone.

netif_set_real_num_rx_queues() has a real failure path:

net/core/dev.c:netif_set_real_num_rx_queues() {
	...
	rc = net_rx_queue_update_kobjects(dev, dev->real_num_rx_queues, rxq);
	if (rc)
		return rc;
	...
}

so ethtool can report failure while the TX count has permanently changed.
Should netif_set_real_num_tx_queues(netdev, old_tx) run here before
returning rc, and does the current code match the commit message claim
"Make TX/RX updates fail atomically"?

+			if (firmware_has_feature(FW_FEATURE_CMO)) {
+				unsigned long dma;
+
+				dma = ibmveth_get_desired_dma(adapter->vdev);
+				vio_cmo_set_dev_desired(adapter->vdev, dma);
+			}

[Severity: Medium]
Is the entitlement raised here ever lowered again if the stashed count is
never realized?  The desired value is sized for goal_rx RX queues, but no
buffer lists, queue DMA or pools are mapped while down.

If the next open() fails for the larger count, or if it applies the MQ
fallback and drops the count back to 1, neither ibmveth_open(),
ibmveth_close() nor ibmveth_apply_mq_fallback() recomputes desired DMA, so
the partition keeps CMO entitlement reserved for RX resources that do not
exist until a later successful set_channels or change_mtu.

+		}
+		return 0;
+	}

[Severity: Medium]
Can a request that returns 0 from this branch be silently discarded on the
next open()?

If firmware rejected MQ during a previous ibmveth_register_rx_queues(),
adapter->mq_fallback is latched while adapter->multi_queue is still
non-zero, so "ethtool -L ethX rx 8" passes the MQ check in
ibmveth_resize_rx_channels(), takes this !opened branch, publishes 8, sets
real_num_rx_queues = 8, raises CMO desired and returns success.

ibmveth_apply_mq_fallback() then runs first in ibmveth_open():

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

with no error back to the user and no entitlement recompute, and
ibmveth_get_channels() afterwards reports:

	channels->rx_count = ibmveth_get_num_rx_queues(adapter);

which is 1, contradicting the request that returned 0.  Should the down
path reject (or at least warn about) goal_rx > 1 when mq_fallback is
latched?

+
+	if (goal_rx != old_rx)
+		rx_changed = true;
 
 	/* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
 	 * but we may need to alloc/free the ltb's.
 	 */
+	if (goal_tx == old_tx)
+		return 0;
+
 	netif_tx_stop_all_queues(netdev);
 
-	/* Allocate any queue that we need */
-	for (i = old; i < goal; i++) {
+	/* Allocate any new TX LTBs. i starts at old_tx for the free walk
+	 * below when this loop body never runs (goal_tx == old_tx already
+	 * returned; goal_tx < old_tx is scale-down).
+	 */
+	i = old_tx;
+	for (; i < goal_tx; i++) {

[Severity: Low]
The commit message says:

    Initialize the TX alloc loop bound so a scale-down path that never
    allocates still has defined cleanup if set_real_num_tx_queues() fails.

Was there anything undefined before?  The removed line is:

	for (i = old; i < goal; i++) {

whose initializer runs even when the body does not (old >= goal), so i was
already defined for the later "for (i = old; i > goal; i--)" free walk.
Could this be described as a readability change rather than a fix?

[ ... ]

 	netif_tx_wake_all_queues(netdev);
 
-	return rc;
+	if (netdev->real_num_tx_queues != want_tx) {
+		if (rx_changed) {
+			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: Medium]
This rollback is best effort only, so the same "fail atomically" question
applies to the up path.  For "ethtool -L ethX rx <smaller> tx <larger>",
the RX scale-down has already destroyed queues; if
ibmveth_allocate_tx_ltb() then fails, the rollback here is a scale-up whose
own steps can fail too:

	rc = ibmveth_alloc_single_rx_queue(adapter, i, rxq_entries);
	if (rc) { ... goto cleanup_new_queues; }

and the same for ibmveth_register_single_rx_queue(),
ibmveth_setup_single_rx_interrupt(), ibmveth_enable_irq() and
netif_set_real_num_rx_queues().  Its cleanup path leaves RX at the reduced
count, and here that is only logged before returning an error.

Is there a way to order this so the destructive RX change happens only
after the TX LTB allocations have succeeded, so no partial state can be
left behind when the call reports failure?

 }
 
 static const struct ethtool_ops netdev_ethtool_ops = {