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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 3 · Low: 2

- [Medium] A user-triggerable rtnl hang fix is bundled into a 400-line
  refactor with an incorrect justification for omitting a Fixes: tag.
- [Medium] ibmveth_poll() re-arms the PHYP interrupt
  (ibmveth_enable_irq(), ibmveth.c:2123) only *after*…
- [Medium] Teardown frees Linux IRQ handlers (and, on the MQ paths,
  disposes subordinate virq mappings) without ensuring PHYP delivery was…
- [Low] Two permanent behavioural changes to ibmveth_close() are absent
  from the commit message: (a) ibmveth_update_rx_no_buffer() is moved…
- [Low] Claimed: ibmveth_schedule_rx_queue()'s kernel-doc documents an
  out-of-range index as an ordinary `return false`, while the body…

Pre-existing issues:
- [High] Claimed: on the ibmveth_alloc_buffer_pools() failure path,
  ibmveth_open() jumps to out_free_tx_ltb without h_free_logical_lan(),…

--

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(), the IRQ handler and the process-context RX kick sites. It also adds
two new adapter flags, opened and rx_irq_setup, that gate close/cleanup
work. 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 37a6d13e603ea..335712faaa42a 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -338,6 +340,320 @@ ibmveth_cleanup_rx_resources(struct ibmveth_adapter *adapter)
>  	}
>  }
>  
> +/**
> + * ibmveth_toggle_irq - Common helper to enable/disable queue interrupts

[ ... ]

> +		/*
> +		 * 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 (h_rc) {
> +		dev_err_ratelimited(&adapter->netdev->dev,
> +				    "Failed to %s IRQ for queue %d, rc=0x%lx\n",
> +				    action, queue_index, h_rc);
> +		return -EIO;
> +	}
> +	return 0;
> +}

[ ... ]

> +static int
> +ibmveth_setup_rx_interrupts(struct ibmveth_adapter *adapter)
> +{

[ ... ]

> +	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);
> +				for (; i >= 0; i--) {
> +					ibmveth_disable_irq(adapter, i);
> +					synchronize_irq(adapter->queue_irq[i]);
> +				}
> +				rc = -EIO;
> +				goto err_disable_napi;
> +			}
> +		}
> +	}

[ ... ]

> +err_disable_napi:
> +	/* STOP: remask after napi_disable; an in-flight poll can re-arm. */
> +	for (i = 0; i < num; i++)
> +		napi_disable(&adapter->napi[i]);
> +	for (i = 0; i < num; i++) {
> +		if (!adapter->queue_irq[i])
> +			continue;
> +		ibmveth_disable_irq(adapter, i);
> +		synchronize_irq(adapter->queue_irq[i]);
> +	}
> +	for (i = 0; i < num; i++) {
> +		if (adapter->queue_irq[i])
> +			free_irq(adapter->queue_irq[i], &adapter->napi[i]);
> +	}
> +	goto err_dispose_mappings;

[ ... ]

> +static void
> +ibmveth_cleanup_rx_interrupts(struct ibmveth_adapter *adapter)
> +{
> +	int i;
> +
> +	if (!adapter->rx_irq_setup)
> +		return;
> +
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		if (!adapter->queue_irq[i])
> +			continue;
> +		ibmveth_disable_irq(adapter, i);
> +		synchronize_irq(adapter->queue_irq[i]);
> +	}
> +
> +	for (i = 0; i < adapter->num_rx_queues; i++)
> +		napi_disable(&adapter->napi[i]);
> +
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		if (!adapter->queue_irq[i])
> +			continue;
> +		ibmveth_disable_irq(adapter, i);
> +		synchronize_irq(adapter->queue_irq[i]);
> +	}
> +
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		if (adapter->queue_irq[i])
> +			free_irq(adapter->queue_irq[i], &adapter->napi[i]);
> +	}

[Severity: Medium]
The commit message says cleanup "remasks and synchronizes again after it
because an in-flight poll can re-arm". Does the second remask really close
that window?

ibmveth_poll() re-arms only after NAPI ownership has already been dropped:

	if (!napi_complete_done(napi, frames_processed))
		goto out;
	...
	rc = ibmveth_enable_irq(adapter, 0);

napi_disable() waits for NAPIF_STATE_SCHED / NAPIF_STATE_NPSVC to clear, not
for the poll function to return, so this interleaving looks possible:

