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

[sashiko-bot@kernel.org]

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

New issues:
- [High] User-configured 128G port speed is silently lost upon HBA reset.

Pre-existing issues:
- [High] Applying a user-configured port speed (<= 64G) in the Initialization Control Block inadvertently wipes out other critical firmware flags.
--

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

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

This patch adds support for 64G and 128G port speeds by mapping sysfs inputs 
to the corresponding PORT_SPEED_* constants. It also updates the ring 
configuration to only set the data rate in the Initialization Control Block 
(ICB) for speeds up to 64G to prevent overflow.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> --- 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.
> +		 */
> +		if (ha->set_data_rate <= PORT_SPEED_64GB)

[Severity: High]
Does this code silently drop the user-configured 128G port speed upon HBA
reset?

If a user sets the 128G port speed via sysfs, and later a normal operational
reset (such as a LIP reset or driver re-initialization) triggers
qla24xx_config_rings(), the 128G speed won't be set here since it's > 64G.

However, the driver doesn't seem to issue the MBC_DATA_RATE mailbox command
during or after the reset sequence to restore the 128G setting.

Does the adapter fall back to auto-negotiation or the NVRAM default in this
case?

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

[Severity: High]
This is a pre-existing issue, but does this absolute assignment blindly
overwrite the entire firmware_options_3 register instead of preserving
previously configured bits?

When qla24xx_config_rings() applies a custom port speed (<= 64G), it overwrites
flags like BIT_8 (N2N driver-initiated login) and BIT_0 (RIDA Format2) that
were initialized earlier in qla24xx_nvram_config().

Can this break N2N topology login and other features when a custom port speed
is used?

>  	}
>  
>  	/* PCI posting */

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