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

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?

> +	}
> +	spin_unlock_irq(&rport->lock);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9