Re: [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag

On Wed, Aug 12, 2026 at 2:26 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The NVME_CTRL_MARGINAL flag is cleared on the wrong structure's flags field in the Fibre Channel transport.
> - [Medium] The NVME_CTRL_MARGINAL flag is not cleared universally on all controller resets, leaving non-FC transports and manual resets permanently in a marginal state.
> --
>
> --- Patch [2]: [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag ---
> commit f9c336bbdc7993d3daa484b869cd6845d7fce7a7
> Author: Bryan Gurney <[email protected]>
>
> nvme: add NVME_CTRL_MARGINAL flag
>
> Add a new controller flag, NVME_CTRL_MARGINAL, to help multipath I/O
> policies to react to a path that is set to a "marginal" state.
> The flag is cleared on controller reset, which is often the case when
> faulty cabling or transceiver hardware is replaced.
>
> [ ... ]
>
> > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> > index 04363b9c4489e..d2ca65f70a0c5 100644
> > --- a/drivers/nvme/host/fc.c
> > +++ b/drivers/nvme/host/fc.c
> > @@ -788,6 +788,10 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl)
> >               "Reconnect", ctrl->cnum);
> >
> >       set_bit(ASSOC_FAILED, &ctrl->flags);
> > +
> > +     /* clear 'marginal' flag as controller will be reset */
> > +     clear_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
>
> [Severity: High]
> Does this clear the flag on the wrong structure?

Yes, Will fix.

>
> Here, ctrl is a pointer to struct nvme_fc_ctrl. Clearing the bit on
> &ctrl->flags will affect the FC-specific flags field instead of the intended
> core NVMe controller flags (&ctrl->ctrl.flags).
>
> [Severity: Medium]
> Will other transports remain in a marginal state indefinitely?

Yes, this is intended it wont affect their functionality.

> The commit message mentions the flag is cleared on controller reset, but the
> clearing logic is only added here in the FC-specific connectivity loss handler.
>
> Should this flag instead be cleared universally inside nvme_reset_ctrl() in
> drivers/nvme/host/core.c to ensure PCIe, TCP, and RDMA paths also recover
> after a reset?
>
> drivers/nvme/host/core.c:nvme_reset_ctrl() {
>     if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>         return -EBUSY;
>     ...
>
> > +
> >       nvme_reset_ctrl(&ctrl->ctrl);
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
>