CPU1 (softirq)                    CPU0 (ibmveth_close)
ibmveth_poll()
  napi_complete_done()
    /* SCHED cleared */
  <delayed>
                                  ibmveth_cleanup_rx_interrupts()
                                    disable_irq + synchronize_irq
                                    napi_disable()  /* returns at once */
                                    disable_irq + synchronize_irq
                                    free_irq()
  ibmveth_enable_irq(adapter, 0)

That leaves PHYP delivery unmasked on a queue with no handler installed
until h_free_logical_lan() runs. The synchronize_net() added in close()
runs after free_irq(), so it cannot cover this either, and the
err_disable_napi path above has the same shape.

The ordering is unchanged at the end of the series; the poll_stopping()
check added later is passed by CPU1 before CPU0 sets the teardown state.
Would arming the interrupt before napi_complete_done(), or gating the arm
on a teardown flag ordered with the mask, work better here?

[Severity: Medium]
Both ibmveth_disable_irq() calls in this function discard their return
value, and free_irq() below then runs unconditionally. In close(),
h_free_logical_lan() now runs after free_irq(), where the baseline freed
the LAN first and only then removed the handler.

If the mask hcall did not take effect, can PHYP deliver an interrupt in the
interval between free_irq() and h_free_logical_lan(), with no handler
installed? synchronize_irq() only drains a handler that is already running;
it does not stop the hypervisor source.

Related: ibmveth_toggle_irq() folds H_PARAMETER into success on the disable
path, while its own comment says H_PARAMETER may mean bad arguments rather
than "already in requested state", so teardown can treat a rejected mask as
a completed one. The MQ rollback loop in ibmveth_setup_rx_interrupts() has
the same unchecked-remask-then-free_irq pattern. I could not demonstrate
these hcalls failing in practice, so this one may be theoretical.

> +
> +	ibmveth_dispose_subordinate_irq_mappings(adapter);
> +
> +	/* Queue 0 uses netdev->irq; leave queue_irq[0] for next open. */
> +	adapter->rx_irq_setup = false;
> +}
> +
> +/**
> + * 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_schedule_prep() succeeded and NAPI was scheduled.
> + * Mask is attempted in that case; a failed disable_irq() is logged by the
> + * helper and does not change the return (queue may still be unmasked).
> + * false if the index is out of range or prep failed (including NAPI
> + * already scheduled).
> + */
> +static bool ibmveth_schedule_rx_queue(struct ibmveth_adapter *adapter,
> +				      int qindex)
> +{
> +	struct napi_struct *napi = &adapter->napi[qindex];
> +
> +	if (WARN_ON(qindex < 0 || qindex >= adapter->num_rx_queues))
> +		return false;

[Severity: Low]
This isn't a bug, but the kdoc and the body disagree. The kdoc describes an
out-of-range index as an ordinary return:

	 * false if the index is out of range or prep failed (including NAPI
	 * already scheduled).

while the body splats on it. ibmveth_interrupt() documents the opposite for
the same condition:

	/*
	 * Quiet on out-of-range: teardown can leave a residual IRQ after the
	 * live count drops. Do not WARN-storm; return IRQ_NONE until free_irq.
	 */

A later patch in the series changes this helper to WARN only on qindex < 0
and to return false quietly when qindex is past the live queue count. Could
that form be used from the start?

The napi pointer is also formed from qindex before the range check; it is
never dereferenced before the check, but computing it afterwards would read
more clearly.

> +
> +	/*
> +	 * Only mask PHYP when NAPI will run. Masking on prep failure can
> +	 * race a completing poll that already re-enabled the queue, leaving
> +	 * NAPI idle with the IRQ masked (TX works, RX stalls) until reload.
> +	 * Storm prevention on teardown remains in cleanup/disable paths.
> +	 */
> +	if (napi_schedule_prep(napi)) {
> +		/* Failure is already logged with the hcall rc by the helper. */
> +		ibmveth_disable_irq(adapter, qindex);
> +		__napi_schedule(napi);
> +		return true;
> +	}
> +	return false;
> +}
> +

[ ... ]

> @@ -1000,24 +1315,20 @@ static int ibmveth_open(struct net_device *netdev)
>  	if (rc)
>  		goto out_free_tx_ltb;
>  

[Severity: High]
This isn't a bug introduced by this patch, but the buffer-pool allocation
failure just above (ibmveth_alloc_buffer_pools() -> goto out_free_tx_ltb)
still skips h_free_logical_lan(), unlike the setup_rx_interrupts() failure
right below it.

