linux-kernel - Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the parent device

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: <0c0cf0ba-e729-a1d8-ac0a-38c8eb13a5c6@ti.com>
Date:   Wed, 20 Dec 2017 14:32:01 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 1/3] phy: core: Move runtime PM reference counting to the
 parent device

Hi Ulf,

On Wednesday 20 December 2017 02:05 PM, Ulf Hansson wrote:
> On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@...com> wrote:
>> Hi Ulf,
>>
>> On Wednesday 20 December 2017 02:52 AM, Ulf Hansson wrote:
>>> The runtime PM deployment in the phy core is a bit unnecessary complicated
>>> and the main reason is because it operates on the phy device, which is
>>> created by the phy core and assigned as a child device of the phy provider
>>> device.
>>>
>>> Let's simplify the code, by replacing the existing calls to
>>> phy_pm_runtime_get_sync() and phy_pm_runtime_put(), with regular calls to
>>> pm_runtime_get_sync() and pm_runtime_put(). While doing that, let's also
>>> change to give the phy provider device as the parameter to the runtime PM
>>> calls. This together with adding error paths, that allows the phy
>>> provider device to be runtime PM disabled, enables further clean up the
>>> code. More precisely, we can simply avoid to enable runtime PM for the phy
>>> device altogether, so let's do that as well.
>>>
>>> More importantly, this change also fixes an issue for system suspend.
>>> Especially in those cases when the phy provider device gets put into a low
>>> power state via calling the pm_runtime_force_suspend() helper, as is the
>>> case for a Renesas SoC, which has the phy provider device attached to the
>>> generic PM domain.
>>>
>>> The problem in this case, is that pm_runtime_force_suspend() expects the
>>> child device of the provider device to be runtime suspended, else this will
>>> trigger a WARN splat (correctly) when runtime PM gets re-enabled at system
>>> resume.
>>>
>>> In the current case, even if phy_power_off() triggers a pm_runtime_put()
>>> during system suspend the phy device (child) doesn't get runtime suspended,
>>> because that is prevented in the system suspend phases. However, by
>>> avoiding to enable runtime PM, this problem goes away.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
>>> ---
>>>  drivers/phy/phy-core.c | 33 +++++++++++++--------------------
>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index b4964b0..9fa3f13 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -222,10 +222,10 @@ int phy_init(struct phy *phy)
>>>       if (!phy)
>>>               return 0;
>>>
>>> -     ret = phy_pm_runtime_get_sync(phy);
>>> -     if (ret < 0 && ret != -ENOTSUPP)
>>> +     ret = pm_runtime_get_sync(phy->dev.parent);
>>
>> Won't this make phy-core manage pm_runtime of phy_provider even though the
>> phy_provider might not intend it?
> 
> No it shouldn't.
> 
> There are two cases to consider around this.
> 
> 1) CONFIG_PM is unset. In this case pm_runtime_get_sync() will return
> 1, which is treated as succeeds by the error path.
> 
> 2) CONFIG_PM is set, but the phy provider don't use runtime PM, thus
> it hasn't called pm_runtime_enable() for its device. In this case,
> pm_runtime_get_sync() returns -EACCES, which is also treated as
> success by the error path.

There can be a case where the phy_provider uses runtime PM but doesn't want
phy-core to manage it.

Thanks
Kishon