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