Re: [PATCH net-next v5 05/15] ibmveth: Refactor RX interrupt control for MQ RX queues

[Jakub Kicinski <kuba@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.
---
ibmveth: Refactor RX interrupt control for MQ RX queues

This patch introduces queue-indexed RX interrupt helpers (toggle/enable/
disable, setup/cleanup, schedule) and wires them into open(), close(),
poll() and the IRQ handler, with request_irq() now using &adapter->napi[i]
as the per-queue cookie. Runtime behaviour stays single-queue.

A few questions below.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 1a428ee60d58c..99eeb6ef51bf3 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -331,6 +333,311 @@ ibmveth_cleanup_rx_resources(struct ibmveth_adapter *adapter)

[ ... ]

> +		/*
> +		 * H_PARAMETER is ambiguous (already in requested state vs bad
> +		 * args). Fold only on disable as an idempotent mask. On enable
> +		 * keep it an error so a stuck-masked queue stays visible to
> +		 * poll/resize recovery.
> +		 */
> +		if (h_rc == H_PARAMETER && !enable) {
> +			dev_warn_ratelimited(&adapter->netdev->dev,
> +					     "H_VIOCTL %s IRQ returned H_PARAMETER for queue %d (hwirq=%lu)\n",
> +					     action, queue_index, hwirq);
> +			return 0;
> +		}

[ ... ]

> +	if (adapter->multi_queue && num > 1) {
> +		for (i = 0; i < num; i++) {
> +			rc = ibmveth_enable_irq(adapter, i);
> +			if (rc) {
> +				netdev_err(netdev,
> +					   "Failed to enable IRQ for queue %d, rc=%d\n",
> +					   i, rc);
> +				while (--i >= 0) {
> +					ibmveth_disable_irq(adapter, i);
> +					synchronize_irq(adapter->queue_irq[i]);
> +				}
> +				rc = -EIO;
> +				goto err_disable_napi;
> +			}
> +		}
> +	}

[ ... ]

> +err_disable_napi:
> +	/* PHYP unmask was rolled back above; disable NAPI before free_irq */
> +	for (i = 0; i < num; i++)
> +		napi_disable(&adapter->napi[i]);
> +	for (i = 0; i < num; i++) {
> +		if (adapter->queue_irq[i])
> +			free_irq(adapter->queue_irq[i], &adapter->napi[i]);
> +	}

[Severity: Medium]
In ibmveth_setup_rx_interrupts(), when ibmveth_enable_irq() fails for queue
i the rollback loop begins with --i, so queue i itself is never masked, yet
err_disable_napi then napi_disable()s and free_irq()s that same queue.

Is queue i guaranteed to be masked at that point?  ibmveth_toggle_irq()
folds H_PARAMETER to success only on disable, and its own comment says
H_PARAMETER may mean "already in requested state":

	if (h_rc == H_PARAMETER && !enable) {

so an enable can report an error for a queue PHYP has already unmasked.

While napi_disable() sleeps, an interrupt on that still-unmasked queue
reaches ibmveth_interrupt() -> ibmveth_schedule_rx_queue(), where
napi_schedule_prep() fails and the helper deliberately does not mask:

	if (napi_schedule_prep(napi)) {
		rc = ibmveth_disable_irq(adapter, qindex);
		WARN_ON(rc);
		__napi_schedule(napi);
		return true;
	}
	return false;

The handler still returns IRQ_HANDLED, so nothing quiets the source.  Can
this leave the PHYP interrupt asserted and re-entering the handler for the
duration of the napi_disable() wait, which is the opposite of the "mask
PHYP and synchronize_irq before napi_disable" rule stated in the changelog?

The multi_queue && num > 1 branch cannot run at this commit because
multi_queue is hardcoded to 0, but it becomes live later in the series and
the rollback loop is unchanged at the end of the series.

[ ... ]

> +/**
> + * ibmveth_schedule_rx_queue - Mask PHYP IRQ and schedule NAPI for one RX queue
> + * @adapter: ibmveth adapter structure
> + * @qindex: RX queue index
> + *
> + * Shared by the IRQ handler and process-context kick sites (open, resume,
> + * pool sysfs, poll_controller).
> + *
> + * Return: true if NAPI was scheduled (and PHYP masked), false if prep failed
> + */
> +static bool ibmveth_schedule_rx_queue(struct ibmveth_adapter *adapter,
> +				      int qindex)
> +{
> +	struct napi_struct *napi = &adapter->napi[qindex];
> +	int rc;
> +
> +	if (WARN_ON(qindex < 0 || qindex >= adapter->num_rx_queues))
> +		return false;

[Severity: Low]
This isn't a bug, but the documented return contract does not match the
code in two ways.

The out-of-range WARN_ON branch also returns false, which is not a prep
failure, so callers cannot distinguish the two cases.

And "true ... (and PHYP masked)" is not established, since the return of
ibmveth_disable_irq() is only passed to WARN_ON() and then discarded:

	if (napi_schedule_prep(napi)) {
		rc = ibmveth_disable_irq(adapter, qindex);
		WARN_ON(rc);
		__napi_schedule(napi);
		return true;
	}

so when the mask hypercall fails the helper still reports true while the
queue remains unmasked.  Should the kernel-doc be reworded, or should the
mask failure be reflected in the return value for callers like open(),
resume(), veth_pool_store() and the later ibmveth_restart_rx_queue()?

[ ... ]

> @@ -991,24 +1297,20 @@ static int ibmveth_open(struct net_device *netdev)
>  	if (rc)
>  		goto out_free_tx_ltb;
>  
> -	netdev_dbg(netdev, "registering irq 0x%x\n", netdev->irq);
> -	rc = request_irq(netdev->irq, ibmveth_interrupt, 0, netdev->name,
> -			 netdev);
> -	if (rc != 0) {
> -		netdev_err(netdev, "unable to request irq 0x%x, rc %d\n",
> -			   netdev->irq, rc);
> +	rc = ibmveth_setup_rx_interrupts(adapter);
> +	if (rc) {
>  		do {
>  			lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>  		} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
> -
>  		goto out_free_buffer_pools;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but the ordering here is worth a
question now that close() gains an early return.

ibmveth_open() registers the logical LAN before allocating buffer pools, and
the pool allocation failure branch takes goto out_free_tx_ltb, which runs:

out_free_tx_ltb:
	while (--i >= 0)
		ibmveth_free_tx_ltb(adapter, i);
	ibmveth_cleanup_rx_resources(adapter);
out_free_filter_list:
	ibmveth_free_filter_list(adapter);

That dma_unmaps buffer_list_dma[0], dma_free_coherent()s the RX ring and
frees the filter list page without ever calling h_free_logical_lan().  Does
this leave PHYP registered against the buffer list, filter list and RX
descriptor ring after those pages have been unmapped and returned to the
page allocator?

The same ordering exists in the baseline at 4f93b12cf7b2, so the root
problem predates this patch.  What changes here is that the new
"if (!adapter->opened) return 0;" gate in ibmveth_close() also removes the
accidental recovery a later ndo_stop used to provide by running the
h_free_logical_lan() loop.

For reference, this is resolved by the end of the series: open() there
allocates the buffer pools before ibmveth_register_rx_queues(), and every
post-registration error path funnels into ibmveth_free_all_queues(), which
issues h_free_logical_lan() in a retry loop.

> @@ -1032,27 +1333,32 @@ static int ibmveth_close(struct net_device *netdev)
>  	long lpar_rc;
>  	int i;
>  
> -	netdev_dbg(netdev, "close starting\n");
> +	/* Gate on opened, not IFF_UP: pool_store/change_mtu close+open can
> +	 * leave IFF_UP set after a failed reopen.
> +	 */
> +	if (!adapter->opened)
> +		return 0;
>  
> -	napi_disable(&adapter->napi[0]);
> +	adapter->opened = false;

[Severity: Low]
This is a pre-existing issue rather than something introduced here, but
should this gate carry a Fixes: tag and a note in the changelog?

In the baseline, ibmveth_close() unconditionally ran
napi_disable(&adapter->napi[0]) and free_irq(netdev->irq, netdev).  After a
failed reopen from veth_pool_store() or ibmveth_change_mtu() (both call
close() then open() directly and leave IFF_UP set when open fails), a
subsequent "ip link set down" re-entered ndo_stop with NAPI already
disabled and no IRQ registered.  napi_disable_locked() leaves
NAPIF_STATE_SCHED|NPSVC set, so the second napi_disable() spins in its
usleep_range() loop while holding rtnl, and free_irq() splats "Trying to
free already-free IRQ".

The changelog describes the mechanism ("opened / rx_irq_setup gate whether
cleanup walks IRQ/NAPI state") and the v5 notes below the --- describe the
scenario, but the commit message itself never says a hang is being fixed
and there is no Fixes: tag, so the fix is hard to identify for backports.
It also cannot be applied on its own since it depends on the new helpers,
flags and queue_irq[] array added here.

[Severity: Medium]
Separately, can this early return skip TX long-term-buffer teardown?  At
this commit ibmveth_set_channels() keys its allocation decision off IFF_UP:

	if (!(netdev->flags & IFF_UP))
		return netif_set_real_num_tx_queues(netdev, goal);

so after a failed reopen (IFF_UP still set, adapter->opened false),
"ethtool -L eth0 tx N" takes the allocating branch while the adapter is
closed.  The next ndo_stop then returns early and never reaches:

	for (i = 0; i < netdev->real_num_tx_queues; i++)
		ibmveth_free_tx_ltb(adapter, i);

and the next ibmveth_open() assigns tx_ltb_ptr[i]/tx_ltb_dma[i]
unconditionally.  Does that leak the bounce buffer and its DMA mapping per
TX queue?

By the end of the series ibmveth_set_channels() gates on
"if (!adapter->opened)" and only stashes queue counts, so no path allocates
TX LTBs while the adapter is closed, which removes this window.

[ ... ]

> @@ -1696,7 +2002,7 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  			container_of(napi, struct ibmveth_adapter, napi[0]);
>  	struct net_device *netdev = adapter->netdev;
>  	int frames_processed = 0;
> -	unsigned long lpar_rc;
> +	int rc;
>  	u16 mss = 0;
>  
>  restart_poll:

[ ... ]

> @@ -1796,15 +2102,15 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>  	/* We think we are done - reenable interrupts,
>  	 * then check once more to make sure we are done.
>  	 */
> -	lpar_rc = h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_ENABLE);
> -	if (WARN_ON(lpar_rc != H_SUCCESS)) {
> +	rc = ibmveth_enable_irq(adapter, 0);
> +	if (WARN_ON(rc)) {
>  		schedule_work(&adapter->work);
>  		goto out;
>  	}
>  
>  	if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule(napi)) {
> -		lpar_rc = h_vio_signal(adapter->vdev->unit_address,
> -				       VIO_IRQ_DISABLE);
> +		rc = ibmveth_disable_irq(adapter, 0);
> +		WARN_ON(rc);
>  		goto restart_poll;
>  	}

[Severity: Low]
This isn't a bug today, but the two halves of the new per-queue protocol
disagree.  ibmveth_interrupt() now resolves the queue that fired:

	qindex = napi - adapter->napi;
	...
	ibmveth_schedule_rx_queue(adapter, qindex);

while ibmveth_poll() still recovers the adapter through
container_of(napi, struct ibmveth_adapter, napi[0]) and hardcodes queue 0
in ibmveth_enable_irq(adapter, 0) / ibmveth_disable_irq(adapter, 0).  For a
napi index above 0 that container_of would compute a wrong adapter pointer
and the unmask would target queue 0 rather than the queue that fired.

Since IBMVETH_MAX_RX_QUEUES is 1 and multi_queue is 0 at this commit only
napi[0] can be registered, and later commits in the series ("ibmveth:
Harden RX poll path with helpers" / "ibmveth: Enable multi-queue RX receive
path") derive queue_index = napi - adapter->napi in poll() with a range
check.  Would it be clearer to convert poll() in the same patch that
introduces the queue-indexed handler, so the two sides never disagree?