Re: [PATCH v3 05/14] lpfc: Add handling for when PLOGI or PRLI is dropped during link failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The vport-wide counter fc_prli_sent is modified under a node-specific spinlock (ndlp->lock), leading to data races during concurrent PRLI operations.
- [High] The patch only cleans up state flags for PLOGI and PRLI in the link down completion handler, completely missing identical cleanup requirements for LOGO and ADISC, which causes the session to remain irrecoverably stuck.
--

--- Patch [5]: [PATCH v3 05/14] lpfc: Add handling for when PLOGI or PRLI is dropped during link failure ---
commit ef308ec0ae24b6ee74db89f761bac77e41718c55
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Add handling for when PLOGI or PRLI is dropped during link failure

This commit modifies the link down completion handler to clear the 
NLP_PLOGI_SND and NLP_PRLI_SND flags and decrement the fc_prli_sent 
counters. This ensures that normal cleanup actions are not missed when 
PLOGI or PRLI commands are aborted due to a link failure. The fix 
helps prevent recovery failures for login sessions.

> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c

[ ... ]

> @@ -1237,17 +1239,40 @@ lpfc_cmpl_els_link_down(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>  	ulp_status = get_job_ulpstatus(phba, rspiocb);
>  	ulp_word4 = get_job_word4(phba, rspiocb);
>  

[ ... ]

> +	if (cmd == ELS_CMD_PLOGI) {
> +		/* A PLOGI ELS IO needs to clear the PLOGI_SND flag to
> +		 * acknowledge the ELS completion and allow recovery. Otherwise
> +		 * a subsequent PLOGI gets rejected as a duplicate.
> +		 */
> +		clear_bit(NLP_PLOGI_SND, &ndlp->nlp_flag);
> +	} else if (cmd == ELS_CMD_PRLI || cmd == ELS_CMD_NVMEPRLI) {

[Severity: High]
This isn't a bug introduced by this patch, but does this handler also need to
clean up state flags for LOGO and ADISC?

If a LOGO or ADISC ELS command is aborted by a link down event and routed to
lpfc_cmpl_els_link_down(), it appears the NLP_LOGO_SND and NLP_ADISC_SND
flags are not cleared.

Could this permanently block subsequent connection or management commands to
this node? For example, would lpfc_rcv_plogi() silently ignore incoming PLOGIs 
if NLP_LOGO_SND remains set?

> +		/* A PRLI ELS IO needs to decrement the fc4_prli_sent count
> +		 * added by the lpfc_issue_els_prli function.  A nonzero count
> +		 * stops transport registrations.
> +		 */
> +		clear_bit(NLP_PRLI_SND, &ndlp->nlp_flag);
> +		spin_lock_irq(&ndlp->lock);
> +		vport->fc_prli_sent--;
> +		ndlp->fc4_prli_sent--;
> +		spin_unlock_irq(&ndlp->lock);

[Severity: High]
This is a pre-existing issue, but can modifying the shared vport-wide counter
vport->fc_prli_sent under a localized node-specific lock (ndlp->lock) lead to
data races?

If concurrent ELS PRLI completions or issues on different nodes within the same
virtual port occur, each thread might acquire its own localized ndlp->lock and
perform a read-modify-write on vport->fc_prli_sent without mutual exclusion.

Could this lost update leave the counter permanently non-zero, causing
lpfc_scan_finished() to hang indefinitely and block SCSI device discovery?

>  	}
>  
>  	if (cmdiocb->cmd_flag & LPFC_IO_FABRIC) {

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