Re: [PATCH v3 11/14] lpfc: Put iocbq on phba->txq when ELS WQ is full or ELS SGL unavailable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incomplete refactoring of __lpfc_sli_issue_iocb_s4 return codes causes unpatched callers to leak IOCBs.
- [High] Early returns on IOCB_FAILED_PUT in patched callers bypass critical state machine updates and counter increments.
- [Medium] The logic in lpfc_drain_txq to queue an IOCB to the head of the txq when the SGL pool is empty is dead code.
--

--- Patch [11]: [PATCH v3 11/14] lpfc: Put iocbq on phba->txq when ELS WQ is full or ELS SGL unavailable ---
commit e664b47bccd196f6d682ce56b8b0fe9c32e4c4a8
Author: Justin Tee <justintee8345@gmail.com>

lpfc: Put iocbq on phba->txq when ELS WQ is full or ELS SGL unavailable

When ELS/CT commands can't be sent due to ELS WQ full, queue the iocbq on
the phba->txq tail for retrying submission later in lpfc_drain_txq.
lpfc_drain_txq is also updated to queue an iocbq back on to the head of
phba->txq when observing ELS WQ full or SGL unavailable events.

> 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
> @@ -2766,7 +2780,21 @@ lpfc_issue_els_prli(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp,
>  	}
>  
>  	rc = lpfc_sli_issue_iocb(phba, LPFC_ELS_RING, elsiocb, 0);
> -	if (rc == IOCB_ERROR) {
> +	if (rc) {
> +		lpfc_vlog_msg(vport, KERN_NOTICE,
> +			      LOG_ELS | LOG_DISCOVERY | LOG_NODE,
> +			      "0155 PRLI WQE Put returned %d\n",
> +			      rc);
> +
> +		/* 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 (rc == IOCB_FAILED_PUT && phba->sli_rev == LPFC_SLI_REV4) {
> +			lpfc_sli4_queue_io_for_retry(phba, elsiocb, false);
> +			return 0;
> +		}

[Severity: High]
Does this early return bypass state machine updates?

By returning 0 here, the function skips setting the NLP_PRLI_SND flag,
incrementing the outstanding PRLI counters (vport->fc_prli_sent and
ndlp->fc4_prli_sent), and looping to send the NVMe PRLI.

When the retried IO eventually completes, the completion handler could decrement
counters that were never incremented, potentially causing an integer underflow
that breaks lpfc_scan_finished(). Also, it appears the second PRLI for NVMe
targets would never be sent.

> +
>  		lpfc_els_free_iocb(phba, elsiocb);
>  		lpfc_nlp_put(ndlp);
>  		return 1;
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -10793,12 +10839,18 @@ __lpfc_sli_issue_iocb_s4(struct lpfc_hba *phba, uint32_t ring_number,
>  			return IOCB_ERROR;
>  	}
>  
> -	if (lpfc_sli4_wq_put(wq, wqe))
> -		return IOCB_ERROR;
> + retry_io:
> +	piocb->cmd_flag &= ~LPFC_IO_IN_RETRY;
>  
> -	lpfc_sli_ringtxcmpl_put(phba, pring, piocb);
> +	/* Push the wqe to the wq.  If the push fails with -EBUSY it means
> +	 * the WQ is full.  Pass this status back to enable a retry.
> +	 */
> +	rc = lpfc_sli4_wq_put(wq, wqe);
> +	if (rc == -EBUSY)
> +		return IOCB_FAILED_PUT;
>  
> -	return 0;
> +	lpfc_sli_ringtxcmpl_put(phba, pring, piocb);
> +	return rc;
>  }

[Severity: High]
Will unpatched callers leak resources by treating IOCB_FAILED_PUT as success?

Several existing ELS/CT callers, such as lpfc_issue_els_logo() and
lpfc_issue_els_adisc(), explicitly check if the return value is exactly
IOCB_ERROR:

drivers/scsi/lpfc/lpfc_els.c:lpfc_issue_els_logo() {
	...
	rc = lpfc_sli_issue_iocb(phba, LPFC_ELS_RING, elsiocb, 0);
	if (rc == IOCB_ERROR) {
		lpfc_els_free_iocb(phba, elsiocb);
		...
	}
	...
}

Because IOCB_FAILED_PUT (7) is not equal to IOCB_ERROR (1), these functions
might treat the failure as a successful submission. Could this cause the driver
to permanently leak the IOCB buffer/SGL/DMA mappings and hang the ELS state
machine?

> @@ -21277,7 +21329,7 @@ lpfc_drain_txq(struct lpfc_hba *phba)
>  
>  		ret = __lpfc_sli_issue_iocb(phba, pring->ringno, piocbq, 0);

[ ... ]

> +		if (txq_cnt == 0 || ret == IOCB_BUSY ||
> +		    ret == IOCB_FAILED_PUT) {
> +			/* If ELS WQ was full (IOCB_FAILED_PUT), then an SGL has
> +			 * been assigned.  If SGL pool was empty (IOCB_BUSY),
> +			 * then all we need is to put back on phba->txq to try
> +			 * again later.
> +			 */

[ ... ]

> +			switch (ret) {
> +			case IOCB_FAILED_PUT:
> +				piocbq->cmd_flag |= LPFC_IO_IN_RETRY;
> +				__lpfc_sli_ringtx_put(phba, pring, piocbq,
> +						      true);
> +				break;
> +			case IOCB_BUSY:
> +				__lpfc_sli_ringtx_put(phba, pring, piocbq,
> +						      true);
> +				break;

[Severity: Medium]
Is this IOCB_BUSY case dead code?

The call to __lpfc_sli_issue_iocb() above passes 0 for the flag argument.
If an SGLQ cannot be allocated, __lpfc_sli_issue_iocb_s4() evaluates:

	if (!(flag & SLI_IOCB_RET_IOCB)) {
		__lpfc_sli_ringtx_put(phba, pring, piocb, false);
		return IOCB_SUCCESS;
	} else {
		return IOCB_BUSY;
	}

Since flag is 0, it silently requeues the IO to the tail of txq and returns
IOCB_SUCCESS (0). Therefore, lpfc_drain_txq() would never receive IOCB_BUSY,
and the intended optimization to queue the IOCB to the head when the SGL pool
is empty would be bypassed.

> +			}
> +			spin_unlock_irqrestore(&pring->ring_lock, iflags);
>  			break;
> +		}
> +
> +		spin_unlock_irqrestore(&pring->ring_lock, iflags);
>  	}

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