Re: [PATCH net-next v8 07/15] ibmveth: Add RX queue register helpers for MQ

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch's version notes do not match the code.
- [Low] The v8 notes say ibmveth_register_logical_lan() was changed to
  return `long`, 'the hypercall status type', but the callers convert…

--

ibmveth: Add RX queue register helpers for MQ

This commit moves queue 0 logical LAN registration and teardown out of
ibmveth_open() and ibmveth_close() into ibmveth_register_logical_lan(),
ibmveth_register_rx_queues() and ibmveth_free_all_queues(), and adds a
queue_registered[] flag next to queue_handle[]. It also moves buffer pool
allocation in open() ahead of PHYP registration and calls
netif_set_real_num_rx_queues() after registration.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index cab895cc970c5..e2aa108655676 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1335,10 +1335,31 @@ static void ibmveth_free_tx_resources(struct ibmveth_adapter *adapter)

[ ... ]

> +static long ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,
> +					 union ibmveth_buf_desc rxq_desc,
> +					 u64 mac_address)
> +{
> +	long rc;

[ ... ]

> @@ -1362,14 +1398,97 @@ static int ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,

[ ... ]

> +static void ibmveth_free_all_queues(struct ibmveth_adapter *adapter)
> +{
> +	unsigned long lpar_rc;
> +	int i;
> +
> +	netdev_dbg(adapter->netdev, "freeing all RX queues at once\n");
> +
> +	do {
> +		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
> +	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
> +
> +	if (lpar_rc != H_SUCCESS) {
> +		netdev_err(adapter->netdev,
> +			   "h_free_logical_lan failed: %ld\n", lpar_rc);
> +	}

[ ... ]

> +static int
> +ibmveth_register_rx_queues(struct ibmveth_adapter *adapter, u64 mac_address)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +	union ibmveth_buf_desc rxq_desc;
> +	unsigned long lpar_rc;
> +	int rc;

[ ... ]

> +	lpar_rc = ibmveth_register_logical_lan(adapter, rxq_desc, mac_address);
> +	if (lpar_rc != H_SUCCESS) {
> +		netdev_err(netdev,
> +			   "h_register_logical_lan failed: %ld\n", lpar_rc);

[Severity: Low]
This isn't a bug, but the v8 changelog says:

  - ibmveth_register_logical_lan() returns long, the hypercall status
    type

ibmveth_register_rx_queues() stores that long return value in an
unsigned long lpar_rc and prints it with %ld.

ibmveth_free_all_queues() does the same with the return value of
h_free_logical_lan(). The close() code it replaces declared that local
as long lpar_rc.

PAPR H_* error codes are negative (H_PARAMETER is -4, for example). The
printed value only comes out right because varargs pass the same bits,
and the default -Wformat does not check signedness.

Should both locals be declared long so the status stays signed?

[ ... ]

> @@ -1480,15 +1597,7 @@ static int ibmveth_close(struct net_device *netdev)
>  
>  	ibmveth_cleanup_rx_interrupts(adapter);
>  
> -	/* Full LAN teardown (subordinates arrive with register helpers). */
> -	do {
> -		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
> -	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
> -	if (lpar_rc != H_SUCCESS) {
> -		netdev_err(adapter->netdev,
> -			   "h_free_logical_lan failed with %lx, continuing\n",
> -			   lpar_rc);
> -	}
> +	ibmveth_free_all_queues(adapter);
>  	ibmveth_update_rx_no_buffer(adapter);

[Severity: Low]
This isn't a bug, but the per-version notes don't match the code here.
The v5 notes say:

  - Call out close() update_rx_no_buffer() before free_lan as intentional
    last-glimpse accounting while the LAN is still registered (order
    already in v4; v4 step list omitted it)

In ibmveth_close(), ibmveth_free_all_queues() issues H_FREE_LOGICAL_LAN
first, and ibmveth_update_rx_no_buffer() runs after it. So the
no-buffer counter is read after the LAN has been freed, not while it is
still registered.

The pre-patch close() used the same order, so behaviour is unchanged.
The note just describes an ordering the code does not follow.

The v6 notes also say:

  This patch no longer touches ibmveth.h

However, v8 adds queue_registered[] to struct ibmveth_adapter in
drivers/net/ethernet/ibm/ibmveth.h.

Could these notes be updated to match the code?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235556.60604-1-mmc%40linux.ibm.com