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

From: Ulf Hansson
Date: Sat Dec 23 2017 - 10:09:44 EST


[...]

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