Re: [PATCH net-next v3 04/13] net: mana: swap queue sets in mana_set_ringparam
From: sashiko-bot@kernel.org
Date: 2026-09-02 01:45:39
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The patch introduces a boolean flag (`channel_changing`) into `mana_set_ringparam` to act as an ad-hoc lock against concurrent RDMA operations. The subsystem guidelines explicitly prohibit using a boolean flag set on entry and cleared on exit of a code region to prevent concurrent access, requiring a proper lock instead. Furthermore, this ad-hoc lock is entirely redundant and constitutes unnecessary code complexity: the RDMA path (`mana_cfg_vport`) already returns `-EBUSY` when `apc->vport_use_count > 0`. Because `mana_set_ringparam` returns early if the port is down, `vport_use_count` is guaranteed to be `> 0` when the flag is set (as the author notes in the code comment). This means the RDMA path will always short-circuit and fail before it even evaluates `apc->channel_changing`, making the flag updates dead code. -- commit ea4e4054a6be8a40258c13cd5a0aa9f21fa11e29 Author: Long Li [off-list ref] net: mana: swap queue sets in mana_set_ringparam Convert the ring size path to pre-allocate and swap so that allocation failures return an error without touching the running configuration. This drops the fallback on failure and returns early when the requested sizes round to the values already in use.
quoted hunk ↗ jump to hunk
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
[ ... ]
+ /* Block RDMA from acquiring the vport for the duration. The vport
+ * itself is never released, so vport_use_count stays > 0.
+ */
+ 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: Low] Does this introduce a boolean flag as an ad-hoc lock? Subsystem guidelines prohibit using a boolean flag set on entry and cleared on exit of a code region to prevent concurrent access, requiring a proper lock instead. Additionally, could this flag be entirely redundant? Since the RDMA path returns -EBUSY when apc->vport_use_count > 0, and the port is up here (meaning vport_use_count > 0, as noted in the comment), the RDMA path will likely short-circuit and fail before it even evaluates apc->channel_changing. Does this make the flag updates dead code? [ ... ]
+clear_flag: + mutex_lock(&apc->vport_mutex); + apc->channel_changing = false; + mutex_unlock(&apc->vport_mutex); return err; }
[Severity: Low] This clears the ad-hoc lock mentioned above. Can we remove this flag-based synchronization entirely if the RDMA path is already blocked by vport_use_count? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260901014442.2945689-1-longli@microsoft.com?part=4