Re: [PATCH v2 2/7] spi: qcom-qspi: Fix incomplete error handling in runtime PM

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 4/14/26 7:08 PM, Viken Dadhaniya wrote:
> The runtime PM functions had incomplete error handling that could leave the
> system in an inconsistent state. If any operation failed midway through
> suspend or resume, some resources would be left in the wrong state while
> others were already changed, leading to potential clock/power imbalances.
> 
> Fix by adding proper error checking for all operations and using goto-based
> cleanup to ensure all successfully acquired resources are properly released
> on any error.
> 
> Signed-off-by: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
> ---
>  drivers/spi/spi-qcom-qspi.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> index 7e39038160e0..38af859713a7 100644
> --- a/drivers/spi/spi-qcom-qspi.c
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -819,19 +819,31 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
>  	int ret;
>  
>  	/* Drop the performance state vote */
> -	dev_pm_opp_set_rate(dev, 0);
> +	ret = dev_pm_opp_set_rate(dev, 0);
> +	if (ret)
> +		return ret;
> +
>  	clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);

You first need to disable the clocks and only then potentially reduce the
performance state, otherwise there's a brief period of brownout risk

[...]

>  static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev)
> @@ -840,20 +852,34 @@ static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev)
>  	struct qcom_qspi *ctrl = spi_controller_get_devdata(host);
>  	int ret;
>  
> -	pinctrl_pm_select_default_state(dev);
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		return ret;
>  
>  	ret = icc_enable(ctrl->icc_path_cpu_to_qspi);
>  	if (ret) {
>  		dev_err_ratelimited(ctrl->dev, "%s: ICC enable failed for cpu: %d\n",
>  			__func__, ret);
> -		return ret;
> +		goto err_select_sleep_state;
>  	}
>  
>  	ret = clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks);
>  	if (ret)
> -		return ret;
> +		goto err_disable_icc;
>  
> -	return dev_pm_opp_set_rate(dev, ctrl->last_speed * 4);
> +	ret = dev_pm_opp_set_rate(dev, ctrl->last_speed * 4);
> +	if (ret)
> +		goto err_disable_clk;

similarly here, the OPP state is only altered after the clocks are
running (potentially at a high speed because the rate is cached)

Konrad