Re: [PATCH net-next v3 07/15] ibmveth: Add RX queue register/deregister helpers for MQ

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

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.
>
>> 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
> [ ... ]
>> @@ -401,6 +401,28 @@ ibmveth_dispose_subordinate_irq_mappings(struct ibmveth_adapter *adapter)
> [ ... ]
>> +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.

*

>> @@ -1136,6 +1176,293 @@ static void __maybe_unused ibmveth_free_rx_qstats(struct ibmveth_adapter *adapte
> [ ... ]
>> +static int
>> +ibmveth_register_logical_lan_queue(struct ibmveth_adapter *adapter,
>> +				   union ibmveth_buf_desc rxq_desc,
>> +				   int queue_index)
>> +{
> [ ... ]
>> +	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.

*
>> +	}
>> +
>> +	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.

*
>> +	}
> [ ... ]
>> +static int
>> +ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
>> +{
> [ ... ]
>> +	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

*

>
>> +			goto err_unregister;
>> +		}
>> +	}