Re: [PATCH v4 35/56] scsi: qla2xxx: Update VP control IOCB handling for 29xx series

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The completion path for VP control commands drops error statuses from timeouts and hardware errors, falsely reporting success to the caller.
--

--- Patch [35]: [PATCH v4 35/56] scsi: qla2xxx: Update VP control IOCB handling for 29xx series ---
commit 77dc435924819ef08c343b24f1c7855d5ea36176
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Update VP control IOCB handling for 29xx series

Update VP control IOCB command and response handling to support the
29xx series adapters, which use the 128-byte vp_ctrl_entry_24xx_ext
layout.

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 774ccf6b2adc8..7d2b6d135dc84 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -3000,13 +3000,19 @@ static void qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
>  }
>  
>  static void qla_ctrlvp_completed(scsi_qla_host_t *vha, struct req_que *req,
> -    struct vp_ctrl_entry_24xx *vce)
> +				 void *pkt)
>  {
>  	const char func[] = "CTRLVP-IOCB";
> +	/*
> +	 * vp_ctrl_entry_24xx_ext overlays vp_ctrl_entry_24xx for all
> +	 * fields read here (entry_status, comp_status, vp_idx_failed),
> +	 * so the read goes through one struct vp_ctrl_entry_24xx * view.
> +	 */
> +	struct vp_ctrl_entry_24xx *vce = pkt;
>  	srb_t *sp;
>  	int rval = QLA_SUCCESS;
>  
> -	sp = qla2x00_get_sp_from_handle(vha, func, req, vce);
> +	sp = qla2x00_get_sp_from_handle(vha, func, req, pkt);
>  	if (!sp)
>  		return;

[Severity: High]
This is a pre-existing issue, but is the error handling in this function
actually reachable?

In qla24xx_process_response_queue(), any IOCB response with a non-zero
entry_status is intercepted and routed to qla2x00_error_entry() instead of
falling through to the normal completion paths:

	if (((response_t *)pkt)->entry_status != 0) {
		if (qla2x00_error_entry(vha, rsp, (sts_entry_t *)pkt))
			goto process_err;

		((response_t *)pkt)->signature = RESPONSE_PROCESSED;
		wmb();
		continue;
	}

Since qla2x00_error_entry() returns 0 for VP_CTRL_IOCB_TYPE, the processing
loop skips the process_err label and bypasses qla_ctrlvp_completed()
entirely. Does this mean the vce->entry_status != 0 check further down in
this function is dead code?

Furthermore, when qla2x00_error_entry() or a timeout path invokes the
completion callback qla_ctrlvp_sp_done(), the callback ignores its res
parameter:

static void qla_ctrlvp_sp_done(srb_t *sp, int res)
{
	if (sp->comp)
		complete(sp->comp);
	/* don't free sp here. Let the caller do the free */
}

Because sp->rc is never updated and remains zero (success), wouldn't
qla24xx_control_vp() erroneously assume the hardware command succeeded even
if it failed?

Could this cause a state desync where the driver and firmware disagree on
whether a virtual port is enabled or disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714095353.289460-1-njavali@marvell.com?part=35