Re: [PATCH net-next v3 14/15] ibmveth: Wire ethtool set_channels to MQ RX queue resize

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

On 7/14/26 6:07 AM, Simon Horman wrote:

> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Wire ethtool set_channels to MQ RX queue resize
>
> Expose incremental RX resize through ethtool channel control.
> get_channels() reports rx_count from adapter->num_rx_queues and max_rx
> as IBMVETH_MAX_RX_QUEUES when MQ firmware is enabled, else 1.
> set_channels() validates rx_count is within 1..IBMVETH_MAX_RX_QUEUES.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index ac4d89a66a8db..50a332ab83fd3 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -2534,19 +2534,69 @@ static int ibmveth_set_channels(struct net_device *netdev,
>>   				struct ethtool_channels *channels)
>>   {
>>   	struct ibmveth_adapter *adapter = netdev_priv(netdev);
> [ ... ]
>> -	if (!(netdev->flags & IFF_UP))
>> +	if (!(netdev->flags & IFF_UP)) {
>> +		if (goal_rx > 1 && !adapter->multi_queue) {
> [ ... ]
>> +		/* Stash desired RX count; open() publishes it via
>> +		 * netif_set_real_num_rx_queues() after queue registration.
>> +		 */
>> +		if (goal_rx != adapter->num_rx_queues)
>> +			adapter->num_rx_queues = goal_rx;
>> +
>>   		return netif_set_real_num_tx_queues(netdev, goal);
> [Severity: Medium]
> If netif_set_real_num_tx_queues() fails and returns an error, the TX
> configuration command will fail, but the RX queue count has already been
> permanently modified in adapter->num_rx_queues. Will this leave the device in
> an inconsistent state where the new RX count is applied on the next open
> despite the command failing?
*

Agreed — will only commit the stashed RX count after

netif_set_real_num_tx_queues() succeeds, or restore on failure.

*
>> +	}
> [ ... ]
>> +	if (goal_rx != old_rx) {
>> +		rc = ibmveth_resize_rx_queues_incremental(adapter, goal_rx,
>> +							  rxq_entries);
> [Severity: High]
> Does this new ethtool scale-down path expose an interrupt storm?
>
> Looking at ibmveth_resize_rx_queues_incremental(), when scaling down queues it
> disables NAPI before disabling the hardware interrupt:
>
> drivers/net/ethernet/ibm/ibmveth.c:ibmveth_resize_rx_queues_incremental() {
>      ...
> 		for (i = new_count; i < old_count; i++)
> 			napi_disable(&adapter->napi[i]);
>
> 		for (i = new_count; i < old_count; i++)
> 			ibmveth_drain_rx_queue(adapter, i);
>
> 		synchronize_net();
>      ...
> 		/* Disable hypervisor interrupts and wait for handlers to complete
> 		 * before updating num_rx_queues.
> 		 */
> 		for (i = new_count; i < old_count; i++) {
> 			ibmveth_disable_irq(adapter, i);
>      ...
> }
>
> If the hypervisor delivers a packet during this window, ibmveth_interrupt()
> will execute. Since NAPI is disabled, napi_schedule_prep() fails, causing the
> handler to return IRQ_HANDLED without disabling the hardware interrupt. Will
> this leave the virtual interrupt asserted and force an infinite interrupt loop?
Yeah, *Same issue as on the incremental resize patch — **

  will fix IRQ/NAPI ordering in ibmveth_resize_rx_queues_incremental() 
in v4.

*
>> +		if (rc) {
>> +			netdev_err(netdev, "Failed to resize RX queues: %d\n", rc);
>> +			return rc;
>> +		}
>> +	}
>>   
>>   	/* We have IBMVETH_MAX_QUEUES netdev_queue's allocated
>>   	 * but we may need to alloc/free the ltb's.
>>   	 */
> [Severity: High]
> This is a pre-existing issue, but does this function use an uninitialized
> variable in the error path?
>
> When decreasing the number of TX queues (goal < old), the allocation loop for
> (i = old; i < goal; i++) is skipped, leaving 'i' uninitialized. If
> netif_set_real_num_tx_queues() then fails, the error path assigns old = i;.
>
> The cleanup loop then uses this garbage value:
> 	for (i = old; i > goal; i--) {
> 		if (adapter->tx_ltb_ptr[i - 1])
>
> Will this access adapter->tx_ltb_ptr out of bounds and pass arbitrary pointers
> to ibmveth_free_tx_ltb()?
*

Good catch — pre-existing, but we’ll fix it while touching

set_channels in v4 (initialize i / don’t use it uninitialized on

TX scale-down failure).

*
>> @@ -2580,7 +2630,7 @@ static int ibmveth_set_channels(struct net_device *netdev,
>>   
>>   	netif_tx_wake_all_queues(netdev);
>>   
>> -	return rc;
>> +	return 0;
> [Severity: High]
> Does this unconditional return of 0 mask TX queue configuration failures?
>
> If applying the new TX queue count via netif_set_real_num_tx_queues() fails,
> the code handles it locally, but the function exits with 0 regardless. Will
> this mislead userspace into believing the TX channel count was successfully
> modified?
*

Agreed — that return 0 is a regression versus returning rc. Will

restore proper error reporting in v4 (and be careful about partial

RX-success / TX-failure).

*
*

Thanks for your review feedback.

Mingming

*