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

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?

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?

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