Re: [PATCH v2 3/3] soc: qcom: ice: Add SCMI support for sa8255p based targets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Type confusion in `qcom_ice_create` causes system crash for legacy consumers.
- [Critical] Premature hardware access on `fw_managed` devices before SCMI PM domain is resumed.
- [Low] Discarded `const` qualifier on `device_get_match_data()` return value.
--

commit f67a1074aa6c150b2ffc9f1fb18d4a18e025ed13
Author: Linlin Zhang <linlin.zhang@oss.qualcomm.com>

soc: qcom: ice: Add SCMI support for sa8255p based targets

This commit adds SCMI power and performance protocol support for managing
ICE clocks on the Qualcomm SA8255p SoC. It introduces a firmware-managed
flag via match data to skip explicit clock enablement, relying on runtime
PM framework APIs instead.

> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 6f9d679b530cc..cf185a6e19731 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
[ ... ]
> @@ -570,20 +575,23 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>  	engine->dev = dev;
>  	engine->base = base;
>  
> -	/*
> -	 * Legacy DT binding uses different clk names for each consumer,
> -	 * so lets try those first. If none of those are a match, it means
> -	 * the we only have one clock and it is part of the dedicated DT node.
> -	 * Also, enable the clock before we check what HW version the driver
> -	 * supports.
> -	 */
> -	engine->core_clk = devm_clk_get_optional_enabled(dev, "ice_core_clk");
> -	if (!engine->core_clk)
> -		engine->core_clk = devm_clk_get_optional_enabled(dev, "ice");
> -	if (!engine->core_clk)
> -		engine->core_clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(engine->core_clk))
> -		return ERR_CAST(engine->core_clk);
> +	engine_cfg = device_get_match_data(dev);
> +	if (!engine_cfg || !engine_cfg->fw_managed) {

When qcom_ice_create() is called via the legacy fallback path
(of_qcom_ice_get()), the dev parameter points to the consumer device (e.g.,
the SDHCI or UFS host controller), not the dedicated ICE device.

Will calling device_get_match_data(dev) here retrieve the match data of the
consumer driver rather than struct engine_desc?

If the consumer's match data structure happens to have a non-zero byte where
fw_managed is expected, this could falsely evaluate to true, skipping clock
initialization and leading to a crash.

> +		/*
> +		 * Legacy DT binding uses different clk names for each consumer,
> +		 * so lets try those first. If none of those are a match, it means
> +		 * the we only have one clock and it is part of the dedicated DT node.
> +		 * Also, enable the clock before we check what HW version the driver
> +		 * supports.
> +		 */
> +		engine->core_clk = devm_clk_get_optional_enabled(dev, "ice_core_clk");
> +		if (!engine->core_clk)
> +			engine->core_clk = devm_clk_get_optional_enabled(dev, "ice");
> +		if (!engine->core_clk)
> +			engine->core_clk = devm_clk_get_enabled(dev, NULL);
> +		if (IS_ERR(engine->core_clk))
> +			return ERR_CAST(engine->core_clk);
> +	}
>  
>  	if (!qcom_ice_check_supported(engine))
>  		return ERR_PTR(-EOPNOTSUPP);

For firmware-managed devices, the driver skips explicitly enabling the
ICE core clock. However, qcom_ice_check_supported() immediately reads the
QCOM_ICE_REG_VERSION hardware register.

Could this access cause a bus fault on firmware-managed devices?

The runtime PM framework is responsible for the SCMI power and performance
domain, but devm_pm_runtime_enable() is called in probe() after
qcom_ice_create() returns. Reading hardware registers before the PM
domain is active might violate power management ordering.

[ ... ]
> @@ -756,13 +764,17 @@ static void qcom_ice_remove(struct platform_device *pdev)
>  
>  static int ice_runtime_resume(struct device *dev)
>  {
> -	struct qcom_ice *ice = dev_get_drvdata(dev);
> +	struct engine_desc *engine_cfg = device_get_match_data(dev);

This isn't a bug, but device_get_match_data() returns a const void pointer.

Assigning it to a non-const struct engine_desc pointer discards the const
qualifier and triggers a compiler warning. Could this be declared as
const struct engine_desc *engine_cfg instead, similar to how it is handled
in ice_runtime_suspend()?

>  	int err = 0;
>  
> -	err = clk_prepare_enable(ice->core_clk);
> -	if (err) {
> -		dev_err(dev, "failed to enable core clock (%d)\n",
> -			err);
> +	if (!engine_cfg || !engine_cfg->fw_managed) {
> +		struct qcom_ice *ice = dev_get_drvdata(dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512033750.3393050-1-linlin.zhang@oss.qualcomm.com?part=3