Re: [PATCH V1 1/2] scsi: ufs: ufs-qcom: Add specified gear support for multi gear scaling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Valid `opp-level` DT gear configurations are improperly ignored and downgraded if they exceed the attached device's maximum supported gear, breaking forward compatibility.
--

commit d0c65635e24a4008e442b75486fc367af65e28be
Author: Ziqi Chen <ziqi.chen@oss.qualcomm.com>

scsi: ufs: ufs-qcom: Add specified gear support for multi gear scaling

This commit introduces a device tree based configuration interface that allows
specifying the HS gear speed for supported operating frequencies using the
"opp-level" property. If this property is not configured, the driver falls
back to a default frequency-to-gear mapping table.

> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 291c434487648..399269c3f0597 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c

[ ... ]

> @@ -2490,12 +2491,32 @@ static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
>  
>  static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
>  {
> -	u32 gear = UFS_HS_DONT_CHANGE;
> +	struct dev_pm_opp *opp;
>  	unsigned long unipro_freq;
> +	u32 gear = UFS_HS_DONT_CHANGE;
>  
>  	if (!hba->use_pm_opp)
>  		return gear;
>  
> +	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
> +	if (IS_ERR_OR_NULL(opp)) {
> +		dev_err(hba->dev, "%s: Failed to find OPP for exact frequency %lu\n",
> +			__func__, freq);
> +		return gear;
> +	}
> +
> +	/* Get HS gear speed from 'opp-level' */
> +	gear = dev_pm_opp_get_level(opp);
> +	dev_pm_opp_put(opp);
> +
> +	/*
> +	 * Greater than max gear means that there is no specified gear configured in DT
> +	 * or the specified gear is invalid.
> +	 */
> +	if (gear <= hba->max_pwr_info.info.gear_rx)
> +		return gear;
> +

[Severity: High]
Does this condition inadvertently downgrade valid DT gear configurations if
they exceed the attached device's maximum supported gear?

If a platform specifies a valid opp-level (e.g., 5) but the connected UFS
device supports a lower maximum gear (e.g., gear_rx = 4), this check
evaluates to false. The driver will then incorrectly fall back to the
legacy hardcoded mapping table instead of using the capped value.

If the frequency is not present in the fallback table, it returns
UFS_HS_DONT_CHANGE, which can cause a mismatch between the configured clock
and active gear speed. This mismatch could potentially lead to UFS link drops
and system panics during frequency scaling.

It appears this logic conflates a missing DT property (where gear might
evaluate to OPP_LEVEL_UNSET or U32_MAX) with a valid property that simply
exceeds the device capability.

Could this be addressed by explicitly checking if the property was provided,
and if so, safely capping it to the device's maximum supported gear using
min_t(u32, gear, hba->max_pwr_info.info.gear_rx)?

> +	gear = UFS_HS_DONT_CHANGE;
>  	unipro_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829074355.946543-1-nitin.rawat@oss.qualcomm.com?part=1