Re: [PATCH 2/3] nvmet: pci-epf: Clear CC and CSTS when disabling the controller

[Niklas Cassel <cassel@kernel.org>]

On Tue, Apr 08, 2025 at 11:47:32AM +0900, Damien Le Moal wrote:
> When a host shuts down the controller when shutting down but does so
> without first disabling the controller, the enable bit remains set in
> the controller configuration register. When the host restarts and
> attempts to enable the controller again, the
> nvmet_pci_epf_poll_cc_work() function is unable to detect the change
> from 0 to 1 of the enable bit, and thus the controller is not enabled
> again, which result in a device scan timeout on the host. This problem
> also occurs if the host shuts down uncleanly or if the PCIe link goes
> down: as the CC.EN value is not reset, the controller is not enabled
> again when the host restarts.
> 
> Fix this by introducing the function nvmet_pci_epf_clear_ctrl_config()
> to clear the CC and CSTS registers of the controller when the PCIe link
> is lost (nvmet_pci_epf_stop_ctrl() function), and when starting the

s/, and when/, or when/


> controller fails (nvmet_pci_epf_enable_ctrl() fails). Also use this
> function in nvmet_pci_epf_init_bar() to simplify the initialization of
> the CC and CSTS registers.
> 
> Furthermore, modify the function nvmet_pci_epf_disable_ctrl() to clear
> the CC.EN bit and write this updated value to the BAR register when the
> controller is shutdown by the host, to ensure that upon restart, we can
> detect the host setting CC.EN.
> 
> Fixes: 0faa0fe6f90e ("nvmet: New NVMe PCI endpoint function target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/nvme/target/pci-epf.c | 49 +++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvme/target/pci-epf.c b/drivers/nvme/target/pci-epf.c
> index f6b22ef4c267..f18faf407eab 100644
> --- a/drivers/nvme/target/pci-epf.c
> +++ b/drivers/nvme/target/pci-epf.c
> @@ -1802,6 +1802,21 @@ static void nvmet_pci_epf_cq_work(struct work_struct *work)
>  				   NVMET_PCI_EPF_CQ_RETRY_INTERVAL);
>  }
>  
> +static void nvmet_pci_epf_clear_ctrl_config(struct nvmet_pci_epf_ctrl *ctrl)
> +{
> +	struct nvmet_ctrl *tctrl = ctrl->tctrl;
> +
> +	/* Initialize controller status. */
> +	tctrl->csts = 0;
> +	ctrl->csts = 0;

This can be written on one line:
tctrl->csts = ctrl->csts = 0;


> +	nvmet_pci_epf_bar_write32(ctrl, NVME_REG_CSTS, ctrl->csts);
> +
> +	/* Initialize controller configuration and start polling. */
> +	tctrl->cc = 0;
> +	ctrl->cc = 0;

Same here.


Otherwise, this looks good to me:
Reviewed-by: Niklas Cassel <cassel@kernel.org>