On 11/12/26 3:57 PM, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 2
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] ibmveth_destroy_subordinate_rx_queue() is used for live
ethtool -L scale-down, the scale-up enable_irq-failure branch and the…
- [Low] When a scale-down hits the destroy-failure keep path,
ibmveth_scale_down_rx_queues() (drivers/net/ethernet/ibm/ibmveth.c)…
- [Low] The kernel-doc of ibmveth_scale_up_rx_queues() says "A failure
unwinds only the queues this call added; a queue PHYP refuses to…
--
ibmveth: Implement incremental MQ RX queue resize
This commit lets ethtool -L change the RX queue count on a live ibmveth
adapter one queue at a time. It adds ibmveth_scale_up_rx_queues() and
ibmveth_scale_down_rx_queues(), with unwind paths for hypervisor
failures. It also factors ibmveth_desired_dma_for_rxqs() out of
ibmveth_get_desired_dma(), and frees queues stranded above the live
count after a successful H_FREE_LOGICAL_LAN.
diff --git a/drivers/net/ethernet/ibm/ibmveth.c
b/drivers/net/ethernet/ibm/ibmveth.c
index e805e7b1d2fe2..510d9d80e2df5 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
@@ -2028,6 +2315,644 @@ static void ibmveth_release_lan_resources(struct
ibmveth_adapter *adapter)
ibmveth_free_filter_list(adapter);
}
[ ... ]
+/**
+ * ibmveth_destroy_subordinate_rx_queue - Tear down one subordinate RX queue
+ * @adapter: ibmveth adapter structure
+ * @queue_idx: Queue index to destroy (1..N)
+ *
+ * Deregister with PHYP before unmapping buffer pools so hypervisor buffer
+ * ownership is released while queue metadata is still valid. After a
+ * successful H_FREE, fold the last no_buffer sample into retired while
+ * the page is still mapped. If the hcall fails, leave the queue mapped,
+ * keep the handle, and skip retire.
+ *
+ * Return: 0 on success, -EIO if deregister failed
+ */
+static int
+ibmveth_destroy_subordinate_rx_queue(struct ibmveth_adapter *adapter,
+ int queue_idx)
+{
+ int rc;
+
+ rc = ibmveth_deregister_single_rx_queue(adapter, queue_idx);
+ if (rc)
+ return rc;
+ ibmveth_retire_rx_no_buffer(adapter, queue_idx);
+ ibmveth_cleanup_single_rx_interrupt(adapter, queue_idx);
+ ibmveth_free_single_rx_queue(adapter, queue_idx);
+ return 0;
+}
[Severity: Medium]
Is the order of H_FREE_LOGICAL_LAN_QUEUE and the IRQ teardown safe here?
ibmveth_deregister_single_rx_queue() issues H_FREE_LOGICAL_LAN_QUEUE
first. Only after that does ibmveth_cleanup_single_rx_interrupt() run
free_irq() and irq_dispose_mapping() on the queue's virq. That virq was
created by irq_create_mapping(NULL, hwirq), using the hwirq returned by
H_REG_LOGICAL_LAN_QUEUE. So the interrupt source belongs to the queue
being freed.
On pseries XIVE, free_irq() reaches xive_irq_shutdown(), which does an
ESB access and an H_INT_SET_SOURCE_CONFIG hcall against that source:
arch/powerpc/sysdev/xive/common.c:xive_irq_shutdown() {
...
xive_do_source_set_mask(xd, true);
xive_ops->configure_irq(hw_irq,
get_hard_smp_processor_id(xd->target),
0xff, XIVE_BAD_IRQ);
...
}
irq_dispose_mapping() then tears down the per-source XIVE data and its
ESB mapping.
If H_FREE_LOGICAL_LAN_QUEUE releases the LISN, these operations would
run against a source the partition no longer owns. Could that produce
failed hcalls, stale irq_desc state, or an ESB access to a page the
hypervisor no longer backs?
Checked this with the PHYP team. H_FREE_LOGICAL_LAN and
H_FREE_LOGICAL_LAN_QUEUE don't release the interrupt source; the sources
stay allocated while the adapter is allocated, including while it's
down, and can be masked or unmasked during that time. Before this path
calls H_FREE_LOGICAL_LAN_QUEUE, scale-down has already masked the queue
in PHYP with H_VIOCTL and waited for the handler with synchronize_irq().
The rest of this driver tears the IRQ down first:
- ibmveth_close() runs ibmveth_cleanup_rx_interrupts() before
ibmveth_free_all_queues().
- The open() error path disposes the mappings before
H_FREE_LOGICAL_LAN.
- The scale-up request_irq failure branch disposes before it
deregisters.
ibmvnic's release_sub_crqs() also calls free_irq() and
irq_dispose_mapping() before release_sub_crq_queue() issues
H_FREE_SUB_CRQ.
The commit message explains why deregister has to come before the DMA
unmap, but not why it has to come before the IRQ teardown. Would it
work to keep PHYP masked, call free_irq() (and dispose the mapping),
then issue H_FREE_LOGICAL_LAN_QUEUE, and re-request the IRQ in the rare
case the hcall fails?
The order is there for the rollback path, not the normal success path.
If H_FREE_LOGICAL_LAN_QUEUE fails with something other than busy, the
queue stays registered and PHYP may still DMA into it. Scale-down and
the scale-up unwind then put that same queue back in service by
enabling NAPI and unmasking the same virq. If we'd already freed the
IRQ and disposed the mapping before the H_FREE, the failure path would
have to rebuild the mapping and request the IRQ again in the middle of a
rollback that's already failing.
So destroy keeps the IRQ until H_FREE succeeds, or until H_PARAMETER
says the queue is already free. I'd rather not reorder just to match
close(): close() can free the IRQs first because it never has to put
the LAN back in service after a failed H_FREE_LOGICAL_LAN.
Thanks,
Mingming
