Re: [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin()

On Wed, Aug 12, 2026 at 2:27 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] `nvme_fc_ctrl_connectivity_loss()` clears the `NVME_CTRL_MARGINAL` bit on the wrong structure's flags field, causing state corruption and permanent marginal state.
> --
>
> --- Patch [9]: [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin() ---
> commit a1aaa3c652a2b842059c76da3884f9c0dcb9925a
> Author: Jesse Taube <[email protected]>
>
> nvme-fc: add nvme_fc_set_remoteport_fpin()
>
> Add nvme_fc_set_remoteport_fpin() and supporting functions. This
> function is called by the SCSI FC transport and driver layer to set or
> clear the 'marginal' path status for a specific rport.
>
> > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> > index d2ca65f70a0c5..4acf23ea8aea2 100644
> > --- a/drivers/nvme/host/fc.c
> > +++ b/drivers/nvme/host/fc.c
> > @@ -895,6 +895,22 @@ nvme_fc_set_remoteport_devloss(struct nvme_fc_remote_port *portptr,
> >  }
> >  EXPORT_SYMBOL_GPL(nvme_fc_set_remoteport_devloss);
> >
> > +void
> > +nvme_fc_set_remoteport_fpin(struct nvme_fc_remote_port *portptr, bool marginal)
> > +{
> > +     struct nvme_fc_rport *rport = remoteport_to_rport(portptr);
> > +     struct nvme_fc_ctrl *ctrl;
> > +
> > +     spin_lock_irq(&rport->lock);
> > +     list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> > +             if (marginal)
> > +                     set_bit(NVME_CTRL_MARGINAL, &ctrl->ctrl.flags);
> > +             else
> > +                     clear_bit(NVME_CTRL_MARGINAL, &ctrl->ctrl.flags);
>
> [Severity: High]
> This code correctly uses &ctrl->ctrl.flags for NVME_CTRL_MARGINAL. However, in
> nvme_fc_ctrl_connectivity_loss() introduced in the earlier commit "nvme: add
> NVME_CTRL_MARGINAL flag", the marginal state is cleared like this:
>
> drivers/nvme/host/fc.c:nvme_fc_ctrl_connectivity_loss() {
>         ...
>         /* clear 'marginal' flag as controller will be reset */
>         clear_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
>         ...
> }
>
> Could clearing NVME_CTRL_MARGINAL on the transport-specific &ctrl->flags
> instead of &ctrl->ctrl.flags leave the NVMe controller permanently marked as
> marginal across reconnects, preventing the multipath scheduler from fully
> utilizing the repaired path?

Will fix, same issue brought up earlier.

>
> > +     }
> > +     spin_unlock_irq(&rport->lock);
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
>