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: <ZuqmB3Cn7mGfA2PU@hovoldconsulting.com>
Date: Wed, 18 Sep 2024 12:05:59 +0200
From: Johan Hovold <johan@...nel.org>
To: Abel Vesa <abel.vesa@...aro.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 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?).

> 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().

> 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