[v2,1/3] phy: core: Move runtime PM reference counting to the parent device
diff mbox

Message ID 1513778960-10073-2-git-send-email-ulf.hansson@linaro.org
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Ulf Hansson Dec. 20, 2017, 2:09 p.m. UTC
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.

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.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/phy/phy-core.c  | 33 +++++++++++++++------------------
 include/linux/phy/phy.h |  1 +
 2 files changed, 16 insertions(+), 18 deletions(-)

Comments

Rafael J. Wysocki Dec. 21, 2017, 1:39 a.m. UTC | #1
On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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. :-/

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

Thanks,
Rafael
Ulf Hansson Dec. 21, 2017, 10:50 a.m. UTC | #2
On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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?

Of course, if we had something that could replace
pm_runtime_force_suspend(), that would be great, but there isn't.

>
>> 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?

Kind regards
Uffe
Rafael J. Wysocki Dec. 23, 2017, 1:35 a.m. UTC | #3
On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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?

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

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

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.

So the conclusion may be that the phy code is OK, but calling
pm_runtime_force_suspend() from genpd isn't.

Thanks,
Rafael
Rafael J. Wysocki Dec. 23, 2017, 1:50 a.m. UTC | #4
On Sat, Dec 23, 2017 at 2:35 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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.

[cut]

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

Actually, I sort of agree that the phy's usage of runtime PM is too
convoluted.  For example, it uses pm_runtime_enabled() unnecessarily
at least in some places, but that doesn't seem to be fixed by your
patches.

Thanks,
Rafael
Ulf Hansson Dec. 23, 2017, 12:37 p.m. UTC | #5
On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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.

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

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

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

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

Kind regards
Uffe
Rafael J. Wysocki Dec. 23, 2017, 12:39 p.m. UTC | #6
On Thu, Dec 21, 2017 at 2:39 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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. :-/
>
>> 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.

So IMO the changes you are proposing make sense regardless of the
genpd issue, because they generally simplify the phy code, but the
additional use_runtime_pm field in struct phy represents redundant
information (manipulating reference counters shouldn't matter if
runtime PM is disabled), so it doesn't appear to be necessary.

[On a related note, I'm not sure why phy tries to intercept runtime PM
errors and "fix up" the reference counters.  That doesn't look right
to me at all.]

That said, the current phy code is not strictly invalid.  While it
looks more complicated than necessary, it doesn't do things documented
as invalid in principle, so saying "The behaviour around the runtime
PM deployment cause some issues during system suspend" in the
changelog is describing the problem from a very specific angle.
Simply put, pm_runtime_force_suspend() and the current phy code cannot
work together and so using them together is a bug.  None of them
individually is at fault, but combining them is incorrect.

Fortunately enough, the phy code can be modified so that it can be
used with pm_runtime_force_suspend() without problems, but picturing
it as "problematic", because it cannot do that today is not entirely
fair IMO.

Thanks,
Rafael
Rafael J. Wysocki Dec. 23, 2017, 12:47 p.m. UTC | #7
On Sat, Dec 23, 2017 at 1:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> 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
Ulf Hansson Dec. 23, 2017, 3:09 p.m. UTC | #8
[...]

>
> So IMO the changes you are proposing make sense regardless of the
> genpd issue, because they generally simplify the phy code, but the
> additional use_runtime_pm field in struct phy represents redundant
> information (manipulating reference counters shouldn't matter if
> runtime PM is disabled), so it doesn't appear to be necessary.
>

Actually, the first version I posted treated the return codes from
pm_runtime_get_sync() according to your suggestion above.

However, Kishon pointed out that it didn't work. That's because, there
are phy provider drivers that enables runtime PM *after* calling
phy_create(). And in those cases, that is because they want to treat
runtime PM themselves.

I think that's probably something we should look into to change, but I
find it being a separate issue, that I didn't want to investigate as
part of this series.

See more about the thread here:
https://www.spinics.net/lists/linux-renesas-soc/msg21711.html

> [On a related note, I'm not sure why phy tries to intercept runtime PM
> errors and "fix up" the reference counters.  That doesn't look right
> to me at all.]
>
> That said, the current phy code is not strictly invalid.  While it
> looks more complicated than necessary, it doesn't do things documented
> as invalid in principle, so saying "The behaviour around the runtime
> PM deployment cause some issues during system suspend" in the
> changelog is describing the problem from a very specific angle.
> Simply put, pm_runtime_force_suspend() and the current phy code cannot
> work together and so using them together is a bug.  None of them
> individually is at fault, but combining them is incorrect.
>
> Fortunately enough, the phy code can be modified so that it can be
> used with pm_runtime_force_suspend() without problems, but picturing
> it as "problematic", because it cannot do that today is not entirely
> fair IMO.

Right, this makes sense. Let me clarify this in the changelog.

Kind regards
Uffe
Rafael J. Wysocki Dec. 24, 2017, noon UTC | #9
On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote:
> [...]
> 
> >
> > So IMO the changes you are proposing make sense regardless of the
> > genpd issue, because they generally simplify the phy code, but the
> > additional use_runtime_pm field in struct phy represents redundant
> > information (manipulating reference counters shouldn't matter if
> > runtime PM is disabled), so it doesn't appear to be necessary.
> >
> 
> Actually, the first version I posted treated the return codes from
> pm_runtime_get_sync() according to your suggestion above.
> 
> However, Kishon pointed out that it didn't work. That's because, there
> are phy provider drivers that enables runtime PM *after* calling
> phy_create(). And in those cases, that is because they want to treat
> runtime PM themselves.
> 
> I think that's probably something we should look into to change, but I
> find it being a separate issue, that I didn't want to investigate as
> part of this series.
> 
> See more about the thread here:
> https://www.spinics.net/lists/linux-renesas-soc/msg21711.html

