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