Re: [PATCH v2 3/3] scsi: lpfc: Return MBX status when a non-polled mbox cannot be issued

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: lpfc: wrong MBX status in lpfc_sli_issue_mbox_s4() for
  MBX_NOWAIT
- [Low] scsi: lpfc: missing Fixes trailer for commit da0436e915a5
--

commit f038148a081f8cf95b1d6a94df61cce938e8192e
Author: Wentao Liang <vulab@iscas.ac.cn>

> scsi: lpfc: Return MBX status when a non-polled mbox cannot be issued
>
> lpfc_sli_issue_mbox_s4() reports a failure to hand a command to the port
> with an MBX status code in every path except one: when interrupts are not
> enabled and the caller asked for MBX_NOWAIT, it returns -EIO.  A negative
> errno does not belong in that code space, and a caller has no MBX value to
> test for this outcome.
>
> Return MBXERR_ERROR, as lpfc_sli4_post_sync_mbox() already does for a
> command it cannot post.  lpfc_issue_lip() maps MBXERR_ERROR to -EIO, and
> the bsg mailbox path now converts a failure of its own, so no positive
> status is handed to a caller that expects an errno.
>
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>

[Severity: Low]
This fixes the -EIO return value introduced with SLI-4 mailbox support.
Should it include a Fixes tag?

  Fixes: da0436e915a5 ("[SCSI] lpfc 8.3.2 : Addition of SLI4 Interface - Base Support")

> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 715b328d3d184..05a76b00702a8 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -10121,7 +10121,7 @@ lpfc_sli_issue_mbox_s4(struct lpfc_hba *phba, LPFC_MBOXQ_t *mboxq,
>  		if (flag == MBX_POLL)
>  			rc = lpfc_sli4_post_sync_mbox(phba, mboxq);
>  		else
> -			rc = -EIO;
> +			rc = MBXERR_ERROR;
>  		if (rc != MBX_SUCCESS)
>  			lpfc_printf_log(phba, KERN_WARNING, LOG_MBOX | LOG_SLI,
>  					"(%d):2541 Mailbox command x%x "

[Severity: High]
Should this return MBX_NOT_FINISHED instead of MBXERR_ERROR when an
MBX_NOWAIT command cannot be issued?

All other pre-enqueue failure paths for asynchronous commands in
lpfc_sli_issue_mbox_s4() (and the equivalent interrupt-disabled check in
lpfc_sli_issue_mbox_s3()) return MBX_NOT_FINISHED, whereas MBXERR_ERROR is
used for synchronous MBX_POLL commands in lpfc_sli4_post_sync_mbox().
Callers of lpfc_sli_issue_mbox() with MBX_NOWAIT check specifically for
MBX_NOT_FINISHED to detect that the command was not queued:

drivers/scsi/lpfc/lpfc_sli.c:lpfc_sli4_resume_rpi() {
	...
	rc = lpfc_sli_issue_mbox(phba, mboxq, MBX_NOWAIT);
	if (rc == MBX_NOT_FINISHED) {
		...
		lpfc_nlp_put(ndlp);
		mempool_free(mboxq, phba->mbox_mem_pool);
		return -EIO;
	}
	return 0;
}

When interrupts are disabled (for example after lpfc_stop_port_s4()) and
MBXERR_ERROR is returned, callers treat the unqueued command as accepted.
Because its completion handler never runs, mboxq, DMA buffers, and ndlp
references are leaked, and callers such as lpfc_get_sfp_info_wait() via
sysfs lpfc_xcvr_data treat the unposted zeroed mailbox as having succeeded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107161655.2628796-1-vulab@iscas.ac.cn?part=3