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

[Ulf Hansson]

On 20 December 2017 at 07:42, Kishon Vijay Abraham I <kishon@xxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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.

Does it make sense?

Kind regards
Uffe