Re: [PATCH v5 44/56] scsi: qla2xxx: Add 64G/128G port speed setting support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The 128G port speed setting is not re-applied across adapter resets, causing it to silently revert to default speeds (e.g., Auto-negotiation).
- [Medium] A Time-Of-Check to Time-Of-Use race condition on `ha->set_data_rate` can bypass the boundary check, causing an out-of-bounds bit shift that corrupts bit 16.

Pre-existing issues:
- [High] Direct assignment to `icb->firmware_options_3` using `=` silently overwrites and clears previously configured NVRAM-derived flags.
--

--- Patch [44]: [PATCH v5 44/56] scsi: qla2xxx: Add 64G/128G port speed setting support ---
commit 8fb2f0a9558bd7064a13dd344fce6bb250eb5ac0
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Add 64G/128G port speed setting support

The port speed setting paths topped out at 32G. This patch maps the 64 and
128 sysfs inputs to PORT_SPEED_64GB and PORT_SPEED_128GB, and accepts those
values in qla2x00_set_data_rate(). Firmware validates the requested rate.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 545066f67e020..5f7bc68aab3c5 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c

[ ... ]

> @@ -4793,7 +4793,14 @@ qla24xx_config_rings(struct scsi_qla_host *vha)
>  		ql_dbg(ql_dbg_init, vha, 0x00fd,
>  		    "Speed set by user : %s Gbps \n",
>  		    qla2x00_get_link_speed_str(ha, ha->set_data_rate));
> -		icb->firmware_options_3 = cpu_to_le32(ha->set_data_rate << 13);
> +		/*
> +		 * The ICB data-rate field is 3 bits (bits 13-15); rates above
> +		 * 64G do not fit and would overflow into bit 16 (75 ohm
> +		 * termination select). Such rates are forced via MBC_DATA_RATE.
> +		 */

[Severity: High]
The comment mentions rates above 64G are forced via MBC_DATA_RATE. Is this
compensatory MBC_DATA_RATE command actually sent during the adapter
initialization or reset path?

If a user configures 128G via sysfs and the adapter undergoes an ISP abort or
link reset, qla24xx_config_rings() skips setting the speed in the ICB here.
Without a subsequent MBC_DATA_RATE command in the initialization path, does
this cause the 128G setting to be silently lost and revert to default speeds
like Auto-negotiation?

> +		if (ha->set_data_rate <= PORT_SPEED_64GB)

[Severity: Medium]
Is there a Time-Of-Check to Time-Of-Use race condition here between the
boundary check and the shift below?

Since ha->set_data_rate can be updated locklessly via the sysfs store
function, if a concurrent write changes the rate from PORT_SPEED_64GB to
PORT_SPEED_128GB exactly between this check and the assignment, will the
shift use the 128GB value?

This could bypass the safety check and cause an out-of-bounds shift
(128 << 13) which overflows into bit 16 and unintentionally toggles the
75 ohm termination select bit on the adapter.

> +			icb->firmware_options_3 =
> +			    cpu_to_le32(ha->set_data_rate << 13);

[Severity: High]
This is a pre-existing issue, but does this direct assignment to
icb->firmware_options_3 silently overwrite previously configured
NVRAM-derived flags?

During adapter initialization, functions like qla24xx_nvram_config() set
multiple flags in icb->firmware_options_3, such as BIT_8 for N2N
driver-initiated login. Using an assignment (=) instead of a
read-modify-write (|= and &= ~MASK) appears to wipe out those previously
configured flags.

>  	}
>  
>  	/* PCI posting */

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