Re: [PATCH v5 32/56] scsi: qla2xxx: Enhance task management IOCB handling for 29xx series

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The async abort IOCB builder `qla24xx_abort_iocb()` uses the 64-byte `struct abort_entry_24xx` layout unconditionally without adapting for QLA29XX adapters, which require the 128-byte `struct abort_entry_24xx_ext` layout. In the 29xx extended layout, the `vp_index` field is at offset 18, whereas in the 24xx layout it is at offset 51. Since the request slot is zero-initialized and the function writes to offset 51, the 29xx firmware will read a `vp_index` of 0.
--

--- Patch [32]: [PATCH v5 32/56] scsi: qla2xxx: Enhance task management IOCB handling for 29xx series ---
commit 64d0e684253fddc727ea9ea4498791ce4527162b
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enhance task management IOCB handling for 29xx series

This commit updates the QLA2XXX task management IOCB builders to use the
extended tsk_mgmt_entry_ext structure for QLA29XX adapters. By checking
IS_QLA29XX(), it correctly shifts the vp_index field assignment to match
the 128-byte stride layout expected by the 29xx series firmware.

> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
[ ... ]
> @@ -2736,26 +2736,39 @@ qla24xx_tm_iocb(srb_t *sp, struct tsk_mgmt_entry *tsk)
[ ... ]
>  	tsk->control_flags = cpu_to_le32(flags);
> -	tsk->port_id[0] = fcport->d_id.b.al_pa;
> -	tsk->port_id[1] = fcport->d_id.b.area;
> -	tsk->port_id[2] = fcport->d_id.b.domain;
> -	tsk->vp_index = fcport->vha->vp_idx;
> +	if (IS_QLA29XX(ha)) {
> +		((struct tsk_mgmt_entry_ext *)pkt)->vp_index =
> +		    cpu_to_le16(fcport->vha->vp_idx);
> +	} else {
> +		tsk->port_id[0] = fcport->d_id.b.al_pa;
> +		tsk->port_id[1] = fcport->d_id.b.area;
> +		tsk->port_id[2] = fcport->d_id.b.domain;
> +		tsk->vp_index = fcport->vha->vp_idx;
> +	}

[Severity: High]
Does this logic also need to be applied to the asynchronous abort path in
qla24xx_abort_iocb() within the same file?

Looking at qla24xx_abort_iocb(), it appears to still use the legacy 64-byte
layout unconditionally:

static void
qla24xx_abort_iocb(srb_t *sp, struct abort_entry_24xx *abt_iocb)
{
    ...
	abt_iocb->vp_index = vha->vp_idx;
	abt_iocb->req_que_no = aio->u.abt.req_que_no;
    ...
}

Because the 29xx extended layout expects vp_index at offset 18 instead
of offset 51, and the request slot is zero-initialized, won't the 29xx
firmware read a vp_index of 0 here?

Could this result in asynchronous I/O aborts on NPIV virtual ports being
incorrectly routed through the physical port (VP 0) or rejected with
errors like CS_PORT_UNAVAILABLE?

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