On 11/12/26 3:57 PM, [email protected] 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.
[ ... ]
