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 <[email protected]> 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/[email protected]?part=4
