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

[Jakub Kicinski <kuba@kernel.org>]

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?

[ ... ]

> @@ -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.

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

[ ... ]

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

[ ... ]