Re: [PATCH v4 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:
- [High] Legacy 64-byte IOCB structures are incorrectly used on 29xx hardware for management, control, and abort paths, causing field corruption and out-of-bounds writes.
- [High] Response queue payload extraction and continuation processing loops fail to account for the 128-byte extended format on 29xx hardware, dropping data.
- [High] The driver miscalculates the required IOCB count (`req_cnt`) for standard SCSI commands on 29xx hardware, causing ring desynchronization.
- [High] Target mode I/O requests with >6 scatter-gather segments on 29xx hardware cause ring desynchronization and zero-length DMA truncation.
- [High] Open-coded legacy response ring advance in `qla27xx_copy_multiple_pkt()` incorrectly increments by 64 bytes on 29xx hardware.
--

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

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

This commit wires the 128-byte extended IOCB structures into the IO
submission, completion, and queue-management paths for the 29xx adapters.
It introduces helpers for determining ring entry sizes and advancing ring
slots while falling back to the existing 64-byte IOCBs for earlier adapters.

> 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
[ ... ]
> @@ -1620,46 +1640,84 @@ qla24xx_start_scsi(srb_t *sp)
>  	cmd->host_scribble = (unsigned char *)(unsigned long)handle;
>  	req->cnt -= req_cnt;
>  
> -	cmd_pkt = (struct cmd_type_7 *)req->ring_ptr;
> -	cmd_pkt->handle = make_handle(req->id, handle);
> +	if (IS_QLA29XX(ha)) {
[ ... ]
> +		/* Build IOCB segments */
> +		qla29xx_build_scsi_iocbs(sp, cmd_pkt_ext, tot_dsds, req);
> +
> +		/* Set total data segment count. */
> +		cmd_pkt_ext->entry_count = (uint8_t)req_cnt;

[Severity: High]
Could this miscalculate the required IOCB count for standard SCSI commands on
29xx hardware? Earlier in qla24xx_start_scsi(), req_cnt is calculated using
qla24xx_calc_iocbs(). That assumes 1 inline DSD and 5 per continuation, but
29xx extended IOCBs carry 4 inline DSDs and 10 per continuation. Writing the
oversized req_cnt to cmd_pkt_ext->entry_count might desynchronize the ring if
it doesn't match the slots built by qla29xx_build_scsi_iocbs().

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
[ ... ]
> @@ -287,13 +281,7 @@ int __qla_copy_purex_to_buffer(struct scsi_qla_host *vha,
>  				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 truncate the payload extraction on 29xx hardware? The code caps the
read to sizeof(new_pkt->data) (which is 60 bytes) instead of the 124-byte
capacity of the 29xx sts_cont_entry_ext structure. Because
qla_rsp_ring_advance() moves the pointer by 128 bytes, it seems this might
permanently skip the remaining 68 bytes of data in every extended continuation
slot. A similar cap exists in qla27xx_copy_fpin_pkt() and
qla2x00_status_cont_entry().

[Severity: High]
Is there a missing update to qla27xx_copy_multiple_pkt() in this patch?
Many open-coded ring advances were replaced with qla_rsp_ring_advance()
to handle the 128-byte stride on 29xx hardware, but it appears
qla27xx_copy_multiple_pkt() still manually increments rsp_q->ring_ptr++.
Advancing by 64 bytes instead of the 29xx 128-byte stride might cause the
firmware to read zeros and drop multi-IOCB unsolicited ELS/FPIN packets.

[Severity: High]
Does this also cause field corruption on 29xx hardware for abort paths? In
qla24xx_process_abts() in drivers/scsi/qla2xxx/qla_isr.c, the 128-byte slot is
cast to the 64-byte els_entry_24xx struct for the ABTS response, which might
corrupt fields like control_flags. Similarly, legacy 64-byte structures are
still used in qla24xx_abort_iocb(), qla24xx_tm_iocb(), and qla_els_pt_iocb(),
writing vp_index to offset 51 instead of the 18 or 30 required by the 29xx
extended abort/TM IOCBs.

[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
[ ... ]
> @@ -2600,11 +2596,12 @@ static void qlt_load_cont_data_segments(struct qla_tgt_prm *prm)
>  {
>  	int cnt;
>  	struct dsd64 *cur_dsd;
> +	struct qla_hw_data *ha = prm->cmd->qpair->hw;
>  
>  	/* Build continuation packets */
>  	while (prm->seg_cnt > 0) {
>  		cont_a64_entry_t *cont_pkt64 =
> -			(cont_a64_entry_t *)qlt_get_req_pkt(
> +			(cont_a64_entry_t *)qlt_get_req_pkt(ha,
>  			   prm->cmd->qpair->req);
>  
>  		/*
> @@ -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));

[Severity: High]
Will target mode I/O requests with more than 6 scatter-gather segments cause
ring desynchronization here? The loop right below this initialization still
hardcodes QLA_TGT_DATASEGS_PER_CONT_24XX (5 DSDs) per continuation packet. On
29xx hardware, the firmware reads 10 DSDs per continuation packet from the
128-byte slot, which could lead to reading empty DSDs and terminating the
transfer early. qlt_pci_map_calc_cnt() also appears to allocate the number of
continuation packets based on 5 DSDs.

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