linux-kernel - Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

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]

Date:   Tue, 12 Apr 2022 15:42:35 -0500
From:   Alex Elder <elder@...aro.org>
To:     Stephen Boyd <swboyd@...omium.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, patches@...ts.linux.dev,
        Taniya Das <quic_tdas@...cinc.com>
Subject: Re: [PATCH] clk: qcom: rpmh: Set wake/sleep state for BCM clks

On 4/12/22 2:45 PM, Stephen Boyd wrote:
> Set the wake and sleep state for BCM clks here, not just the active
> state, as the active only state is dropped when CPUs go to deep idle.
> This ensures the clk is always on when the driver thinks it is on.
> 
> This was found by inspection, and could very well be incorrect if the
> RPMh hardware copies over the active only state to the sleep and wake
> states.

I ran with this code and found no obvious errors.  And
it looks reasonable to me.

However someone (probably Taniya) needs to comment on
whether it's the right thing to do (based on Stephen's
second paragraph above), or if not, why not.  It might
warrant an additional comment also.

I can add a "Tested-by" but I'm going to wait for
some more input.

					-Alex

> Cc: Alex Elder <elder@...aro.org>
> Cc: Taniya Das <quic_tdas@...cinc.com>
> Fixes: 04053f4d23a4 ("clk: qcom: clk-rpmh: Add IPA clock support")
> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> ---
>   drivers/clk/qcom/clk-rpmh.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index aed907982344..29da1ffd10cf 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -260,6 +260,7 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>   	struct tcs_cmd cmd = { 0 };
>   	u32 cmd_state;
>   	int ret = 0;
> +	enum rpmh_state state;
>   
>   	mutex_lock(&rpmh_clk_lock);
>   	if (enable) {
> @@ -274,15 +275,19 @@ static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
>   		cmd.addr = c->res_addr;
>   		cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
>   
> -		ret = clk_rpmh_send(c, RPMH_ACTIVE_ONLY_STATE, &cmd, enable);
> -		if (ret) {
> -			dev_err(c->dev, "set active state of %s failed: (%d)\n",
> -				c->res_name, ret);
> -		} else {
> -			c->last_sent_aggr_state = cmd_state;
> +		for (state = RPMH_SLEEP_STATE; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
> +			ret = clk_rpmh_send(c, state, &cmd, enable);
> +			if (ret) {
> +				dev_err(c->dev, "set %s state of %s failed: (%d)\n",
> +					!state ? "sleep" :
> +					state == RPMH_WAKE_ONLY_STATE	?
> +					"wake" : "active", c->res_name, ret);
> +				goto out;
> +			}
>   		}
> +		c->last_sent_aggr_state = cmd_state;
>   	}
> -
> +out:
>   	mutex_unlock(&rpmh_clk_lock);
>   
>   	return ret;
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17