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] [day] [month] [year] [list]

Message-ID: <Zu06IXym0it9UPpa@linaro.org>
Date: Fri, 20 Sep 2024 12:02:25 +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-20 10:40:13, Johan Hovold wrote:
> On Wed, Sep 18, 2024 at 04:10:05PM +0300, Abel Vesa wrote:
> > 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
>  
> > > > 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
> 
> No, that's already handled today so is clearly not a valid argument.

I think we're saying the same thing here. I was trying to say that,
as it is currently done and it's correct, the init() needs those
resources enabled.

> 
> > Anyway, ignore this version as it was already NACKed by Dmitry.
> 
> No, my feedback is still valid, and you're bound to repeat the same
> mistakes over and over again unless you try to understand what I've been
> saying here.

Duly noted. My reply wasn't trying to dismiss your feedback.

Thanks for your feedback.

> 
> > > > 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.
> 
> Johan