Re: [PATCH v4 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] Port speeds > 64G are silently lost after an adapter reset and cannot be re-applied via sysfs.

Pre-existing issues:
- [Medium] The driver updates the software state `ha->set_data_rate` before hardware validation, but fails to roll it back if the hardware rejects the setting.
--

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

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

This patch adds support for setting 64G and 128G port speeds via sysfs. It
updates the port speed store function to map inputs to PORT_SPEED_64GB and
PORT_SPEED_128GB, and allows these values in qla2x00_set_data_rate(). It
also modifies the ICB configuration to handle rates up to 64G.

> 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;
> +		break;
>  	default:
>  		ql_log(ql_log_warn, vha, 0x1199,
>  		    "Unrecognized speed setting:%lx. Setting Autoneg\n",
>  		    speed);
>  		ha->set_data_rate = PORT_SPEED_AUTO;
>  	}
>  
>  	if (qla2x00_chip_is_down(vha) || (oldspeed == ha->set_data_rate))
>  		return -EINVAL;
>  
>  	ql_log(ql_log_info, vha, 0x70da,
>  	    "Setting speed to %lx Gbps \n", type);
>  
>  	rval = qla2x00_set_data_rate(vha, mode);
>  	if (rval != QLA_SUCCESS)
>  		return -EIO;

[Severity: Medium]
This is a pre-existing issue, but if qla2x00_set_data_rate() fails here
(e.g., if the firmware rejects an unsupported speed or the mailbox times out),
we return -EIO without rolling back ha->set_data_rate to oldspeed.

Does this leave ha->set_data_rate corrupted with the rejected speed?

If so, subsequent adapter resets might attempt to use this invalid speed,
and the user would be prevented from retrying the exact same speed via sysfs
because of the oldspeed == ha->set_data_rate check earlier in this function.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index e31997241c348..5f04190c14879 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.
> +		 */
> +		if (ha->set_data_rate <= PORT_SPEED_64GB)
> +			icb->firmware_options_3 =
> +			    cpu_to_le32(ha->set_data_rate << 13);

[Severity: High]
Since MBC_DATA_RATE is only issued from the sysfs store function and not
during reset recovery, what happens if the adapter resets asynchronously?

If the user configured a 128G speed, this ICB bypass logic skips setting it.
It seems the hardware might come up at a lower default speed (such as
auto-negotiation), effectively losing the configured 128G speed.

Furthermore, because ha->set_data_rate still holds the > 64G value, if the
user tries to re-apply the 128G setting via sysfs, will the
oldspeed == ha->set_data_rate check in qla2x00_port_speed_store() block the
write and prevent recovery?

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