Re: [PATCH v3 5/8] spi: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: spi_geni_runtime_suspend() desynchronizes PM state if geni_se_set_rate() fails
- [Medium] pm: spi_geni_runtime_resume() leaks clock/power resources on geni_se_set_rate() error
--

commit 18dd7909ccc7f7c6dc80af1c7154961b4692ab24
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

On the SA8255P platform there is no Linux clock handler for the SE
source clock, so the driver has no way to look up the source clock
index and divider needed to fill the GSI CONFIG0 TRE for a requested
transfer frequency. To work around this, firmware instead exposes the
SE HW supported source clock frequencies as OPPs on the perf domain
device.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 91e75e213f929..cbccd6d8689ab 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
[ ... ]
> @@ -1248,9 +1246,14 @@ static int 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;
> +
> +	ret = mas->dev_data->power_off ?
> +	      mas->dev_data->power_off(&mas->se) : 0;
> +	if (ret)
> +		return ret;
>  
> -	return mas->dev_data->power_off ?
> -	       mas->dev_data->power_off(&mas->se) : 0;
> +	return geni_se_set_rate(&mas->se, 0);
>  }

[Severity: High]
Does this desynchronize the PM state if geni_se_set_rate() fails?

If mas->dev_data->power_off() successfully powers off the device (disabling
clocks, interconnects, etc.), but geni_se_set_rate() subsequently fails and
returns a negative error code, the PM core will abort the suspend transition
and leave dev->power.runtime_status as RPM_ACTIVE.

Since the PM core considers the device active, will subsequent operations
bypass the pm_runtime_get_sync() resume path and directly access hardware
registers while the clocks are actually disabled, potentially leading to an
unclocked access exception?

Could the error from geni_se_set_rate() be ignored or handled gracefully,
or should geni_se_set_rate(0) be called before power_off()?

>  static int spi_geni_runtime_resume(struct device *dev)
> @@ -1265,10 +1268,7 @@ static int spi_geni_runtime_resume(struct device *dev)
>  			return ret;
>  	}
>  
> -	if (mas->se.has_opp)
> -		return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
> -
> -	return 0;
> +	return geni_se_set_rate(&mas->se, mas->cur_sclk_hz);
>  }

[Severity: Medium]
Can this leak clock and power resources if geni_se_set_rate() fails?

If power_on() successfully enables the device clocks and interconnects, but
geni_se_set_rate() fails and returns an error, this function returns that
error directly without first calling power_off() to unwind the state.

The PM core will then mark the resume as failed and transition the device
state back to RPM_SUSPENDED, orphaning the active hardware resources.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=5