Re: [PATCH v4 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 1 potential issue(s) to consider:
- [Medium] scsi: lpfc: txq list churning in lpfc_drain_txq due to missing SLI_IOCB_RET_IOCB flag
--

commit 8b96ef178c664b569b564d0486b1112a8fe88e10
Author: Nigel Kirkland <nkirkland2304@gmail.com>

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

This commit adds logic to queue ELS/CT commands to the head or tail of the
txq for retry when the ELS WQ is full or SGLs are unavailable. It updates
lpfc_drain_txq to handle these retry scenarios.

> 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
[ ... ]
> @@ -10819,12 +10866,19 @@ __lpfc_sli_issue_iocb_s4(struct lpfc_hba *phba, uint32_t ring_number,
>  			if (!sglq) {
>  				if (!(flag & SLI_IOCB_RET_IOCB)) {
>  					__lpfc_sli_ringtx_put(phba,
>  							pring,
> -							piocb);
> +							piocb, false);
>  					return IOCB_SUCCESS;
>  				} else {
>  					return IOCB_BUSY;
>  				}

[Severity: Medium]
If lpfc_drain_txq() passes 0 for the flag, won't it trigger this block?

When SLI_IOCB_RET_IOCB is omitted, this path places the IOCB at the tail of
the txq and returns IOCB_SUCCESS. This defeats the newly added back-off
mechanism in lpfc_drain_txq() which relies on receiving an IOCB_BUSY status
to execute its retry logic and push the failing IOCB to the head.

[ ... ]
> @@ -21301,12 +21355,15 @@ lpfc_drain_txq(struct lpfc_hba *phba)
>  		}
>  		txq_cnt--;
>  
> +		/* Capture the return values that indicate an error during IO
> +		 * submit and cannot be retried. Prefix message 2822.
> +		 */
>  		ret = __lpfc_sli_issue_iocb(phba, pring->ringno, piocbq, 0);

[Severity: Medium]
Does this missing flag cause txq list churning during resource exhaustion?

Because the flag is passed as 0 here (instead of SLI_IOCB_RET_IOCB), the
underlying __lpfc_sli_issue_iocb() call will not return IOCB_BUSY when SGL
resources are exhausted. Since IOCB_SUCCESS is returned instead, the loop
below won't break early and will continuously pop and push items back to the
tail of the txq.

> -
> -		if (ret && ret != IOCB_BUSY) {
> +		if (ret && ret != IOCB_BUSY && ret != IOCB_FAILED_PUT) {
>  			fail_msg = " - Cannot send IO ";
>  			piocbq->cmd_flag &= ~LPFC_DRIVER_ABORTED;
>  		}
> +
>  		if (fail_msg) {
>  			piocbq->cmd_flag |= LPFC_DRIVER_ABORTED;
>  			/* Failed means we can't issue and need to cancel */
> @@ -21319,9 +21376,35 @@ lpfc_drain_txq(struct lpfc_hba *phba)
>  			list_add_tail(&piocbq->list, &completions);
>  			fail_msg = NULL;
>  		}
> -		spin_unlock_irqrestore(&pring->ring_lock, iflags);
> -		if (txq_cnt == 0 || ret == IOCB_BUSY)
> +
> +		if (txq_cnt == 0 || ret == IOCB_BUSY ||
> +		    ret == IOCB_FAILED_PUT) {
> +			/* IOCB_FAILED_PUT is unique to SLI4 and means SGL/XRI
> +			 * resources are allocated.  For this case, set the
> +			 * in retry flag.  For SLI3 and 4, push the IO to the
> +			 * txq for retry.
> +			 */
[ ... ]

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