Re: [PATCH v5 11/56] scsi: qla2xxx: Update IO path to use 128-byte IOCBs for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Non-IO path IOCB builders (Abort, Task Management, ELS, Login, Logout, PRLI) fail to use the extended 128-byte struct layouts on QLA29XX adapters, corrupting extended IOCBs and breaking NPIV isolation.
- [Critical] The ISR completion path fails to handle 29xx extended status IOCBs, resulting in garbage values being read for SCSI status, sense lengths, and NVMe ERSP data.
- [High] Manual 64-byte ring pointer advancement in `qla27xx_copy_multiple_pkt()` corrupts the response ring on QLA29XX adapters.
- [High] Continuation IOCB payloads are severely truncated on QLA29XX adapters due to hardcoded 60-byte copy limits.
- [High] Continuation IOCBs in target mode are under-packed with DSDs on 29xx hardware, leading to truncated data transfers.
--

--- Patch [11]: [PATCH v5 11/56] scsi: qla2xxx: Update IO path to use 128-byte IOCBs for 29xx ---
commit 5e78bc810efc46ed3e8aed3841ea031f78694acf
Author: Anil Gurumurthy <agurumurthy@marvell.com>

scsi: qla2xxx: Update IO path to use 128-byte IOCBs for 29xx

This commit updates the driver to support 128-byte extended IOCB
structures for the IO submission, completion, and queue management paths
on 29xx adapters. It introduces helpers for entry size selection and
slot advancement, falling back to 64-byte IOCBs for earlier adapters.

Link: https://lore.kernel.org/oe-kbuild-all/202607141827.R9mpeMZx-lkp@intel.com/

> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index dbe3cd4e274c8..aab7905c0d6ca 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c

[ ... ]

[Severity: Critical]
Does qla2x00_start_sp() properly dispatch non-IO paths for QLA29XX adapters?

Currently, it seems to dispatch management commands (like Abort, Task
Management, ELS, Login, Logout, PRLI) to legacy 64-byte builders like
qla24xx_tm_iocb() and qla24xx_abort_iocb().

If these builders cast the 128-byte slot to the legacy struct and write to
the old offsets (e.g., vp_index written to offset 51 instead of 18 or 28),
could this result in management commands being sent to the wrong virtual
port (VP 0) and breaking NPIV isolation?

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index c47c38e099ff8..e95fb0e59f387 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c

[ ... ]

> @@ -1212,13 +1200,7 @@ qla27xx_copy_fpin_pkt(struct scsi_qla_host *vha, void **pkt,
>  				break;
>  			}
>  
> -			rsp_q->ring_index++;
> -			if (rsp_q->ring_index == rsp_q->length) {
> -				rsp_q->ring_index = 0;
> -				rsp_q->ring_ptr = rsp_q->ring;
> -			} else {
> -				rsp_q->ring_ptr++;
> -			}
> +			qla_rsp_ring_advance(rsp_q);
>  			no_bytes = (pending_bytes > sizeof(new_pkt->data)) ?
>  			    sizeof(new_pkt->data) : pending_bytes;

[Severity: High]
Does this logic truncate extended payloads on 29xx adapters? 

The advancement helper qla_rsp_ring_advance() steps by 128 bytes for extended
rings, but the copy size is still bounded by sizeof(new_pkt->data) which is 60
bytes. Can this ignore the remaining data the 29xx firmware places in the
extended slot?

Also, is there a similar issue in qla27xx_copy_multiple_pkt() where the manual
rsp_q->ring_index advancement was left behind? Using the old 64-byte stride
logic there could corrupt the response ring on QLA29XX adapters by misaligning
the pointer.

[ ... ]

> @@ -4003,13 +3980,7 @@ void qla24xx_process_response_queue(struct scsi_qla_host *vha,
>  		pkt = (struct sts_entry_24xx *)rsp->ring_ptr;
>  		cur_ring_index = rsp->ring_index;
>  
> -		rsp->ring_index++;
> -		if (rsp->ring_index == rsp->length) {
> -			rsp->ring_index = 0;
> -			rsp->ring_ptr = rsp->ring;
> -		} else {
> -			rsp->ring_ptr++;
> -		}
> +		qla_rsp_ring_advance(rsp);
>  
>  		if (pkt->entry_status != 0) {
>  			if (qla2x00_error_entry(vha, rsp, (sts_entry_t *) pkt))

[Severity: Critical]
Will reading completion entries through a hardcoded 64-byte structure like
sts_entry_24xx cause problems on 29xx hardware? 

If functions like qla2x00_status_entry() or qla24xx_nvme_iocb_entry()
unconditionally cast the 128-byte response slot to the legacy 64-byte struct,
wouldn't they read unrelated fields for the SCSI status, sense lengths, and
NVMe ERSP data?

> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index e47da45e93a01..53a505df8da0a 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c

[ ... ]

> @@ -2614,7 +2611,7 @@ static void qlt_load_cont_data_segments(struct qla_tgt_prm *prm)
>  		 * that.
>  		 */
>  
> -		memset(cont_pkt64, 0, sizeof(*cont_pkt64));
> +		memset(cont_pkt64, 0, qla_req_entry_size(ha));
>  
>  		cont_pkt64->entry_count = 1;
>  		cont_pkt64->sys_define = 0;

[Severity: High]
Does this loop pack enough DSDs for 29xx hardware?

Looking slightly further down in qlt_load_cont_data_segments():

		for (cnt = 0;
		    cnt < QLA_TGT_DATASEGS_PER_CONT_24XX && prm->seg_cnt;
		    cnt++, prm->seg_cnt--) {
			append_dsd64(&cur_dsd, prm->sg);
			prm->sg = sg_next(prm->sg);
		}

If the loop strictly limits packing to QLA_TGT_DATASEGS_PER_CONT_24XX (5) DSDs,
will the 29xx firmware (which expects 10 DSDs per 128-byte slot) encounter
zero-length DSDs mid-transfer and prematurely terminate target mode data
transfers?

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