Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Brian Norris <briannorris@xxxxxxxxxxxx>]

(NB: I have several nearly identical copies of this email. I'm replying
to the latest one I see.)

Hi Jon,

On Sun, Aug 25, 2024 at 11:54:22AM +0800, Jon Lin wrote:
> Fix WARN_ON:
> [   22.869352][ T1885] clk_spi0 already unprepared
> [   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
> [   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
> [   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
> [   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
> [   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
> [   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
> [   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

I appreciate the snippet of a WARNING trace, but I'd also appreciate
some actual explanation of what the problem is, and why you're solving
it this way.

> Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
> Signed-off-by: Jon Lin <jon.lin@xxxxxxxxxxxxxx>
> ---
> 
>  drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index e1ecd96c7858..043a7739c330 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c

> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_spi_suspend(struct device *dev)
>  {
> +	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
> -	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
> -	clk_disable_unprepare(rs->spiclk);
> -	clk_disable_unprepare(rs->apb_pclk);
> +	ret = spi_controller_suspend(ctlr);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Avoid redundant clock disable */
> +	if (!pm_runtime_status_suspended(dev))
> +		rockchip_spi_runtime_suspend(dev);

It seems like you'd really be served well by
pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
driver used to use before the breaking change (commit
e882575efc77). Why aren't you just going back to using it? (This is the
kind of thing I might expect in your commit message -- reasoning as to
why you're doing what you're doing.)

And in fact, I already submitted a patch that resolves the above problem
and does exactly that:

https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@xxxxxxxxxxxx/
[PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling

Do you see any problem with it?

Thanks,
Brian

> +	pinctrl_pm_select_sleep_state(dev);
>  
>  	return 0;
>  }