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

[Long Li <longli@microsoft.com>]

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

This locking pattern has existed in the base code for a long time; this is not introduced by this patch.

This patch just uses this locking pattern for the vport changing states.


> 
> [ ... ]
> > +clear_flag:
> > +	mutex_lock(&apc->vport_mutex);
> > +	apc->channel_changing = false;
> > +	mutex_unlock(&apc->vport_mutex);
> >  	return err;
> >  }
> 
> --
> Sashiko AI
> review * https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fsashiko.dev%2F%23%2Fpatchset%2F20260908032843.397667-1-
> longli%40microsoft.com%3Fpart%3D4&data=05%7C02%7Clongli%40microsoft.
> com%7Cc67f289fbf6d44697aa108df0e229871%7C72f988bf86f141af91ab2d7cd
> 011db47%7C1%7C0%7C639245213896893836%7CUnknown%7CTWFpbGZsb3
> d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> joiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=bb9IsrgM5js%2BYCF
> uaioyc5OMJmhNDee3ut65iShCesk%3D&reserved=0