Re: [PATCH net-next v5 02/15] ibmveth: Prepare MQ RX adapter data structures

On 8/17/26 6:47 PM, Jakub Kicinski 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.
> ---
> ibmveth: Prepare MQ RX adapter data structures
>
> This converts the single-instance RX state in struct ibmveth_adapter (napi,
> buffer_list_addr/dma, rx_queue, rx_buff_pool) into queue-indexed arrays sized
> by a new IBMVETH_MAX_RX_QUEUES, and adds queue_handle[]/queue_irq[] plus
> multi_queue/num_rx_queues bookkeeping. All existing references are
> mechanically retargeted to index 0, and IBMVETH_MAX_RX_QUEUES is kept at 1 so
> behaviour is unchanged.
>
> No Link: tags are present in the commit header.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 73e051d26b9d8..7cb828b476c1b 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -482,24 +486,24 @@ static int ibmveth_remove_buffer_from_pool(struct ibmveth_adapter *adapter,
>>   
>>   	mb();
>>   
>> -	atomic_dec(&(adapter->rx_buff_pool[pool].available));
>> +	atomic_dec(&adapter->rx_buff_pool[0][pool].available);
>>   
>>   	return 0;
>>   }
> [Severity: Low]
> This isn't a bug, but the v5 change notes below the --- separator say the
> "extra atomic_dec parentheses" nit from v4 was dropped, and that "this tip
> patch keeps only the mechanical [0] retargeting".
>
> The atomic_dec() in ibmveth_remove_buffer_from_pool() still drops the
> redundant inner parentheses that the baseline had:
>
> 	-	atomic_dec(&(adapter->rx_buff_pool[pool].available));
> 	+	atomic_dec(&adapter->rx_buff_pool[0][pool].available);
>
> Should the changelog be adjusted, or the parentheses kept as-is so the hunk
> really is index-only?

The hunk is the [0] retarget plus dropping those parentheses. Keeping them
fails checkpatch --strict (Unnecessary parentheses) because the retarget
rewrites the line.

>
> [ ... ]
>
>> @@ -1876,6 +1886,10 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   		netdev->features |= NETIF_F_FRAGLIST;
>>   	}
>>   
>> +	/* Initialize queue count - always 1 for now */
>> +	adapter->multi_queue = 0;
>> +	adapter->num_rx_queues = IBMVETH_DEFAULT_RX_QUEUES;
>> +
> [Severity: Low]
> At this commit these two assignments in ibmveth_probe() are the only accesses
> to multi_queue and num_rx_queues, and queue_handle[]/queue_irq[] have neither
> a reader nor a writer anywhere in the driver.
>
> Combined with IBMVETH_MAX_RX_QUEUES being 1U, every array conversion in this
> patch is a pure layout change with no behavioural effect.
>
> Is it worth mentioning in the commit message which later patch first consumes
> each of these fields? Reading the rest of the series, queue_irq[] and
> queue_handle[] get their first users in ibmveth_toggle_irq() and
> ibmveth_register_logical_lan_queue(), num_rx_queues is read through
> ibmveth_get_num_rx_queues(), and IBMVETH_MAX_RX_QUEUES becomes 16U in
> "ibmveth: Enable multi-queue RX receive path", so nothing is broken here.
ch of these fields? Reading the rest of the series,
 > queue_irq[] and queue_handle[] get their first users in
 > ibmveth_toggle_irq() and ibmveth_register_logical_lan_queue(),
 > num_rx_queues is read through ibmveth_get_num_rx_queues(), and
 > IBMVETH_MAX_RX_QUEUES becomes 16U in "ibmveth: Enable multi-queue RX
 > receive path", so nothing is broken here.

Yes. v6 names the first consumer of each field. Three of those are
earlier than the map you sketched:

   num_rx_queues      RX resource-allocation patch
   queue_irq[]             IRQ control patch (ibmveth_toggle_irq)
   multi_queue           IRQ control patch (ibmveth_setup_rx_interrupts)
   queue_handle[]       register-helpers patch, from
                                 H_REGISTER_LOGICAL_LAN (queue 0), not
                                 H_REG_LOGICAL_LAN_QUEUE

>>   	if (ret == H_SUCCESS &&
>>   	    (ret_attr & IBMVETH_ILLAN_RX_MULTI_BUFF_SUPPORT)) {
>>   		adapter->rx_buffers_per_hcall = IBMVETH_MAX_RX_PER_HCALL;
>> @@ -1898,10 +1912,10 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   		memcpy(pool_count, pool_count_cmo, sizeof(pool_count));
>>   
>>   	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> -		struct kobject *kobj = &adapter->rx_buff_pool[i].kobj;
>> +		struct kobject *kobj = &adapter->rx_buff_pool[0][i].kobj;
>>   		int error;
>>   
>> -		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[i], i,
>> +		ibmveth_init_buffer_pool(&adapter->rx_buff_pool[0][i], i,
>>   					 pool_count[i], pool_size[i],
>>   					 pool_active[i]);
>>   		error = kobject_init_and_add(kobj, &ktype_veth_pool,
> [Severity: Low]
> This is a pre-existing issue and is not introduced by this patch, which only
> rewrites rx_buff_pool[i] to rx_buff_pool[0][i] on these lines.
>
> The kobjects registered here live inside netdev_priv() memory, and the probe
> failure paths after this loop call free_netdev() without a matching
> kobject_del()/kobject_put():
>
> 	rc = register_netdev(netdev);
> 	if (rc) {
> 		free_netdev(netdev);
> 		return rc;
> 	}
>
> The only teardown loop is in ibmveth_remove():
>
> 	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> 		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>
> which a failed probe never reaches, so a netif_set_real_num_tx_queues() or
> register_netdev() failure leaves live .../poolN sysfs directories backed by
> freed storage.
>
> Worth noting this is addressed later in the same series: "ibmveth: Enable
> multi-queue RX receive path" adds ibmveth_probe_cleanup(), which calls
> ibmveth_put_pool_kobjs() before free_netdev() on both probe failure paths.
>
> [ ... ]
Yes. v6 notes that the leak predates this [0] retarget. Inline
kobject_put on register failure starts in the MQ enablement patch;
ibmveth_probe_cleanup() replaces it in the statistics patch.

>> diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
>> index c0ef03cad9b9a..fc6a01e810c6e 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.h
>> +++ b/drivers/net/ethernet/ibm/ibmveth.h
>> @@ -257,6 +257,8 @@ static inline long h_illan_attributes(unsigned long unit_address,
>>   #define IBMVETH_MAX_TX_BUF_SIZE (1024 * 64)
>>   #define IBMVETH_MAX_QUEUES 16U
>>   #define IBMVETH_DEFAULT_QUEUES 8U
>> +#define IBMVETH_MAX_RX_QUEUES 1U
>> +#define IBMVETH_DEFAULT_RX_QUEUES 1U
>>   #define IBMVETH_MAX_RX_PER_HCALL 8U
> [ ... ]
>
>> +	struct ibmveth_rx_q rx_queue[IBMVETH_MAX_RX_QUEUES];
>> +	u64 queue_handle[IBMVETH_MAX_RX_QUEUES];
>> +	unsigned int queue_irq[IBMVETH_MAX_RX_QUEUES];
>> +	int multi_queue;
>> +	unsigned int num_rx_queues;
> [ ... ]