Re: [PATCH v4 3/4] spi: qcom-geni: Use resources helper APIs in runtime PM functions

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch relies on APIs and structure fields that do not exist in the baseline kernel tree or the rest of the patch series, causing fatal compilation errors.

Pre-existing issues:
- [High] Hardware registers are accessed without checking if `pm_runtime_get_sync()` successfully resumed the device.
--

commit 8edc09ad870bfce8d07091d2faeae9667787e1f0
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use resources helper APIs in runtime PM functions

This commit updates the geni SPI driver's runtime power management
functions to use streamlined resource activation and deactivation
helpers. It aims to eliminate code duplication across drivers.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 09feb6577efcb..025165c4b9527 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1130,16 +1130,8 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
>  {
>  	struct spi_controller *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> -	int ret;
> -
> -	/* Drop the performance state vote */
> -	dev_pm_opp_set_rate(dev, 0);
> -
> -	ret = geni_se_resources_off(&mas->se);
> -	if (ret)
> -		return ret;
>  
> -	return geni_icc_disable(&mas->se);
> +	return geni_se_resources_deactivate(&mas->se);

[Severity: Low]
It appears the geni_se_resources_deactivate() and
geni_se_resources_activate() functions are not implemented in the baseline
tree, which could lead to immediate build failures.

Are these new helper APIs defined elsewhere in this patch series?

>  }
>  
>  static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
> @@ -1148,15 +1140,14 @@ static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
>  	int ret;
>  
> -	ret = geni_icc_enable(&mas->se);
> +	ret = geni_se_resources_activate(&mas->se);
>  	if (ret)
>  		return ret;
>  
> -	ret = geni_se_resources_on(&mas->se);
> -	if (ret)
> -		return ret;
> +	if (mas->se.has_opp)

[Severity: Low]
The has_opp field appears to be missing from the definition of
struct geni_se, which would cause a compilation error.

Does the has_opp field exist in struct geni_se in another patch within
this series?

> +		return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
>  
> -	return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
> +	return 0;
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but looking at spi_geni_init() in
drivers/spi/spi-geni-qcom.c:

	pm_runtime_get_sync(mas->dev);

	proto = geni_se_read_proto(se);

If pm_runtime_get_sync() fails to resume the device, accessing unclocked
or powered-off registers via geni_se_read_proto() could trigger a
synchronous external abort and result in a kernel panic.

Should the return value of pm_runtime_get_sync() be checked before
accessing hardware registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-enable-spi-on-sa8255p-v4-0-f5b5067e7e1e@oss.qualcomm.com?part=3