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

[Simon Horman <horms@kernel.org>]

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?

[ ... ]
> @@ -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.

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

> +	}
[ ... ]
> +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.

> +			goto err_unregister;
> +		}
> +	}