Even so, it shouldn't matter when the provider enables runtime PM.

Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as
long as they are balanced properly regardless of whether or not
runtime PM is enabled for the device or it is enabled in the meantime.
Of course, the initial runtime PM status of the device must reflect
the values of the usage counters, but that should not be too hard to
ensure.

The reason why it matters here is because the phy core tries to be smart
about manipulating reference counters by itself and that's a mistake.

So it looks to me like the whole thing needs to be reworked and yes,
that should be done in the first place IMO, because it will make the
issue with genpd go away automatically.

[Why is phy_pm_runtime_get() there at all, for instance?  It seems
to have no users and I kind of don't see use cases for it.  Also
phy_pm_runtime_get_sync() is only used by the phy core in three
places - shouldn't be too hard to fix that.]

So the issue with genpd really seems to be a messenger here. :-)

Thanks,
Rafael
Ulf Hansson Jan. 2, 2018, 1:28 p.m. UTC | #10
On 24 December 2017 at 13:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote:
>> [...]
>>
>> >
>> > So IMO the changes you are proposing make sense regardless of the
>> > genpd issue, because they generally simplify the phy code, but the
>> > additional use_runtime_pm field in struct phy represents redundant
>> > information (manipulating reference counters shouldn't matter if
>> > runtime PM is disabled), so it doesn't appear to be necessary.
>> >
>>
>> Actually, the first version I posted treated the return codes from
>> pm_runtime_get_sync() according to your suggestion above.
>>
>> However, Kishon pointed out that it didn't work. That's because, there
>> are phy provider drivers that enables runtime PM *after* calling
>> phy_create(). And in those cases, that is because they want to treat
>> runtime PM themselves.
>>
>> I think that's probably something we should look into to change, but I
>> find it being a separate issue, that I didn't want to investigate as
>> part of this series.
>>
>> See more about the thread here:
>> https://www.spinics.net/lists/linux-renesas-soc/msg21711.html
>
> Even so, it shouldn't matter when the provider enables runtime PM.
>
> Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as
> long as they are balanced properly regardless of whether or not
> runtime PM is enabled for the device or it is enabled in the meantime.
> Of course, the initial runtime PM status of the device must reflect
> the values of the usage counters, but that should not be too hard to
> ensure.

Yes, this is probably the main reason.

However, as stated, I think we should look into addressing this
problem more carefully, in a separate next step.

>
> The reason why it matters here is because the phy core tries to be smart
> about manipulating reference counters by itself and that's a mistake.
>
> So it looks to me like the whole thing needs to be reworked and yes,
> that should be done in the first place IMO, because it will make the
> issue with genpd go away automatically.

Sorry, but I am not fully understanding what you suggest here. Perhaps
you didn't check patch2?

>
> [Why is phy_pm_runtime_get() there at all, for instance?  It seems
> to have no users and I kind of don't see use cases for it.  Also
> phy_pm_runtime_get_sync() is only used by the phy core in three
> places - shouldn't be too hard to fix that.]

Removing these APIs and functions is done in patch2, so I guess I am
already doing what you suggests above? No?

[...]

Kind regards
Uffe

Patch
diff mbox

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b4964b0..09588ec 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -153,12 +153,12 @@  int phy_pm_runtime_get(struct phy *phy)
 {
 	int ret;
 
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	ret = pm_runtime_get(&phy->dev);
+	ret = pm_runtime_get(phy->dev.parent);
 	if (ret < 0 && ret != -EINPROGRESS)
-		pm_runtime_put_noidle(&phy->dev);
+		pm_runtime_put_noidle(phy->dev.parent);
 
 	return ret;
 }
@@ -168,12 +168,12 @@  int phy_pm_runtime_get_sync(struct phy *phy)
 {
 	int ret;
 
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	ret = pm_runtime_get_sync(&phy->dev);
+	ret = pm_runtime_get_sync(phy->dev.parent);
 	if (ret < 0)
-		pm_runtime_put_sync(&phy->dev);
+		pm_runtime_put_sync(phy->dev.parent);
 
 	return ret;
 }
@@ -181,37 +181,37 @@  EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
 
 int phy_pm_runtime_put(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	return pm_runtime_put(&phy->dev);
+	return pm_runtime_put(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
 
 int phy_pm_runtime_put_sync(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	return pm_runtime_put_sync(&phy->dev);
+	return pm_runtime_put_sync(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
 
 void phy_pm_runtime_allow(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return;
 
-	pm_runtime_allow(&phy->dev);
+	pm_runtime_allow(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
 
 void phy_pm_runtime_forbid(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return;
 
-	pm_runtime_forbid(&phy->dev);
+	pm_runtime_forbid(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
 
@@ -774,10 +774,7 @@  struct phy *phy_create(struct device *dev, struct device_node *node,
 	if (ret)
 		goto put_dev;
 
-	if (pm_runtime_enabled(dev)) {
-		pm_runtime_enable(&phy->dev);
-		pm_runtime_no_callbacks(&phy->dev);
-	}
+	phy->use_runtime_pm = pm_runtime_enabled(dev);
 
 	return phy;
 
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 4f8423a..b4298a1 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -83,6 +83,7 @@  struct phy {
 	int			power_count;
 	struct phy_attrs	attrs;
 	struct regulator	*pwr;
+	bool			use_runtime_pm;
 };
 
 /**