The out_free_tx_ltb path continues into ibmveth_cleanup_rx_resources(),
which unmaps and frees the buffer list, the filter list and the RX ring
while the logical LAN registration is still live, so PHYP keeps DMA
addresses for freed pages.

Does the new opened gate in ibmveth_close() also remove the last site that
could have deregistered that residue? A later ndo_stop now returns 0
without calling h_free_logical_lan().

Two later patches in this series rework this ladder, first routing the pool
failure through a path that loops on h_free_logical_lan() and then moving
pool allocation ahead of registration, so the window does not survive the
series. Would it be worth ordering those ahead of this one so this commit
does not carry it?

> -	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;
>  	}
>  
>  	netdev_dbg(netdev, "initial replenish cycle\n");
> -	ibmveth_interrupt(netdev->irq, netdev);
> +	ibmveth_schedule_rx_queue(adapter, 0);
>  
>  	netif_tx_start_all_queues(netdev);
>  
> +	adapter->opened = true;
>  	netdev_dbg(netdev, "open complete\n");
>  
>  	return 0;

[ ... ]

> @@ -1041,27 +1351,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;

[Severity: Medium]
The commit message says:

  "Opened / rx_irq_setup also closes a pre-existing hang: after a
   failed reopen, a later ndo_stop used to napi_disable and free_irq
   a second time (rtnl spin + already-free IRQ). That depends on the
   helpers in this patch, so there is no standalone Fixes: tag."

Is that dependency real? This gate reads only adapter->opened, and the
matching "adapter->opened = true" at the end of ibmveth_open() does not
reference ibmveth_toggle_irq(), ibmveth_setup_rx_interrupts() or
ibmveth_cleanup_rx_interrupts() either.

The hang itself is reachable in the baseline: ibmveth_open() calls
napi_enable() on entry and napi_disable() on its out: path, and
ibmveth_close() starts with napi_disable(). veth_pool_store(),
ibmveth_change_mtu(), ibmveth_set_csum_offload() and ibmveth_set_tso() call
ibmveth_close() then ibmveth_open() directly and do not clear IFF_UP when
the reopen fails, so a later "ip link set dev X down" enters
ibmveth_close() and the second napi_disable() spin-waits forever with rtnl
held.

Could the opened flag and this gate be split into their own patch with a
Fixes: tag, so the fix can be backported without the refactor?

>  
> -	napi_disable(&adapter->napi[0]);
> +	adapter->opened = false;
> +
> +	netdev_dbg(netdev, "close starting\n");
>  
>  	netif_tx_stop_all_queues(netdev);
>  
> -	h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
> +	ibmveth_cleanup_rx_interrupts(adapter);
> +	/* Wait for softirq/poll that already passed shutdown checks. */
> +	synchronize_net();
>  
> +	ibmveth_update_rx_no_buffer(adapter);
> +	/* Full LAN teardown (subordinates arrive with register helpers). */
>  	do {
>  		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>  	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));

[Severity: Low]
Two permanent changes to ibmveth_close() are not described in the commit
message.

ibmveth_update_rx_no_buffer() moves from after h_free_logical_lan() and
free_irq() to before h_free_logical_lan(). It reads the absolute counter
that the hypervisor writes at the end of the buffer list page, and the
hypervisor still owns that page until h_free_logical_lan() returns, so the
final rx_no_buffer / rx_errors value can now miss drops recorded after the
read. Was the reordering intentional?

The new synchronize_net() in the ndo_stop path is mentioned only in the v5
changelog below the --- separator, which does not land in git history.
Could it be described in the commit message as well?

> -
>  	if (lpar_rc != H_SUCCESS) {
> -		netdev_err(netdev, "h_free_logical_lan failed with %lx, "
> -			   "continuing with close\n", lpar_rc);
> +		netdev_err(adapter->netdev,
> +			   "h_free_logical_lan failed with %lx, continuing\n",
> +			   lpar_rc);
>  	}
> -
> -	free_irq(netdev->irq, netdev);
> -
> -	ibmveth_update_rx_no_buffer(adapter);
> -
>  	ibmveth_free_buffer_pools(adapter);
>  	ibmveth_cleanup_rx_resources(adapter);
>  	ibmveth_free_filter_list(adapter);

[ ... ]

> @@ -1805,15 +2120,14 @@ 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 (rc) {
>  		schedule_work(&adapter->work);
>  		goto out;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788102125.git.mmc%40linux.ibm.com