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

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

Hi, Thanks for the view.

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> 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.
No — not that window. napi_disable() only waits for
SCHED to clear, not for poll to return, so after
napi_complete_done() it can return at once. The
second remask then runs, and enable_irq can still
land after both remasks and after free_irq. PHYP
comes back up with no handler. synchronize_net()
is after free_irq, so it does not cover this
either. The second remask only catches a re-arm
that lands before it. v7 drops that overclaim from
the commit message.
> 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?
The arm stays after napi_complete_done. Moving it
before would re-enable PHYP while this poll still
owns NAPI.

Patch 9 adds a stopping check after complete
(napi_disable pending, or the device is down).
That skips enable_irq when cleanup has already
started. It is not ordered with the PHYP mask,
and it does not close the hole if poll already
passed that check before napi_disable. I am
leaving that in Patch 9 rather than inverting
poll here. This teardown sequence pre-dates MQ,
and completely closing the late enable requires a
poll/teardown handshake (or an RCU grace period
before free_irq) across the driver, which is listed
under Known leftovers in the cover letter.
> [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.
Yes. We ask PHYP to mask, then free_irq even if
that hcall failed, so a delivery can still land
before h_free. synchronize_irq only waits for a
handler that is already running.

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

I am keeping free_irq before h_free in this 15,
after we have issued the mask, because I do not
want ndo_stop to fail on a disable hcall. As you
said, 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.
Thanks. v7 rewords the kdoc to match this helper: out of
range is WARN_ON, then false.

The helper keeps the WARN form here rather than the
later quiet one. At this commit num_rx_queues does not shrink,
so >= num is still a driver bug and the WARN is useful.
interrupt() already returns IRQ_NONE before it calls
this helper, so a residual IRQ does not hit the splat.
The quiet >= live path becomes the right one when
resize can publish a lower count; that is Patch 14.

>> +
>> +	/*
>> +	 * 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?
Yes, PHYP can still write those pages on that path.
Pre-existing — the old pool-fail path already jumped to
the same labels without h_free. The request_irq /
setup_rx_interrupts failure below it does issue h_free.
The later register-helper reorder stays out of
this patch.

The opened gate does drop the accidental recovery: a
later ndo_stop now returns 0 and never reaches h_free.
That residue stays until Patch 6.

Patch 6 ("Refactor TX resource allocation in open/close
paths") issues h_free on that pool-fail path. Patch 7
("Add RX queue register helpers for MQ") then allocates
the pools before register, and every later failure goes
through ibmveth_free_all_queues().
>> -	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?
This patch already fixes that hang: open sets opened
only on success, and close() returns if !opened, so a
later ndo_stop does not napi_disable a second time.
There is no Fixes: tag; the commit message names the
hang and declines one. I am keeping it self-contained
in this 15 to avoid cross-tree dependencies; a
standalone fix for net can follow separately (and we
can rebase on top of it if requested).

That is not the later unwind standalone (h_free / TX LTB).

>>   
>> -	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?
Yes — both are intentional. v7 names them in the commit message.
The harvest stays before h_free so the read still hits a
live buffer-list page. synchronize_net() after IRQ/NAPI
teardown waits for a poll that already passed the
shutdown checks.

Thanks,

Mingming


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