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

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

On 11/12/26 3:57 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] This is speculative and latent.
> - [Low] For subordinate queues, ibmveth_toggle_irq()
>    (drivers/net/ethernet/ibm/ibmveth.c) treats an H_PARAMETER return from…
>
> --
>
> ibmveth: Refactor RX interrupt control for MQ RX queues
>
> This adds queue-indexed RX interrupt helpers that use h_vio_signal() for
> queue 0 and H_VIOCTL for subordinate queues. It wires them into
> open/close/poll/interrupt and the process-context RX kick sites. Runtime
> stays single-queue (num_rx_queues is still 1).
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 31fad7713dc70..62e6c9a9f5e91 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -347,6 +349,325 @@ ibmveth_cleanup_rx_resources(struct ibmveth_adapter *adapter)
> [ ... ]
>
>> +static int
>> +ibmveth_toggle_irq(struct ibmveth_adapter *adapter, int queue_index,
>> +		   bool enable)
>> +{
>> +	unsigned long h_rc;
>> +	unsigned long irq = adapter->queue_irq[queue_index];
>> +	const char *action = enable ? "enable" : "disable";
>> +
>> +	if (queue_index == 0) {
>> +		/* Primary queue: use h_vio_signal() */
>> +		h_rc = h_vio_signal(adapter->vdev->unit_address,
>> +				    enable ? VIO_IRQ_ENABLE : VIO_IRQ_DISABLE);
>> +	} else {
>> +		/* Subordinate queues: use H_VIOCTL with hardware IRQ */
>> +		struct irq_data *irq_data = irq_get_irq_data(irq);
>> +		irq_hw_number_t hwirq;
>> +		u64 vioctl_cmd = enable ? H_ENABLE_VIO_INTERRUPT :
>> +			H_DISABLE_VIO_INTERRUPT;
>> +
> [ ... ]
>
>> +		hwirq = irqd_to_hwirq(irq_data);
>> +		h_rc = plpar_hcall_norets(H_VIOCTL,
>> +					  adapter->vdev->unit_address,
>> +					  vioctl_cmd,
>> +					  hwirq, 0, 0);
> [Severity: Medium]
> This one is speculative. Do subordinate queues need the post-migration
> EOI that ibmvnic issues for the same kind of H_VIOCTL-controlled queue
> interrupt?
>
> drivers/net/ethernet/ibm/ibmvnic.c:
>
> /* Due to a firmware bug, the hypervisor can send an interrupt to a
>   * transmit or receive queue just prior to a partition migration.
>   * Force an EOI after migration.
>   */
> static void ibmvnic_clear_pending_interrupt(struct device *dev,
> 					    struct ibmvnic_sub_crq_queue *scrq)
> {
> 	if (!xive_enabled())
> 		ibmvnic_xics_eoi(dev, scrq);
> }
>
> ibmveth_toggle_irq() is the only place that re-arms subordinate queues.
> At the end of the series, ibmveth.c has no H_EOI or xive_enabled()
> handling anywhere.
>
> Suppose ibmveth subordinate RX queues have the same firmware behaviour.
> On XICS, an interrupt pending across LPM would then never get an EOI.
> That queue would stop taking interrupts: NAPI idle, PHYP enabled, RX
> stalled on that queue.

Not reachable for now: the HMC blocks LPM while ILLAN multi-queue is
active, so a partition can't migrate with subordinate queues live.
ibmveth also doesn't get a firmware migration event like ibmvnic does,
so there's nowhere it could issue that EOI today.

On the Power11 LPAR I tested, XIVE is the default, but XICS is still
possible with xive=off, and the ibmvnic workaround is XICS-only. Once
PHYP and the HMC allow LPM with ILLAN multi-queue, I'll check whether
the same firmware issue applies to the subordinate RX queues and add
the post-migration H_EOI if it does.

Thanks,
Mingming

> Nothing would start recovery either. After LPM, ibmveth only schedules a
> reset if the MQ buffer hcalls return H_FUNCTION.
>
> Queue 0 has migrated over XICS for years without this EOI. It is
> controlled by h_vio_signal() rather than H_VIOCTL, though, so that does
> not answer the question for subordinate queues.
>
> This path is latent until the later "ibmveth: Enable multi-queue RX
> receive path" commit. Could it be checked against PAPR/PHYP behaviour
> before multi-queue is enabled?
>
>> +
>> +		/*
>> +		 * Fold H_PARAMETER only on disable (idempotent mask). On
>> +		 * enable it is not already-enabled: a trivial enable
>> +		 * returns success. H_PARAMETER means the interrupt
>> +		 * number was not in the valid range for that unit
>> +		 * address. Keep that an error so a missed LISN 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;
>> +		}
> [Severity: Low]
> Does the comment above this fold actually support it? It says a trivial
> enable already returns success, and that H_PARAMETER means the interrupt
> number was not in the valid range for the unit address.
>
> That describes a bad hwirq argument, not an interrupt that is already
> masked. The same reading would seem to apply to H_DISABLE_VIO_INTERRUPT.
> The commit message calls this an "idempotent mask".
>
> ibmvnic makes the same hcall and treats any non-zero rc as a failure:
>
> drivers/net/ethernet/ibm/ibmvnic.c:disable_scrq_irq() {
>      ...
> 	rc = plpar_hcall_norets(H_VIOCTL, adapter->vdev->unit_address,
> 				H_DISABLE_VIO_INTERRUPT, scrq->hw_irq, 0, 0);
> 	if (rc)
> 		dev_err(dev, "Couldn't disable scrq irq 0x%lx. rc=%ld\n",
> 			scrq->hw_irq, rc);
> 	return rc;
> }
>
> Two teardown paths rely on the mask actually taking effect:
> ibmveth_cleanup_rx_interrupts() and the err_disable_napi rollback in
> ibmveth_setup_rx_interrupts(). Both run mask, synchronize_irq(),
> napi_disable(), synchronize_net(), remask, then free_irq(). With the
> fold, a disable that PHYP rejected looks like a successful mask there.
>
> No subordinate-queue caller of ibmveth_disable_irq() checks the return
> value. So today the visible effect is only the log level (dev_warn
> instead of dev_err) and the self-contradicting comment. The path is also
> only reachable once num_rx_queues > 1, after the later "ibmveth: Enable
> multi-queue RX receive path" commit. The fold is still there at the end
> of the series.
>
> Should H_PARAMETER on disable stay an error, as it does on enable?
> Alternatively, the comment and commit message could explain why disable
> is treated differently.
>
> [ ... ]
>