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

[sashiko-bot@kernel.org]

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 <longli@microsoft.com>

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.

> 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