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