[PATCH 1/1] nvmet-fcloop: Check remoteport port_state before calling done callback

[PATCH 1/1] nvmet-fcloop: Check remoteport port_state before calling done callback

[Justin Tee]

In nvme_fc_handle_ls_rqst_work, the lsrsp->done callback is only set when
remoteport->port_state is FC_OBJSTATE_ONLINE.  Otherwise, the
nvme_fc_xmt_ls_rsp's LLDD call to lport->ops->xmt_ls_rsp is expected to
fail and the nvme-fc transport layer itself will directly call
nvme_fc_xmt_ls_rsp_free instead of relying on LLDD's done callback to free
the lsrsp resources.

Update the fcloop_t2h_xmt_ls_rsp routine to check remoteport->port_state.
If online, then lsrsp->done callback will free the lsrsp.  Else, return
-ENODEV to signal the nvme-fc transport to handle freeing lsrsp.

Cc: Aristeu Rozanski <aris@redhat.com>
Cc: Daniel Wagner <wagi@kernel.org>
Cc: Ewan D. Milne <emilne@redhat.com>
Closes: https://lore.kernel.org/linux-nvme/21255200-a271-4fa0-b099-97755c8acd4c@work/
Fixes: 10c165af35d2 ("nvmet-fcloop: call done callback even when remote port is gone")
Signed-off-by: Justin Tee <justintee8345@gmail.com>
---
 drivers/nvme/target/fcloop.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index c30e9a3e014f..38bd2db3d6bb 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -491,6 +491,7 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
 	struct fcloop_rport *rport = remoteport->private;
 	struct nvmet_fc_target_port *targetport = rport->targetport;
 	struct fcloop_tport *tport;
+	int ret = 0;
 
 	if (!targetport) {
 		/*
@@ -500,12 +501,18 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
 		 * We end up here from delete association exchange:
 		 * nvmet_fc_xmt_disconnect_assoc sends an async request.
 		 *
-		 * Return success because this is what LLDDs do; silently
-		 * drop the response.
+		 * Return success when remoteport is still online because this
+		 * is what LLDDs do and silently drop the response.  Otherwise,
+		 * return with error to signal upper layer to perform the lsrsp
+		 * resource cleanup.
 		 */
-		lsrsp->done(lsrsp);
+		if (remoteport->port_state == FC_OBJSTATE_ONLINE)
+			lsrsp->done(lsrsp);
+		else
+			ret = -ENODEV;
+
 		kmem_cache_free(lsreq_cache, tls_req);
-		return 0;
+		return ret;
 	}
 
 	memcpy(lsreq->rspaddr, lsrsp->rspbuf,
-- 
2.38.0

Re: [PATCH 1/1] nvmet-fcloop: Check remoteport port_state before calling done callback

[Daniel Wagner]

On Thu, Dec 04, 2025 at 12:26:13PM -0800, Justin Tee wrote:
> -		 * Return success because this is what LLDDs do; silently
> -		 * drop the response.
> +		 * Return success when remoteport is still online because this
> +		 * is what LLDDs do and silently drop the response.  Otherwise,
> +		 * return with error to signal upper layer to perform the lsrsp
> +		 * resource cleanup.
>  		 */
> -		lsrsp->done(lsrsp);
> +		if (remoteport->port_state == FC_OBJSTATE_ONLINE)
> +			lsrsp->done(lsrsp);
> +		else
> +			ret = -ENODEV;

Is this what the hardware/driver would do as well? Originally, when
adding this code it return always an error and James told me that this
is not how the hardware/driver work. Though this was without checking
the port_state.

That said, I am fine doing this though, as it clearly fixes a problem.
Just wondering how far we can deviate from the 'real' thing.

In other words:

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 1/1] nvmet-fcloop: Check remoteport port_state before calling done callback

[Aristeu Rozanski]

