Re: [PATCH v6 41/56] scsi: qla2xxx: Replace __le16 bitfields with scalar and accessors

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete fix for endianness bug in 29xx extended IOCBs

Pre-existing issues:
- [High] Target mode ABTS failures on big-endian due to bitfield packing
--

--- Patch [41]: [PATCH v6 41/56] scsi: qla2xxx: Replace __le16 bitfields with scalar and accessors ---
commit aa60e92d6d4c10c7735dd3bda849923fc8a00254
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Replace __le16 bitfields with scalar and accessors

C bitfield packing order is implementation-defined: GCC packs LSB-first
on little-endian targets and MSB-first on big-endian targets.  The
__le16 bitfield declarations for vp_index/sof_type in the 29xx extended
IOCB structures produce incorrect bit positions on big-endian hosts,
and Sparse cannot enforce endianness checks on bitfield members.

> Replace the three sets of __le16 bitfields (in els_entry_24xx_ext,
> els_sts_entry_24xx_ext, and abts_entry_24xx_ext) with a single __le16
> scalar field and provide inline accessor functions that use proper
> le16_to_cpu()/cpu_to_le16() with shift-and-mask operations.

[Severity: High]
Does this fix also need to be applied to vp_rpt_id_entry_24xx_ext in
drivers/scsi/qla2xxx/qla_fw29.h?

It appears vp_rpt_id_entry_24xx_ext still uses __le16 bitfields for
vp_idx and vp_status:

	__le16	vp_idx : 9;		/* VP Index 9bits */
	__le16	vp_status : 7;		/* VP Status 7bits */

When these fields are extracted in qla24xx_report_id_acquisition(), they will
be read incorrectly on big-endian architectures, causing VP ID reporting and
initialization to fail for 29xx series adapters.

Could this have been missed from the earlier patch "scsi: qla2xxx: Add size
check for extended VP report ID entry" in this series?

> Fixes: 4e0e5b8c80e8 ("scsi: qla2xxx: Add 128-byte IOCB definitions for 29xx")
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> Reviewed-by: Hannes Reinecke <hare@kernel.org>

> diff --git a/drivers/scsi/qla2xxx/qla_fw29.h b/drivers/scsi/qla2xxx/qla_fw29.h
> index 600a40d8bd5ff..63bf350ddffc0 100644
> --- a/drivers/scsi/qla2xxx/qla_fw29.h
> +++ b/drivers/scsi/qla2xxx/qla_fw29.h

[ ... ]

> @@ -563,9 +586,7 @@ struct abts_entry_24xx_ext {
>  	__le16	nport_handle;		/* type 0x54 only */
>  
>  	__le16	control_flags;		/* type 0x55 only */
> -	__le16	vp_idx : 9;		/* VP index 9 bits */
> -	__le16	reserved_1_sof : 3;
> -	__le16	sof_type : 4;		/* sof_type is upper nibble */
> +	__le16	vp_idx_sof;		/* bits [8:0]=VP index, [15:12]=SOF type */

[Severity: High]
This isn't a bug introduced by this patch, but does the abts_recv_from_24xx
structure in drivers/scsi/qla2xxx/qla_target.h suffer from a similar
bitfield packing issue?

Looking at abts_recv_from_24xx:

	uint8_t  reserved_3:4;
	uint8_t  sof_type:4;

GCC packs the first bitfield in the most-significant bits on big-endian
systems, moving sof_type to the least-significant bits. Since the hardware
expects sof_type in the upper nibble, could this lead to malformed ABTS
responses and target-mode functional failures?

This appears to be a pre-existing issue dating back to commit 2d70c103fd2a06.

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