linux-kernel - Re: [PATCH 2/2] clk: qcom: gdsc: Add pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <zhhna2wni4yqhnc2tqfc2ugril3h4kzbyr3ix6vpwrgghwytfa@kjfpff5zdl4z>
Date: Tue, 19 Nov 2024 09:34:46 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] clk: qcom: gdsc: Add pm_runtime hooks

On Mon, Nov 18, 2024 at 02:24:33AM +0000, Bryan O'Donoghue wrote:
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
[..]
> @@ -177,10 +182,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
>  	ret = gdsc_poll_status(sc, status);
>  	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>  
> -	if (!ret && status == GDSC_OFF && sc->rsupply) {
> -		ret = regulator_disable(sc->rsupply);
> -		if (ret < 0)
> -			return ret;
> +	if (!ret && status == GDSC_OFF) {
> +		if (pm_runtime_enabled(sc->dev))
> +			pm_runtime_put_sync(sc->dev);

I already made this mistake, and 4cc47e8add63 ("clk: qcom: gdsc: Remove
direct runtime PM calls") covers the reason why it was a mistake.

What I think you want is two things:
1) When you're accessing the registers, you want the clock controller's
power-domain to be on.
2) When the client vote for a GDSC, you want to have the PM framework
also ensure that parent power-domains are kept on.
For the single case, this is handled by the pm_genpd_add_subdomain()
call below. This, or something along those lines, seems like the
appropriate solution.

Regards,
Bjorn