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

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:37 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>>>> The runtime PM deployment in the phy core is deployed using the phy core
>>>>> device, which is created by the phy core and assigned as a child device of
>>>>> the phy provider device.
>>>>>
>>>>> The behaviour around the runtime PM deployment cause some issues during
>>>>> system suspend, in cases when the phy provider device is put into a low
>>>>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>>>>> case for a Renesas SoC, which has its phy provider device attached to the
>>>>> generic PM domain.
>>>>>
>>>>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>>>>> expects the child device of the provider device, which is the phy core
>>>>> device, to be runtime suspended, else a WARN splat will be printed
>>>>> (correctly) when runtime PM gets re-enabled at system resume.
>>>>
>>>> So we are now trying to work around issues with
>>>> pm_runtime_force_suspend().  Lovely. :-/
>>>
>>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
>>> are you saying we should just ignore all issues related to it?
>>
>> Or get rid of it?
>>
>> Seriously, is the resulting pain worth it?
>
> I am not sure what pain you refer to? :-) So far I haven't got much
> errors being reported because of its use, have you?
>
> Moreover, to me, this small series fixes the problems in a rather trivial way.

Well, I agree here (please see the message regarding that I've just
sent in this thread).

>>
>>> Of course, if we had something that could replace
>>> pm_runtime_force_suspend(), that would be great, but there isn't.
>>
>> I beg to differ.
>>
>> At least some of it could be replaced with the driver flags.
>
> Yes, true.
>
> On the other hand that is exactly the problem, only *some*. And more
> importantly, genpd can't use them, because it can't cope with have
> system wide PM callbacks to be skipped.

Its own system-wide PM callbacks cannot be skipped, but it already
skips driver callbacks in some cases. :-)

> In other words, so far, the driver PM flags can't solve this issue.

It is unclear which issue you are talking about, but anyway, yes, they
aren't equivalent to pm_runtime_force_suspend/resume() which is quite
intentional, honestly ...

>>>>> In the current scenario, even if a call to phy_power_off() triggers it to
>>>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>>>>> get runtime suspended, because this is prevented in the system suspend
>>>>> phases by the PM core.
>>>>>
>>>>> To solve this problem, let's move the runtime PM deployment from the phy
>>>>> core device to the phy provider device, as this provides the similar
>>>>> behaviour. Changing this makes it redundant to enable runtime PM for the
>>>>> phy core device, so let's avoid doing that.
>>>>
>>>> I'm not really convinced that this approach is the best one to be honest.
>>>>
>>>> I'll have a deeper look at this in the next few days, stay tuned.
>>>
>>> There is different ways to solve this, for sure. I picked this one,
>>> because I think it's the most trivial thing to do, and it shouldn't
>>> cause any other problems.
>>>
>>> I think any other option would involve assigning ->suspend|resume()
>>> callbacks to the phy core device, but that's fine too, if you prefer
>>> that.
>>>
>>> Also, I have considered how to deal with wakeup paths for phys,
>>> although I didn't want to post changes as a part of this series, but
>>> maybe I should to give a more complete picture?
>>
>> Yes, you should.
>
> Okay!
>
>>
>> The point is that without genpd using pm_runtime_force_suspend() the
>> phy code could very well stay the way it is.  And it is logical,
>> because having a parent with enabled runtime PM without enabling
>> runtime PM for its children is at least conceptually questionable.
>
> I agree that the phy core is today logical okay. But I think that's
> also the case, if we pick up this small series.
>
> There are many reasons and cases where a children is runtime PM
> disabled, while the parent is runtime PM enabled. Moreover the runtime
> PM core copes fine with that.

Fair enough.

>>
>> So the conclusion may be that the phy code is OK, but calling
>> pm_runtime_force_suspend() from genpd isn't.
>
> So I am worried that changing genpd in this regards, may introduce
> other regressions. @subject series seems far less risky - and again,
> to me, the changes are trivial.
>
> Anyway, I will keep your suggestion in mind and think about, how/if
> genpd can be changed.

Thanks a lot!

Rafael