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

[Simon Horman <horms@kernel.org>]

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

...

>  /**
>   * 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.

>  		}
> +	} 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.

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?

 "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?

 "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?


>  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;
>  }

...