Re: [PATCH v3 08/14] lpfc: Improve PLOGI retry handling for large SAN configurations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Non-atomic test-and-set and post-submission setting of NLP_PLOGI_SND causes permanent target block or bypassed serialization.
- [High] New state validation check in `lpfc_issue_els_plogi` silently drops legitimate PLOGIs, stalling discovery.
- [High] TOCTOU race in delayed PLOGI timer and unreg completion permanently drops deferred PLOGIs.
- [Medium] The `NLP_PLOGI_SND` flag is completely bypassed when a PLOGI IOCB is queued for retry upon `IOCB_FAILED_PUT`.
--

--- Patch [8]: [PATCH v3 08/14] lpfc: Improve PLOGI retry handling for large SAN configurations ---
commit 1ce21a9bd9de0c81a785643032705d301dd6dccb
Author: Justin Tee <justintee8345@gmail.com>
Date:   Tue Jul 14 01:18:06 2026 +0000

lpfc: Improve PLOGI retry handling for large SAN configurations

In large SAN configurations with link perturbations, rediscovery of target
ports is problematic due to PLOGI retry race conditions.

This patch improves target rediscovery by ensuring PLOGI retries are
serialized in unregistration and retry handler paths.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 6720fd5ec5232..16b10377a0953 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -2329,17 +2331,43 @@ lpfc_issue_els_plogi(struct lpfc_vport *vport, uint32_t did, uint8_t retry)
[ ... ]
> +	if (test_bit(NLP_PLOGI_SND, &ndlp->nlp_flag)) {

[Severity: Critical]
Can NLP_PLOGI_SND become permanently stuck?

The flag is tested here, but it is not set until after the I/O is submitted
to the HBA at the end of the function. If the hardware completes the PLOGI
quickly, the completion handler lpfc_cmpl_els_plogi() might run before the
issuing thread sets the bit.

The completion handler would clear the bit, achieving nothing, and then the
issuing thread would execute set_bit(), leaving the flag permanently set.

Could this permanently reject future PLOGIs? Would test_and_set_bit() be
more appropriate here to prevent this regression?

> +		lpfc_printf_vlog(vport, KERN_INFO,
> +				 LOG_ELS | LOG_NODE | LOG_DISCOVERY,
> +				 "4113 Reject PLOGI issue, PLOGI in-flight "
> +				 "x%px, DID x%x nflag x%lx\n",
> +				 ndlp, ndlp->nlp_DID, ndlp->nlp_flag);
> +		return 1;
> +	}
> +
> +	if (ndlp->nlp_state > NLP_STE_PLOGI_ISSUE &&
> +	    ndlp->nlp_state <= NLP_STE_MAPPED_NODE) {

[Severity: High]
Does this block legitimate PLOGIs during discovery?

For example, lpfc_rcv_plogi_adisc_issue() calls lpfc_issue_els_plogi() while
the node is in NLP_STE_ADISC_ISSUE. The PLOGI would be silently rejected
here, but the caller proceeds to force the state to NLP_STE_PLOGI_ISSUE
anyway.

Could this cause the port state machine to wait forever for a PLOGI
completion that will never arrive?

> +		lpfc_printf_vlog(vport, KERN_INFO,
> +				 LOG_ELS | LOG_NODE | LOG_DISCOVERY,
> +				 "4114 Reject PLOGI issue, Node in "
> +				 "unexpected state x%px, DID x%x nflag x%lx "
> +				 "in State x%x\n",
> +				 ndlp, ndlp->nlp_DID,
> +				 ndlp->nlp_flag, ndlp->nlp_state);
> +		return 1;
>  	}
>  
>  	cmdsize = (sizeof(uint32_t) + sizeof(struct serv_parm));
> @@ -2415,6 +2443,7 @@ lpfc_issue_els_plogi(struct lpfc_vport *vport, uint32_t did, uint8_t retry)
[ ... ]
>  		/* Under heavy vpi counts, the driver's host_index can catch up
>  		 * to the hba_index causing a put error. Catch this case and
>  		 * put the IO on phba->txq.
>  		 */
>  		if (ret == IOCB_FAILED_PUT && phba->sli_rev == LPFC_SLI_REV4) {
>  			lpfc_sli4_queue_io_for_retry(phba, elsiocb, false);
>  			return 0;
>  		}

[Severity: Medium]
If the IO is queued for retry here and returns early, doesn't it bypass
setting NLP_PLOGI_SND at the bottom of this function?

Since the IO is queued but the flag isn't set, wouldn't concurrent callers
pass the test_bit() check and issue duplicate PLOGIs, bypassing the
intended serialization?

>  		lpfc_els_free_iocb(phba, elsiocb);
>  		lpfc_nlp_put(ndlp);
>  		return 1;
>  	}
>  
> +	set_bit(NLP_PLOGI_SND, &ndlp->nlp_flag);
>  	return 0;
>  }
>  
> @@ -4614,6 +4643,31 @@ lpfc_els_retry_delay_handler(struct lpfc_nodelist *ndlp)
[ ... ]
>  	case ELS_CMD_PLOGI:
> +		/* The driver delayed a PLOGI via the nlp_delayfunc, but
> +		 * it's possible the PLOGI is already on a deferred retry.
> +		 * Catch this case and skip this delayed PLOGI. This prevents
> +		 * multiple PLOGIs in flight. The defer code flow cleans
> +		 * up.
> +		 */
> +		if ((test_bit(NLP_IGNR_REG_CMPL, &ndlp->nlp_flag) ||
> +		     test_bit(NLP_UNREG_INP, &ndlp->nlp_flag)) &&

[Severity: High]
Is there a race condition here with the unreg completion interrupt?

In lpfc_els_retry_delay_handler(), nlp_last_elscmd is temporarily cleared
before reaching this point. If this timer thread decides to skip issuing the
PLOGI because NLP_UNREG_INP is set:

CPU1 (lpfc_els_retry_delay_handler)
    cmd = ndlp->nlp_last_elscmd;
    ndlp->nlp_last_elscmd = 0;
    ...
    if (... test_bit(NLP_UNREG_INP, &ndlp->nlp_flag) ...) {

If CPU2 processes the unreg completion interrupt at this exact moment:

CPU2 (lpfc_sli4_unreg_rpi_cmpl_clr)
    if (!test_bit(NLP_DELAY_TMO, &ndlp->nlp_flag) &&
        ndlp->nlp_last_elscmd == ELS_CMD_PLOGI) {

CPU2 will see nlp_last_elscmd as 0 and skip issuing the PLOGI.

Then CPU1 resumes:

CPU1
    ndlp->nlp_last_elscmd = ELS_CMD_PLOGI;
    break;

Could this result in both paths skipping the PLOGI, permanently stalling
recovery?

> +		    ndlp->nlp_defer_did != NLP_EVT_NOTHING_PENDING &&
> +		    ((ndlp->nlp_DID & Fabric_DID_MASK) != Fabric_DID_MASK) &&
> +		    !test_bit(FC_OFFLINE_MODE, &vport->fc_flag)) {
> +			/* When UNREG_RPI completes we need to have the
> +			 * nlp_last_elscmd set.
> +			 */
> +			ndlp->nlp_last_elscmd = ELS_CMD_PLOGI;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714011812.106753-1-justintee8345@gmail.com?part=8