Re: [PATCH v4 30/56] scsi: qla2xxx: Add size check for ELS status entry layout on 29xx

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting endianness bug in 29xx extended IOCB bitfields, which will cause corrupted DMA descriptors on Big Endian architectures.
--

--- Patch [30]: [PATCH v4 30/56] scsi: qla2xxx: Add size check for ELS status entry layout on 29xx ---
commit ef8ae6f09d5507520905d2b5e51c92a44459978f
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Add size check for ELS status entry layout on 29xx

This commit adds a BUILD_BUG_ON size check for the 29xx extended ELS status
entry layout. It also adds a documentation block explaining the byte-identical
offsets shared between the 24xx and 29xx layouts to justify casting in the
completion handler.

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index e2653620e80be..c286465ae013d 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -2359,6 +2359,22 @@ static void
>  qla24xx_els_ct_entry(scsi_qla_host_t *v, struct req_que *req,
>  		     void *pkt, int iocb_type)
>  {
> +	/*
> +	 * els_sts_entry_24xx_ext (29xx) overlays els_sts_entry_24xx for every
> +	 * field touched in this completion handler: comp_status (offset 8),
> +	 * total_byte_count (32), error_subcode_1 (36), error_subcode_2 (40),
> +	 * d_id[]/s_id[] (24..29), control_flags (30) all sit at byte-identical
> +	 * offsets in both layouts (only vp_index/sof_type at offset 14-15 are
> +	 * bit-packed differently, and that field is write-only on the issue
> +	 * path -- we never read it here). All reads in this function are

[Severity: High]
This is a pre-existing issue, but since this new comment mentions the
differently bit-packed vp_index and sof_type fields on the issue path, is it
possible these bitfields are fundamentally broken on Big Endian machines?

Looking at struct els_entry_24xx_ext in drivers/scsi/qla2xxx/qla_fw29.h:

	__le16	vp_index : 9;
	__le16	reserved_1_sof : 3;
	__le16	sof_type : 4;

And its assignment in drivers/scsi/qla2xxx/qla_inline.h:qla_els_set_vp_sof():

		ext->vp_index = vp_idx;
		ext->sof_type = ELS_EXT_EST_SOFI3;

Since compilers pack bitfields based on host endianness (typically starting
from the MSB on Big Endian systems) and do not automatically byte-swap
bitfields on assignment to an __le16 type, will this result in garbled DMA
descriptors when the Little Endian firmware reads this 16-bit word on a
Big Endian host?

> +	 * therefore stride-agnostic and go through a single struct
> +	 * els_sts_entry_24xx * view; the trailing reserved_4[] of the extended
> +	 * layout is irrelevant on completion.
> +	 *
> +	 * Likewise els_entry_24xx_ext overlays els_entry_24xx through
> +	 * control_flags (offset 30), so the SRB_ELS_CMD_HST_NOLOGIN ctl_flags
> +	 * read below also goes through the 24xx view.
> +	 */
>  	struct sts_entry_24xx *sts24 = pkt;
>  	struct els_sts_entry_24xx *ese = (struct els_sts_entry_24xx *)pkt;

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