linux-kernel - Re: [PATCH] phy: qcom: edp: Add runtime PM support

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: <ZurRLf8S1j6s8GPz@linaro.org>
Date: Wed, 18 Sep 2024 16:10:05 +0300
From: Abel Vesa <abel.vesa@...aro.org>
To: Johan Hovold <johan@...nel.org>
Cc: Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] phy: qcom: edp: Add runtime PM support

On 24-09-18 12:05:59, Johan Hovold wrote:
> On Sat, Sep 07, 2024 at 06:25:21PM +0300, Abel Vesa wrote:
> > Enable runtime PM support by adding proper ops which will handle the
> 
> Avoid words like 'proper' here (what are non-proper runtime PM ops?).

Sure.

> 
> > clocks and regulators. These resources will now be handled on power_on and
> > power_off instead of init and exit PHY ops.
> 
> No, this is simply a false claim and indicates that you haven't reviewed
> how PHY runtime PM works. Core will increment the usage count on init()
> and decrement it on exit().

Yeah, I guess the better argument here would be that the PHY needs
regulators and clocks enabled. Anyway, ignore this version as it
was already NACKed by Dmitry.

> 
> > Also enable these resources on
> > probe in order to balance out the disabling that is happening right after.
> > Prevent runtime PM from being ON by default as well.
> 
> And here you just regressed all current systems that do not have udev
> rules to enable runtime PM, and which will now be stuck with these
> resources always-on (e.g. during DPMS off and system suspend).
> 
> In fact, you are even regressing systems that would enable runtime PM,
> as the runtime suspend callback would not currently be called when you
> enter system suspend so the regulators and clocks will be left on.
> 
> This clearly hasn't been tested and analysed properly.
> 
> > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev)
> > +{
> > +	struct qcom_edp *edp = dev_get_drvdata(dev);
> > +
> > +	dev_err(dev, "Suspending DP phy\n");
> 
> You forgot to drop your development printks (same below).
> 
> Johan