linux-kernel - Re: [PATCH v2 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] [day] [month] [year] [list]

Date:   Tue, 2 Jan 2018 14:28:14 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <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 v2 1/3] phy: core: Move runtime PM reference counting to
 the parent device

On 24 December 2017 at 13:00, Rafael J. Wysocki <rjw@...ysocki.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