Re: [PATCH net-next v4 04/13] net: mana: swap queue sets in mana_set_ringparam

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces an ad-hoc locking pattern using a boolean flag (`apc->channel_changing`) to guard the reconfiguration critical section in `mana_set_ringparam`.
--

commit 40668ebf41a83bf44835ffb33803930c039ce405
Author: Long Li <longli@microsoft.com>

net: mana: swap queue sets in mana_set_ringparam

This commit replaces detach/attach operations in mana_set_ringparam() with
queue-set allocation and publication. Failed allocations now leave the
running queues and ring sizes unchanged instead of risking a detached port.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
[ ... ]
> @@ -774,32 +772,56 @@ static int mana_set_ringparam(struct net_device *ndev,
>  	netdev_info(ndev, "Using nearest power of 2 values for Txq:%d Rxq:%d\n",
>  		    new_tx, new_rx);
>  
> -	/* pre-allocating new buffers to prevent failures in mana_attach() later */
> -	apc->rx_queue_size = new_rx;
> -	err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> -	apc->rx_queue_size = old_rx;
> -	if (err) {
> -		netdev_err(ndev, "Insufficient memory for new allocations\n");
> -		return err;
> +	if (new_rx == apc->rx_queue_size && new_tx == apc->tx_queue_size)
> +		return 0;
> +
> +	if (!apc->port_is_up) {
> +		apc->rx_queue_size = new_rx;
> +		apc->tx_queue_size = new_tx;
> +		return 0;
>  	}
>  
> -	err = mana_detach(ndev, false);
> -	if (err) {
> -		netdev_err(ndev, "mana_detach failed: %d\n", err);
> -		goto out;
> +	/* Exclude RDMA through failure cleanup, which may release the vport. */
> +	mutex_lock(&apc->vport_mutex);
> +	if (apc->channel_changing) {
> +		mutex_unlock(&apc->vport_mutex);
> +		return -EBUSY;
> +	}
> +	apc->channel_changing = true;
> +	mutex_unlock(&apc->vport_mutex);

[Severity: Medium]
Does this code introduce an ad-hoc locking pattern using the
apc->channel_changing boolean flag?

The networking drivers subsystem guidelines indicate that using a boolean
flag set on entry and cleared on exit to guard a reconfiguration critical
section bypasses lockdep and provides weaker synchronization guarantees than
using a proper lock.

Could a standard kernel synchronization primitive be used for this critical
section in mana_set_ringparam() instead of relying on the
apc->channel_changing flag?

[ ... ]
> +clear_flag:
> +	mutex_lock(&apc->vport_mutex);
> +	apc->channel_changing = false;
> +	mutex_unlock(&apc->vport_mutex);
>  	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908032843.397667-1-longli@microsoft.com?part=4