Re: [PATCH net-next v3 08/15] ibmveth: Refactor open/close into MQ-ready resource pipeline

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

On 7/14/26 5:47 AM, Simon Horman wrote:

> On Mon, Jul 06, 2026 at 12:35:56PM -0700, Mingming Cao wrote:
>
> ...
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> ...
*

Hi, Simon


Thanks for the careful read on open/close — these were all fair points.

*
>>   /**
>>    * ibmveth_register_logical_lan_queue - Register subordinate queue with hypervisor
>>    * @adapter: ibmveth adapter structure
>> @@ -1466,208 +1479,108 @@ ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
>>   static int ibmveth_open(struct net_device *netdev)
>>   {
>>   	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> -	u64 mac_address;
>> +	u64 mac_address = ether_addr_to_u64(netdev->dev_addr);
>>   	int rxq_entries = 1;
>> -	unsigned long lpar_rc;
>>   	int rc;
>> -	union ibmveth_buf_desc rxq_desc;
>>   	int i;
>> -	struct device *dev;
>>   
>>   	netdev_dbg(netdev, "open starting\n");
>>   
>> -	napi_enable(&adapter->napi[0]);
>> -
>> -	for(i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
>>   		rxq_entries += adapter->rx_buff_pool[0][i].size;
>>   
>> -	rc = -ENOMEM;
>> -	adapter->buffer_list_addr[0] = (void *)get_zeroed_page(GFP_KERNEL);
>> -	if (!adapter->buffer_list_addr[0]) {
>> -		netdev_err(netdev, "unable to allocate list pages\n");
>> +	rc = ibmveth_alloc_rx_qstats(adapter);
>> +	if (rc)
>>   		goto out;
>> -	}
>>   
>> -	adapter->filter_list_addr = (void*) get_zeroed_page(GFP_KERNEL);
>> -	if (!adapter->filter_list_addr) {
>> -		netdev_err(netdev, "unable to allocate filter pages\n");
>> -		goto out_free_buffer_list;
>> -	}
>> -
>> -	dev = &adapter->vdev->dev;
>> +	rc = ibmveth_alloc_filter_list(adapter);
>> +	if (rc)
>> +		goto out_free_rx_qstats;
>>   
>> -	adapter->rx_queue[0].queue_len = sizeof(struct ibmveth_rx_q_entry) *
>> -						rxq_entries;
>> -	adapter->rx_queue[0].queue_addr =
>> -		dma_alloc_coherent(dev, adapter->rx_queue[0].queue_len,
>> -				   &adapter->rx_queue[0].queue_dma, GFP_KERNEL);
>> -	if (!adapter->rx_queue[0].queue_addr)
>> +	rc = ibmveth_alloc_rx_queues(adapter, rxq_entries);
>> +	if (rc)
>>   		goto out_free_filter_list;
>>   
>> -	adapter->buffer_list_dma[0] = dma_map_single(dev,
>> -						     adapter->buffer_list_addr[0],
>> -						     4096, DMA_BIDIRECTIONAL);
>> -	if (dma_mapping_error(dev, adapter->buffer_list_dma[0])) {
>> -		netdev_err(netdev, "unable to map buffer list pages\n");
>> +	rc = ibmveth_alloc_buffer_pools(adapter);
>> +	if (rc)
>>   		goto out_free_queue_mem;
>> -	}
>>   
>> -	adapter->filter_list_dma = dma_map_single(dev,
>> -			adapter->filter_list_addr, 4096, DMA_BIDIRECTIONAL);
>> -	if (dma_mapping_error(dev, adapter->filter_list_dma)) {
>> -		netdev_err(netdev, "unable to map filter list pages\n");
>> -		goto out_unmap_buffer_list;
>> -	}
>> +	rc = ibmveth_register_rx_queues(adapter, mac_address);
>> +	if (rc)
>> +		goto out_free_buffer_pools;
>>   
>> -	for (i = 0; i < netdev->real_num_tx_queues; i++) {
>> -		if (ibmveth_allocate_tx_ltb(adapter, i))
>> -			goto out_free_tx_ltb;
>> +	rc = netif_set_real_num_rx_queues(netdev, adapter->num_rx_queues);
>> +	if (rc) {
>> +		netdev_err(netdev, "failed to set number of rx queues\n");
>> +		goto out_unregister_queues;
>>   	}
>>   
>> -	adapter->rx_queue[0].index = 0;
>> -	adapter->rx_queue[0].num_slots = rxq_entries;
>> -	adapter->rx_queue[0].toggle = 1;
>> -
>> -	mac_address = ether_addr_to_u64(netdev->dev_addr);
>> -
>> -	rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
>> -					adapter->rx_queue[0].queue_len;
>> -	rxq_desc.fields.address = adapter->rx_queue[0].queue_dma;
>> -
>> -	netdev_dbg(netdev, "buffer list @ 0x%p\n", adapter->buffer_list_addr[0]);
>> -	netdev_dbg(netdev, "filter list @ 0x%p\n", adapter->filter_list_addr);
>> -	netdev_dbg(netdev, "receive q   @ 0x%p\n", adapter->rx_queue[0].queue_addr);
>> -
>> -	h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
>> -
>> -	lpar_rc = ibmveth_register_logical_lan(adapter, rxq_desc, mac_address);
>> -
>> -	if (lpar_rc != H_SUCCESS) {
>> -		netdev_err(netdev, "h_register_logical_lan failed with %ld\n",
>> -			   lpar_rc);
>> -		netdev_err(netdev, "buffer TCE:0x%llx filter TCE:0x%llx rxq "
>> -			   "desc:0x%llx MAC:0x%llx\n",
>> -				     adapter->buffer_list_dma[0],
>> -				     adapter->filter_list_dma,
>> -				     rxq_desc.desc,
>> -				     mac_address);
>> -		rc = -ENONET;
>> -		goto out_unmap_filter_list;
>> -	}
>> +	rc = ibmveth_setup_rx_interrupts(adapter);
>> +	if (rc)
>> +		goto out_unregister_queues;
>>   
>> -	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> -		if (!adapter->rx_buff_pool[0][i].active)
>> -			continue;
>> -		if (ibmveth_alloc_buffer_pool(&adapter->rx_buff_pool[0][i])) {
>> -			netdev_err(netdev, "unable to alloc pool\n");
>> -			adapter->rx_buff_pool[0][i].active = 0;
>> -			rc = -ENOMEM;
>> -			goto out_free_buffer_pools;
>> +	if (adapter->num_rx_queues > 1) {
>> +		for (i = 0; i < adapter->num_rx_queues; i++) {
>> +			netdev_dbg(netdev, "initial replenish cycle for queue %d\n", i);
>> +			ibmveth_replenish_task(adapter, i);
> ibmveth_replenish_task() only has one parameter
> until a later patch in this series.
*

Agreed. Will address this In v4.

the multi-arg replenish call moves to the patch that introduces

the queue-index parameter (or that patch lands

before this open() wiring).

*
>>   		}
>> +	} else {
>> +		netdev_dbg(netdev, "initial replenish cycle\n");
>> +		ibmveth_interrupt(adapter->queue_irq[0], &adapter->napi[0]);
>>   	}
>>   
>> -	netdev_dbg(netdev, "registering irq 0x%x\n", netdev->irq);
>> -	rc = request_irq(netdev->irq, ibmveth_interrupt, 0, netdev->name,
>> -			 netdev);
>> -	if (rc != 0) {
>> -		netdev_err(netdev, "unable to request irq 0x%x, rc %d\n",
>> -			   netdev->irq, rc);
>> -		do {
>> -			lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
>> -		} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
>> -
>> -		goto out_free_buffer_pools;
>> -	}
>> -
>> -	rc = -ENOMEM;
>> -
>> -	netdev_dbg(netdev, "initial replenish cycle\n");
>> -	ibmveth_interrupt(netdev->irq, netdev);
>> +	rc = ibmveth_alloc_tx_resources(adapter);
>> +	if (rc)
>> +		goto out_cleanup_rx_interrupts;
>>   
>>   	netif_tx_start_all_queues(netdev);
>>   
>>   	netdev_dbg(netdev, "open complete\n");
>> -
>>   	return 0;
>>   
>> +out_cleanup_rx_interrupts:
>> +	ibmveth_cleanup_rx_interrupts(adapter);
>> +out_free_tx_resources:
>> +	ibmveth_free_tx_resources(adapter);
> The out_free_tx_resources label is unused until a later patch of this
> series, so it should be added in that patch rather than this one.
*

Agreed — will introduce out_free_tx_resources only when a caller

needs that jump target.

*
>
> And it's not clear to me that ibmveth_free_tx_resources() should
> be called when jumping to out_cleanup_rx_interrupts as
> in that case ibmveth_alloc_tx_resources() hasn't run successfully.
>
> The AI-generated review on sashiko.dev also highlights the error handling
> here:
>
>   "Are the unwind labels ordered incorrectly here?
>
>   "If ibmveth_setup_rx_interrupts() fails, it jumps to
>    out_unregister_queues, which is placed after out_free_buffer_pools. Does
>    this mean we skip freeing the buffer pools and leak memory?
*

Yes — those gotos currently skip free_buffer_pools(). Will fix

unwind ordering in v4 so buffer pools are always released.

*

>
>   "Also, if ibmveth_alloc_tx_resources() fails, it internally frees
>    partially allocated LTBs. It then jumps to out_cleanup_rx_interrupts and
>    falls through to out_free_tx_resources. Because ibmveth_free_tx_ltb()
>    calls dma_unmap_single() unconditionally without checking or zeroing
>    tx_ltb_dma, will this cause a double free and an invalid DMA unmap?

**

*Correct — on alloc_tx_resources() failure TX is already partially*

*

cleaned inside the helper, so falling through to

free_tx_resources() is wrong and can double-unmap. Will fix the

open() unwind graph in v4.

*

>
>   "Finally, in the fall-through path from out_cleanup_rx_interrupts,
>    out_free_buffer_pools is executed before out_unregister_queues (which
>    calls ibmveth_free_all_queues() to unregister the logical LAN). Does this
>    free and unmap the RX buffers while the hypervisor's logical LAN is still
>    active, potentially allowing the hypervisor to DMA incoming packets into
>    freed memory?
>
**

*Agreed — freeing RX pools before h_free_logical_lan() is unsafe.*

*

v4 will unregister/free the logical LAN before releasing buffer

pools on both open failure and close

**

*Thanks,*

*

Mingming

*

*

>>   out_free_buffer_pools:
>> -	while (--i >= 0) {
>> -		if (adapter->rx_buff_pool[0][i].active)
>> -			ibmveth_free_buffer_pool(adapter,
>> -						 &adapter->rx_buff_pool[0][i]);
>> -	}
>> -out_unmap_filter_list:
>> -	dma_unmap_single(dev, adapter->filter_list_dma, 4096,
>> -			 DMA_BIDIRECTIONAL);
>> -
>> -out_free_tx_ltb:
>> -	while (--i >= 0) {
>> -		ibmveth_free_tx_ltb(adapter, i);
>> -	}
>> -
>> -out_unmap_buffer_list:
>> -	dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
>> -			 DMA_BIDIRECTIONAL);
>> +	ibmveth_free_buffer_pools(adapter);
>> +out_unregister_queues:
>> +	ibmveth_dispose_subordinate_irq_mappings(adapter);
>> +	ibmveth_free_all_queues(adapter);
>>   out_free_queue_mem:
>> -	dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
>> -			  adapter->rx_queue[0].queue_addr,
>> -			  adapter->rx_queue[0].queue_dma);
>> +	ibmveth_cleanup_rx_resources(adapter);
>>   out_free_filter_list:
>> -	free_page((unsigned long)adapter->filter_list_addr);
>> -out_free_buffer_list:
>> -	free_page((unsigned long)adapter->buffer_list_addr[0]);
>> +	ibmveth_free_filter_list(adapter);
>> +out_free_rx_qstats:
>> +	ibmveth_free_rx_qstats(adapter);
>>   out:
>> -	napi_disable(&adapter->napi[0]);
>>   	return rc;
>>   }
> ...