On Thu, Dec 04, 2025 at 12:26:13PM -0800, Justin Tee wrote:
> In nvme_fc_handle_ls_rqst_work, the lsrsp->done callback is only set when
> remoteport->port_state is FC_OBJSTATE_ONLINE.  Otherwise, the
> nvme_fc_xmt_ls_rsp's LLDD call to lport->ops->xmt_ls_rsp is expected to
> fail and the nvme-fc transport layer itself will directly call
> nvme_fc_xmt_ls_rsp_free instead of relying on LLDD's done callback to free
> the lsrsp resources.
> 
> Update the fcloop_t2h_xmt_ls_rsp routine to check remoteport->port_state.
> If online, then lsrsp->done callback will free the lsrsp.  Else, return
> -ENODEV to signal the nvme-fc transport to handle freeing lsrsp.
> 
> Cc: Aristeu Rozanski <aris@redhat.com>
> Cc: Daniel Wagner <wagi@kernel.org>
> Cc: Ewan D. Milne <emilne@redhat.com>
> Closes: https://lore.kernel.org/linux-nvme/21255200-a271-4fa0-b099-97755c8acd4c@work/
> Fixes: 10c165af35d2 ("nvmet-fcloop: call done callback even when remote port is gone")
> Signed-off-by: Justin Tee <justintee8345@gmail.com>
> ---
>  drivers/nvme/target/fcloop.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index c30e9a3e014f..38bd2db3d6bb 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -491,6 +491,7 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
>  	struct fcloop_rport *rport = remoteport->private;
>  	struct nvmet_fc_target_port *targetport = rport->targetport;
>  	struct fcloop_tport *tport;
> +	int ret = 0;
>  
>  	if (!targetport) {
>  		/*
> @@ -500,12 +501,18 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
>  		 * We end up here from delete association exchange:
>  		 * nvmet_fc_xmt_disconnect_assoc sends an async request.
>  		 *
> -		 * Return success because this is what LLDDs do; silently
> -		 * drop the response.
> +		 * Return success when remoteport is still online because this
> +		 * is what LLDDs do and silently drop the response.  Otherwise,
> +		 * return with error to signal upper layer to perform the lsrsp
> +		 * resource cleanup.
>  		 */
> -		lsrsp->done(lsrsp);
> +		if (remoteport->port_state == FC_OBJSTATE_ONLINE)
> +			lsrsp->done(lsrsp);
> +		else
> +			ret = -ENODEV;
> +
>  		kmem_cache_free(lsreq_cache, tls_req);
> -		return 0;
> +		return ret;
>  	}
>  
>  	memcpy(lsreq->rspaddr, lsrsp->rspbuf,

survived overnight here and makes sense to me. Thanks!

Tested-by: Aristeu Rozanski <aris@redhat.com>
Acked-by: Aristeu Rozanski <aris@redhat.com>

-- 
Aristeu

Re: [PATCH 1/1] nvmet-fcloop: Check remoteport port_state before calling done callback

[Justin Tee]

> Is this what the hardware/driver would do as well? Originally, when
> adding this code it return always an error and James told me that this
> is not how the hardware/driver work. Though this was without checking
> the port_state.
>
> That said, I am fine doing this though, as it clearly fixes a problem.
> Just wondering how far we can deviate from the 'real' thing.

Great question, and I can help explain from lpfc driver perspective
that I think this patch does not deviate from real FC LLDD behavior:

For the lpfc driver, two sanity checks are made:

1.) In  __lpfc_nvme_xmt_ls_rsp, axchg->state is checked to have to be
in LPFC_NVME_STE_LS_RCV.  Otherwise, non-zero error is returned back
to nvme_fc transport when .xmt_ls_rsp is called.

2.) LPFC_NVME_STE_LS_RCV could only be set for a remoteport that is
online and has completed PRLI-NVME.  This check is in
lpfc_nvme_unsol_ls_handler with log message id 6216 in lpfc driver,
which is part of the queue work item after the interrupt handler for
unsolicited NVME frames.  An ndlp’s state (lpfc’s object for a
remoteport) is checked before setting axchg->state to
LPFC_NVME_STE_LS_RCV.

Re: [PATCH 1/1] nvmet-fcloop: Check remoteport port_state before calling done callback

[Keith Busch]

On Thu, Dec 04, 2025 at 12:26:13PM -0800, Justin Tee wrote:
> In nvme_fc_handle_ls_rqst_work, the lsrsp->done callback is only set when
> remoteport->port_state is FC_OBJSTATE_ONLINE.  Otherwise, the
> nvme_fc_xmt_ls_rsp's LLDD call to lport->ops->xmt_ls_rsp is expected to
> fail and the nvme-fc transport layer itself will directly call
> nvme_fc_xmt_ls_rsp_free instead of relying on LLDD's done callback to free
> the lsrsp resources.
> 
> Update the fcloop_t2h_xmt_ls_rsp routine to check remoteport->port_state.
> If online, then lsrsp->done callback will free the lsrsp.  Else, return
> -ENODEV to signal the nvme-fc transport to handle freeing lsrsp.

Thanks, queued up for nvme-7.0.