Re: [PATCH net-next v8 14/15] ibmveth: Implement incremental MQ RX queue resize

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