Re: [PATCH v6 09/56] scsi: qla2xxx: Add 128-byte IOCB definitions for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Endianness portability violation using bitfields on __le16 DMA structures.
- [Medium] Missing padding array leaves `struct vp_rpt_id_entry_24xx_ext` at 64 bytes instead of the required 128 bytes.
- [Medium] Possible truncation of 9-bit VP indices in `vp_config_entry_24xx_ext` and `vp_ctrl_entry_24xx_ext`.
- [Low] Stale comment in `struct sts_entry_24xx_ext` provides incorrect array offsets for DIF error handling.
--

--- Patch [9]: [PATCH v6 09/56] scsi: qla2xxx: Add 128-byte IOCB definitions for 29xx ---
commit f045d9bbc4916b8badfe0c3d725fe53a57f4cac2
Author: Anil Gurumurthy <agurumurthy@marvell.com>

scsi: qla2xxx: Add 128-byte IOCB definitions for 29xx

This commit introduces a new header file containing extended 128-byte IOCB
structure definitions for the QLogic 29xx series adapters. It maps the new
firmware interface to C structures for hardware DMA interactions.

> diff --git a/drivers/scsi/qla2xxx/qla_fw29.h b/drivers/scsi/qla2xxx/qla_fw29.h
> new file mode 100644
> index 0000000000000..efe1c60bee81d
> --- /dev/null
> +++ b/drivers/scsi/qla2xxx/qla_fw29.h
> @@ -0,0 +1,686 @@

[ ... ]

> +struct sts_entry_24xx_ext {
> +	uint8_t entry_type;		/* Entry type. */
> +	uint8_t entry_count;		/* Entry count. */

[ ... ]

> +	uint8_t	reserved_2[8];
> +	uint8_t act_dif[8];
> +	uint8_t exp_dif[8];
> +	union {

[ ... ]

> +	/*
> +	 * If DIF Error is set in comp_status, these additional fields are
> +	 * defined:
> +	 *
> +	 * !!! NOTE: Firmware sends expected/actual DIF data in big endian
> +	 * format; but all of the "data" field gets swab32-d in the beginning
> +	 * of qla2900_status_entry().
> +	 *
> +	 * &data[10] : uint8_t report_runt_bg[2];	- computed guard
> +	 * &data[12] : uint8_t actual_dif[8];		- DIF Data received
> +	 * &data[20] : uint8_t expected_dif[8];		- DIF Data computed
> +	 */
> +};

[Severity: Low]
Is this comment still accurate for the new 128-byte structure?

It appears this was copied from the legacy 64-byte struct. In this new layout,
the data array starts at offset 68, but the DIF error fields are explicitly
defined as act_dif[8] and exp_dif[8] at offsets 32 and 40.

Could this documentation mislead developers trying to access DIF error data?

[ ... ]

> +struct els_entry_24xx_ext {
> +	uint8_t entry_type;		/* Entry type. */

[ ... ]

> +	__le16	tx_dsd_count;
> +
> +	__le16	vp_index : 9;		/* VP Index 9bits */
> +	__le16	reserved_1_sof : 3;
> +	__le16	sof_type : 4;
> +
> +	__le32	rx_xchg_address;	/* Receive exchange address. */

[Severity: High]
Can this cause endianness issues on big-endian architectures?

Since these structures map directly to hardware DMA buffers, compiler-dependent
C bitfield packing on __le16 variables could scramble the vp_index and sof_type
bits on big-endian hosts.

Would it be safer to use a plain __le16 field and define bitmask macros like
CMD_EXT_VP_INDEX_MASK, similar to what was done for cmd_type_crc_2_ext?

[ ... ]

> +struct els_sts_entry_24xx_ext {
> +	uint8_t entry_type;		/* Entry type. */

[ ... ]

> +	__le16	reserved_1;
> +
> +	__le16	vp_index : 9;		/* VP Index 9bits */
> +	__le16	reserved_1_sof : 3;
> +	__le16	sof_type : 4;
> +
> +	__le32	rx_xchg_address;	/* Receive exchange address. */

[Severity: High]
Are these __le16 bitfields also susceptible to endianness packing issues across
different architectures?

[ ... ]

> +struct abts_entry_24xx_ext {
> +	uint8_t entry_type;
> +	uint8_t entry_count;

[ ... ]

> +	__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 */
> +
> +	__le32	rx_xch_addr;

[Severity: High]
Do these bitfields have the same endianness concerns as the others?

[ ... ]

> +struct vp_ctrl_entry_24xx_ext {
> +	uint8_t entry_type;		/* Entry type. */

[ ... ]

> +	__le16	command;
> +
> +	__le16	vp_count;
> +
> +	uint8_t vp_idx_map[16];
> +	__le16	flags;
> +	__le16	id;

[Severity: Medium]
Are these 8-bit map entries sufficient for a 9-bit virtual port index?

The rest of the 29xx extended structures expand the virtual port index to
9 bits to support up to 512 virtual ports. Will mapping these indices to
uint8_t fields silently truncate values above 255?

[ ... ]

> +struct vp_config_entry_24xx_ext {
> +	uint8_t entry_type;		/* Entry type. */

[ ... ]

> +	uint8_t command;
> +
> +	uint8_t vp_count;
> +
> +	uint8_t vp_index1;
> +	uint8_t vp_index2;
> +
> +	uint8_t options_idx1;

[Severity: Medium]
Could this truncate the 9-bit virtual port indices?

If the 29xx hardware supports indices up to 511, storing them in these
uint8_t variables will drop the upper bit.

[ ... ]

> +struct vp_rpt_id_entry_24xx_ext {
> +	uint8_t entry_type;		/* Entry type. */
> +	uint8_t entry_count;		/* Entry count. */
> +	uint8_t sys_define;		/* System defined. */
> +	uint8_t entry_status;		/* Entry Status. */
> +	__le32 resv1;
> +	uint8_t vp_acquired;
> +	uint8_t vp_setup;
> +	__le16	vp_idx : 9;		/* VP Index 9bits */
> +	__le16	vp_status : 7;		/* VP Status 7bits */

[Severity: High]
Do these bitfields have the same endianness concerns as the ones in
els_entry_24xx_ext?

[ ... ]

> +			uint8_t port_name[8];
> +			uint8_t node_name[8];
> +			uint16_t bbcr;
> +			uint8_t reserved_5[2];
> +			uint8_t remote_nport_id[4];
> +		} f2;
> +	} u;
> +};
> +#endif

[Severity: Medium]
Does this structure meet the 128-byte size requirement?

Unlike the other extended structures which include a reserved padding array
at the end to ensure the struct size is exactly 128 bytes, this one appears
to omit the padding and end at 64 bytes.

If any queue logic relies on the size of this struct, could it miscalculate
strides or corrupt memory queues?

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