Re: [PATCH v6 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 6 potential issue(s) to consider:

New issues:
- [Critical] `qla27xx_copy_multiple_pkt` uses `rsp_q->ring_ptr++` instead of `qla_rsp_ring_advance()`, immediately corrupting the 128-byte response ring on 29xx.
- [High] Target mode scatter-gather transfers > 6 segments will fail on 29xx due to hardcoded continuation DSD limits, and target mode unconditionally uses 64-byte `ctio7_to_24xx` offsets for 128-byte IOCBs.
- [High] Payload extraction paths for PUREX, FPIN, and STATUS_CONT_TYPE response IOCBs fail to use the extended 128-byte struct layouts on 29xx adapters, silently truncating data.
- [High] The completion path (ISR) unconditionally parses 128-byte 29xx status IOCBs using the legacy 64-byte `sts_entry_24xx` layout, reading critical fields from the wrong offsets.
- [High] `qla24xx_tm_iocb` builds Task Management IOCBs using the 24xx layout, corrupting the `vp_index` on 29xx.

Pre-existing issues:
- [High] The 24xx path in `qla24xx_dif_start_scsi` and `qla2xxx_dif_start_scsi_mq` fails to initialize the `vp_index` field of the `cmd_type_crc_2` IOCB, leaving it at 0. This breaks NPIV for DIF/PI commands on 24xx adapters.
--

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

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

This patch updates the IO submission, completion, and queue-management paths
to support 128-byte extended IOCBs on 29xx adapters. It introduces helpers
for dynamic ring entry size and pointer advancement to support both legacy
and extended layouts.

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
[ ... ]
> @@ -1829,17 +1901,24 @@ qla24xx_dif_start_scsi(srb_t *sp)
>  	req->cnt -= req_cnt;
>  
>  	/* Fill-in common area */
> -	cmd_pkt = (struct cmd_type_crc_2 *)req->ring_ptr;
> +	cmd_pkt = (struct cmd_type_crc_2 *)qla_req_ring_slot(ha, req);
>  	cmd_pkt->handle = make_handle(req->id, handle);
>  
>  	clr_ptr = (uint32_t *)cmd_pkt + 2;
> -	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
> +	memset(clr_ptr, 0, qla_req_entry_size(ha) - 8);
>  
>  	/* Set NPORT-ID and LUN number*/
>  	cmd_pkt->nport_handle = cpu_to_le16(sp->fcport->loop_id);
> -	cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> -	cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> -	cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> +	if (!IS_QLA29XX(ha)) {
> +		cmd_pkt->port_id[0] = sp->fcport->d_id.b.al_pa;
> +		cmd_pkt->port_id[1] = sp->fcport->d_id.b.area;
> +		cmd_pkt->port_id[2] = sp->fcport->d_id.b.domain;
> +	}

[Severity: High]
This is a pre-existing issue, but does the 24xx branch fail to initialize
the vp_index field here?

Because vp_index is not assigned, it remains at the zeroed default. This
breaks NPIV for DIF/PI commands on 24xx adapters, as the switch will receive
frames from the base port's WWPN instead of the virtual port's WWPN.

This same pre-existing issue appears in qla2xxx_dif_start_scsi_mq() as well.

> +	/*
> +	 * vp_index: 24xx uses a uint8_t field at offset 51 (in port_id
> +	 * group); 29xx replaces that region with control_flags_2/vp_index as
> +	 * __le16s starting at offset 48.
> +	 */
[ ... ]
> @@ -4289,8 +4529,15 @@ qla_start_scsi_type6(srb_t *sp)

[Severity: High]
Are Task Management IOCBs built correctly for 29xx hardware?

When issuing an abort or reset, qla24xx_tm_iocb() fills the 128-byte slot
assuming the 64-byte tsk_mgmt_entry layout. It writes port_id to offset 48
and vp_index to offset 51, but on 29xx (tsk_mgmt_entry_ext), vp_index is a
16-bit field at offset 28.

Could this cause TMF commands to be routed to the wrong virtual port and
overwrite reserved fields?

> 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
[ ... ]
> @@ -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 code truncate the payload extraction on 29xx adapters?

sizeof(new_pkt->data) evaluates to 60 bytes (for sts_cont_entry_t), but a
29xx continuation slot holds 124 bytes of data. It appears the remaining
64 bytes will be skipped and lost here.

The same truncation issue exists in qla27xx_copy_fpin_pkt() and
qla2x00_status_cont_entry() where SCSI sense data extraction is also
limited to sizeof(pkt->data).

>  			if ((buffer_copy_offset + no_bytes) <= total_bytes) {
[ ... ]
> @@ -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);

[Severity: High]
Will the completion path correctly parse 128-byte 29xx status IOCBs?

The main loop processes completions using the legacy 64-byte sts_entry_24xx
layout. For example, qla2x00_status_entry() reads scsi_status from
offset 22, but on 29xx (sts_entry_24xx_ext), scsi_status is located at
offset 54. Could this result in misinterpreting completion status and
residual counts?

[Severity: Critical]
Did we miss converting a manual pointer increment in
qla27xx_copy_multiple_pkt()?

If rsp_q->ring_ptr++ is used there instead of qla_rsp_ring_advance(), the
pointer will advance by 64 bytes instead of 128 bytes on 29xx hardware,
permanently desynchronizing the driver's view of the ring from the firmware.

>  
>  		if (pkt->entry_status != 0) {
>  			if (qla2x00_error_entry(vha, rsp, (sts_entry_t *) pkt))
> 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
[ ... ]
> @@ -2549,9 +2545,9 @@ static int qlt_24xx_build_ctio_pkt(struct qla_qpair *qpair,
>  	uint16_t temp;
>  	struct qla_tgt_cmd      *cmd = prm->cmd;
>  
> -	pkt = (struct ctio7_to_24xx *)qpair->req->ring_ptr;
> +	pkt = (struct ctio7_to_24xx *)qla_req_ring_slot(qpair->hw, qpair->req);
>  	prm->pkt = pkt;
> -	memset(pkt, 0, sizeof(*pkt));
> +	memset(pkt, 0, qla_req_entry_size(qpair->hw));
>  
>  	pkt->entry_type = CTIO_TYPE7;
>  	pkt->entry_count = (uint8_t)prm->req_cnt;

[Severity: High]
Are we sure it is safe to cast the 128-byte slot to the legacy 64-byte
ctio7_to_24xx struct here?

On 29xx hardware, qla_req_ring_slot() will return a 128-byte slot, but
writing to it with the 64-byte struct offsets will misalign critical fields
like vp_index.

> @@ -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);

[Severity: High]
Does this continuation logic correctly handle the 29xx hardware requirements?

Looking at the loop below, only 5 DSDs are populated into the 128-byte
extended continuation IOCB here, but 29xx hardware expects 10 DSDs. Will
the firmware terminate the transfer early when it encounters the zeroed 6th
DSD?

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