On 7/14/26 5:43 AM, Simon Horman 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.
Full review at: https://sashiko.dev
---
ibmveth: Add RX queue register/deregister helpers for MQ

This patch introduces new helpers to handle RX queue registration and
deregistration for MQ support. It replaces the single adapter-level
register/free paradigm with per-queue register calls to the hypervisor,
fetching queue handles and IRQs. These new paths are only fully exercised
when multi-queue mode is enabled.

quoted

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 63b0184c622a4..7fc11a4e1f61d 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

quoted

@@ -401,6 +401,28 @@ ibmveth_dispose_subordinate_irq_mappings(struct ibmveth_adapter *adapter)

[ ... ]

quoted

+static int __maybe_unused
+ibmveth_setup_rx_interrupts(struct ibmveth_adapter *adapter)
+{
+	struct net_device *netdev = adapter->netdev;
+	int i, rc;
+
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		if (!adapter->queue_irq[i]) {
+			netdev_err(netdev, "queue %d has invalid IRQ (0)\n", i);
+			rc = -EINVAL;
+			goto err_free_irqs;
+		}
+
+		rc = request_irq(adapter->queue_irq[i], ibmveth_interrupt,
+				 0, netdev->name, &adapter->napi[i]);
+		if (rc) {
+			netdev_err(netdev,
+				   "request_irq() failed for irq 0x%x queue %d: %d\n",
+				   adapter->queue_irq[i], i, rc);
+			goto err_free_irqs;
+		}
+	}
+
+	for (i = 0; i < adapter->num_rx_queues; i++)
+		napi_enable(&adapter->napi[i]);

[Severity: Medium]
Could this sequence race with packet reception during interface bring-up?
By calling request_irq() before napi_enable(), the hardware interrupt is
unmasked while the NAPI state is still NAPI_STATE_DISABLE. If an interrupt
fires in this window, could ibmveth_interrupt() fail to schedule the NAPI
poll, potentially leading to dropped events or an interrupt storm?

[ ... ]

**

*On the request_irq vs napi_enable ordering: on open, ibmveth_enable_irq()*

*

(h_vio_signal / H_VIOCTL) runs only after napi_enable(), so PHYP should

not deliver in the request_irq→napi_enable window. That matches what

peers like ibmvnic do (NAPI before firmware unmask). I'll keep the

tested open order (request_irq → napi_enable → PHYP enable) in v4.


For teardown, v4 will follows the hard rule "mask PHYP before

napi_disable": scale-down does that, and close/open-fail cleanup does

too via cleanup_rx_interrupts(). Scale-up still does enable_irq before

napi_enable after publish+replenish — I'm leaving that order for this

v4 and can revisit as a follow-up if needed.

*

quoted

@@ -1136,6 +1176,293 @@ static void __maybe_unused ibmveth_free_rx_qstats(struct ibmveth_adapter *adapte

[ ... ]

quoted

+static int
+ibmveth_register_logical_lan_queue(struct ibmveth_adapter *adapter,
+				   union ibmveth_buf_desc rxq_desc,
+				   int queue_index)
+{

[ ... ]

quoted

+	if (lpar_rc == H_FUNCTION) {
+		if (adapter->multi_queue) {
+			netdev_info(adapter->netdev,
+				    "Multi queue mode not supported by firmware, falling back to single queue\n");
+			adapter->multi_queue = 0;
+		} else {
+			netdev_err(adapter->netdev,
+				   "Unexpected H_FUNCTION for queue %d registration (MQ mode already disabled)\n",
+				   queue_index);
+		}
+		return lpar_rc;

[Severity: Medium]
Does returning lpar_rc here cause a regression for the single queue fallback?
Since this error propagates up to ibmveth_register_rx_queues(), it seems it
will unconditionally jump to err_unregister and fail the interface open
rather than allowing it to continue with a single queue.

*

Good catch — clearing multi_queue still returned the

error and failed open, so the "falling back to single queue" log was

misleading. In v4 I'll treat subordinate H_FUNCTION as a hard error:

honest H_FUNCTION-specific netdev_err, then the existing generic logs,

without clearing multi_queue or claiming fallback.

*

quoted

+	}
+
+	if (try_again) {
+		try_again = 0;
+		goto retry;

[Severity: Low]
Does this retry loop provide any benefit? Unlike the primary queue which
calls h_free_logical_lan() before retrying to clear stale state, this
jumps back to retry the exact same hypercall with the exact same arguments,
which seems like it will immediately fail again.

*

The subordinate try_again path was indeed empty logic. Queue 0 retries

only after h_free_logical_lan() clears stale PHYP state (kexec). The

subqueue path retried the same H_REG_LOGICAL_LAN_QUEUE with the same

args and no free, so the second attempt could not succeed for any reason

we implemented — and H_FUNCTION already returned before the retry.

In v4 I'll drop that blind retry and report the failure once per queue.

*

quoted

+	}

[ ... ]

quoted

+static int
+ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
+{

[ ... ]

quoted

+	for (i = 1; i < adapter->num_rx_queues; i++) {
+		rc = ibmveth_register_single_rx_queue(adapter, i, mac_address);
+		if (rc) {
+			if (!adapter->queue_handle[i] || !adapter->queue_irq[i]) {
+				netdev_err(netdev,
+					   "Invalid hypervisor return for queue %d: handle=0x%llx irq=%u\n",
+					   i, adapter->queue_handle[i],
+					   adapter->queue_irq[i]);
+			}

[Severity: Low]
Is this validation check placed correctly? Because it sits inside the
if (rc) error block, it will log "Invalid hypervisor return" whenever
registration fails normally (e.g., due to resource constraints). It appears
the hypervisor is not expected to populate the handle and irq on failure.

**

*Good catch. on ordinary register failure handle/irq are still zero, so*

*

that caller message falsely claimed an invalid hypervisor return and

duplicated ibmveth_register_single_rx_queue() (which already logs the

hcall rc). In v4 I'll drop that extra log at the caller.


Thanks for the reviewing!

Mingming

*

quoted

+			goto err_unregister;
+		}
+	}