Re: [PATCH net-next v7 06/13] net: mana: swap queue sets in mana_set_priv_flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: mana: ad-hoc trylock using channel_changing flag
--

commit 1135b1db29f999b173b293508a85336e600ae80e
Author: Long Li <longli@microsoft.com>

net: mana: swap queue sets in mana_set_priv_flags

Rebuild queues through the swap path when USE_FULL_PAGE_RXBUF changes
the RX layout. Carry priv_flags with the queue set so allocation failure
leaves the live configuration unchanged and rollback restores the flags.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index a643a8cc81e00..9c7c9879d0aa4 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -891,54 +892,50 @@ static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
>  	if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
>  		return -EINVAL;
>  
> -	apc->priv_flags = priv_flags;
> -
> -	if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> -		if (!apc->port_is_up)
> -			return 0;
> -
> -		/* If XDP is attached or the MTU already forces one buffer per
> -		 * page, single-buffer-per-page is used regardless of this
> -		 * flag. Skip the expensive detach/attach cycle since nothing
> -		 * changes.
> -		 */
> -		if (mana_single_rxbuf_per_page_forced(apc, ndev->mtu))
> -			return 0;
> +	/* No rebuild is needed if the RX buffer flag is unchanged, the port is
> +	 * down, or full-page RX buffers are already required by MTU or XDP.
> +	 */
> +	if (!(changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) ||
> +	    !apc->port_is_up ||
> +	    mana_single_rxbuf_per_page_forced(apc, ndev->mtu)) {
> +		apc->priv_flags = priv_flags;
> +		return 0;
> +	}
>  
> -		/* Block RDMA from grabbing the vport during detach/attach */
> -		mutex_lock(&apc->vport_mutex);
> -		apc->channel_changing = true;
> +	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 open-coded trylock pattern?

Setting a flag on entry and clearing it on exit to prevent concurrent execution,
and returning -EBUSY to skip the work, appears to act as an ad-hoc lock
substitute for the queue rebuild section. Could a proper lock or synchronization
primitive be used instead to guard this state?

>  
> -		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> -		if (err) {
> -			netdev_err(ndev,
> -				   "Insufficient memory for new allocations\n");
> -			apc->priv_flags = old_priv_flags;
> -			goto clear_flag;
> -		}
> +	scratch = mana_qset_scratch_alloc(apc);
> +	if (!scratch) {
> +		err = -ENOMEM;
> +		goto clear_flag;
> +	}
>  
> -		err = mana_detach(ndev, false);
> -		if (err) {
> -			netdev_err(ndev, "mana_detach failed: %d\n", err);
> -			apc->priv_flags = old_priv_flags;
> -			goto out;
> -		}
> +	err = mana_alloc_qset(apc, scratch, apc->num_queues, apc->rx_queue_size,
> +			      apc->tx_queue_size, priv_flags, &newq);
> +	if (err)
> +		goto free_scratch;
>  
> -		err = mana_attach(ndev);
> -		if (err) {
> -			netdev_err(ndev, "mana_attach failed: %d\n", err);
> -			apc->priv_flags = old_priv_flags;
> -		}
> +	err = mana_publish_qset(apc, &newq, &oldq);
> +	if (err) {
> +		mana_free_qset(scratch, &newq);
> +		goto free_scratch;
>  	}
>  
> -out:
> -	mana_pre_dealloc_rxbufs(apc);
> +	mana_free_qset(scratch, &oldq);
> +
> +free_scratch:
> +	mana_publish_close_if_needed(apc);
> +	mana_qset_scratch_free(scratch);
>  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/cover.1791710735.git.weh@linux.microsoft.com?part=6