linux-kernel - Re: [PATCH] Revert "clk: gcc-sc8280xp: keep PCIe power-domains always-on"

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: <ZKbDztmNp-KMXTDu@hovoldconsulting.com>
Date:   Thu, 6 Jul 2023 15:38:22 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     andersson@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
        konrad.dybcio@...aro.org, johan+linaro@...nel.org,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "clk: gcc-sc8280xp: keep PCIe power-domains
 always-on"

On Thu, Jul 06, 2023 at 06:17:00PM +0530, Manivannan Sadhasivam wrote:
> This reverts commit 12d2a4769380f0dc9ba6f827839869db2b81ef00.

Please update the commit summary and remove or rephrase the above as
direct reverts are typically used for patches that were broken or causes
trouble (e.g rephrase as "allow pcie gdsdc to be disabled" or similar).

The patch in question was correct at the time even if it may no longer
be required, but see below.

> With the minimal system suspend support in place for the PCIe driver that
> keeps the interconnect path active, the ALWAYS_ON flags can now be dropped.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> ---
>  drivers/clk/qcom/gcc-sc8280xp.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index 04a99dbaa57e..07eb6110442c 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -6774,10 +6774,6 @@ static struct gdsc pcie_1_tunnel_gdsc = {
>  	.flags = VOTABLE,
>  };
>  
> -/*
> - * The Qualcomm PCIe driver does not yet implement suspend so to keep the
> - * PCIe power domains always-on for now.
> - */
>  static struct gdsc pcie_2a_gdsc = {
>  	.gdscr = 0x9d004,
>  	.collapse_ctrl = 0x52128,
> @@ -6786,7 +6782,7 @@ static struct gdsc pcie_2a_gdsc = {
>  		.name = "pcie_2a_gdsc",
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.flags = VOTABLE | ALWAYS_ON,
> +	.flags = VOTABLE,
>  };

Are you sure this is correct? Won't the controller state be lost if the
GDSC is powered off during suspend? Shouldn't this be PWRSTS_RET_ON at
least?

Johan