> 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 <[email protected]>
>
> 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?
The flag is not introduced by this patch. channel_changing is already in
mainline: declared in struct mana_port_context, checked inside
mana_cfg_vport() via its check_channel_changing argument, and
set/cleared by mana_set_channels(), mana_set_priv_flags() and the
per-port reset work handler. This patch converts mana_set_ringparam() to
the swap model and follows the convention the neighbouring ethtool
setters already use.
It is also not dead code. The reasoning holds for the success path:
while the swap succeeds the vport is never released, vport_use_count
stays at 1, and mana_cfg_vport() short-circuits on the first disjunct
exactly as you describe. The failure path releases it:
free_scratch:
mana_publish_close_if_needed(apc); /* -> mana_dealloc_queues()
-> mana_destroy_vport()
-> mana_uncfg_vport(): 1 -> 0 */
mana_qset_scratch_free(scratch);
clear_flag:
apc->channel_changing = false;
When mana_publish_qset() hits a dead end the port is closed, which drops
vport_use_count to 0, and the flag is only cleared after that. In that
window channel_changing is the only thing still blocking RDMA, which
calls mana_cfg_vport(..., true) from drivers/infiniband/hw/mana/main.c
without RTNL and can therefore land in it. The same applies to the reset
work handler, which this series does not convert and which still does a
full detach/attach.
Thanks,
Long
