Re: [PATCH v3 44/88] 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:
- [High] The 64GB port speed is silently ignored when read from the firmware due to a legacy 0x7 exclusion check.
- [High] Setting port speed to 128GB overflows the 3-bit data rate field, configuring 1GB speed and mistakenly enabling 75 ohm termination.
--

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

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

The patch adds support for 64G and 128G port speeds by mapping sysfs inputs to
PORT_SPEED_64GB and PORT_SPEED_128GB, and accepting these values in
qla2x00_set_data_rate().

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 6cf74f8c9628d..3b24e8a5e29ba 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c

[ ... ]

> @@ -1860,6 +1860,12 @@ qla2x00_port_speed_store(struct device *dev, struct device_attribute *attr,
>  	case 32:
>  		ha->set_data_rate = PORT_SPEED_32GB;
>  		break;
> +	case 64:
> +		ha->set_data_rate = PORT_SPEED_64GB;
> +		break;
> +	case 128:
> +		ha->set_data_rate = PORT_SPEED_128GB;

[Severity: High]
When setting ha->set_data_rate = PORT_SPEED_128GB (which is 0x08), does
this overflow the 3-bit data rate field during adapter initialization?

Looking at qla24xx_config_rings() in drivers/scsi/qla2xxx/qla_init.c:

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

Since the data rate is a 3-bit field (bits 13-15), shifting 0x08 (binary 1000)
by 13 overflows into bit 16.

Bits 13-15 would become 0 (1GB speed), and bit 16 (Enable 75 ohm Termination
Select) would become 1.

Could this cause the hardware to initialize at 1GB instead of 128GB and
erroneously enable hardware termination?

> +		break;
>  	default:

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index d0894cc904700..ba822c196894b 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c

[ ... ]

> @@ -5720,6 +5720,8 @@ qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode)
>  	case PORT_SPEED_8GB:
>  	case PORT_SPEED_16GB:
>  	case PORT_SPEED_32GB:
> +	case PORT_SPEED_64GB:

[Severity: High]
Since PORT_SPEED_64GB is defined as 0x07, will this new speed be silently
ignored by legacy 0x7 exclusion checks when reading from the firmware?

For example, in qla2x00_get_data_rate() the code explicitly drops 0x7:

drivers/scsi/qla2xxx/qla_mbx.c:qla2x00_get_data_rate() {
    ...
    if (mcp->mb[1] != 0x7)
        ha->link_data_rate = mcp->mb[1];
    ...
}

Does this leave the speed as unknown or stale in ha->link_data_rate when the
firmware is operating at 64GB, breaking sysfs port speed reporting?

Similarly, the debug log lower down in this function is also suppressed for
the newly added 64GB speed:

drivers/scsi/qla2xxx/qla_mbx.c:qla2x00_set_data_rate() {
    ...
    if (mcp->mb[1] != 0x7)
        ql_dbg(ql_dbg_mbx, vha, 0x1179,
               "Speed set:0x%x\n", mcp->mb[1]);
    ...
}

> +	case PORT_SPEED_128GB:
>  		val = ha->set_data_rate;
>  		break;
>  	default:

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