linux-kernel - Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdViD3vqzits=th52+01d5y3p5iwbAg0jbUBcoGvYfULuA@mail.gmail.com>
Date:   Wed, 29 Nov 2017 10:43:29 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        USB list <linux-usb@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()

Hi Ulf,

On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> On 29 November 2017 at 09:21, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@...esas.com> wrote:
>>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
>>> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>>> <yoshihiro.shimoda.uh@...esas.com> wrote:
>>> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>>> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>>> >> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> <snip>
>>> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>>> >> R-Car H3:
>>> >>
>>> >>     ohci-platform ee080000.usb: runtime PM trying to suspend device
>>> >> but active child
>>> >>     phy_rcar_gen3_usb2 ee080200.usb-phy: runtime PM trying to suspend
>>> >> device but active child
>>> >>     ohci-platform ee0c0000.usb: runtime PM trying to suspend device
>>> >> but active child
>>> >>     ohci-platform ee0a0000.usb: runtime PM trying to suspend device
>>> >> but active child
>>> >>     phy_rcar_gen3_usb2 ee0c0200.usb-phy: runtime PM trying to suspend
>>> >> device but active child
>>> >>     phy_rcar_gen3_usb2 ee0a0200.usb-phy: runtime PM trying to suspend
>>> >> device but active child
>>> >>
>>> >> so this was an existing issue with USB before.
>>> >
>>> > Thank you for the report!
>>> > I know that, but since this didn't cause any trouble until now,
>>> > I postponed to investigate the issue... But, I investigate it today.
>>> > I don't find the root cause yet. However, it seems related to usb host and/or usb core.
>>> > --> USB host related devices' child_count will be 1 in suspend timing.
>>> >  --> I guess remote wakeup feature is enabled? But, I don't find the point yet.
>>>
>>> I am guessing the issue is triggered by genpd in the suspend noirq
>>> phase (genpd_suspend_noirq()). In there,  there is a call to
>>> pm_runtime_force_suspend() (which calls pm_runtime_set_suspended() and
>>> which triggered the earlier error messages being printed).
>>>
>>> The reason why genpd calls pm_runtime_force_suspend(), is because when
>>> validating wakeup configurations for the device "if
>>> (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))", it's
>>> thinks wakeup isn't configured while it probably should be.
>>>
>>> An additional note, only when genpd has the GENPD_FLAG_PM_CLK set,
>>> which makes the genpd->dev_ops.stop|start() being assigned, genpd
>>> calls pm_runtime_force_suspend() - else it doesn't.
>>>
>>> Perhaps try out the series I recently posted improving the code
>>> dealing with wakeups in genpd and the PM core:
>>> https://www.spinics.net/lists/linux-renesas-soc/msg20122.html
>>> To that, you need to set the new flag (invented in the above series)
>>> DPM_FLAG_IN_BAND_WAKEUP in the driver that configures wakeup of its
>>> device.
>>>
>>> Hope this helps!
>>
>> Thank you for the comments!
>> I tried DPM_FLAG_IN_BAND_WAKEUP, but the issue still exists.
>> I added the flag in the [eo]hci-platform driver and usb/core/driver.c.
>> I also added the flag in the phy_rcar_gen3_usb2 driver except usb host drivers.
>
> First, did you confirm that genpd was used? Then for what device?

All 6 devices are part of the SYSC PM Domain.

> Second, did you check the call to pm_runtime_force_suspend() called by
> genpd, is the reason to the error messages?
>
> Third, it should be sufficient to add DPM_FLAG_IN_BAND_WAKEUP for the
> driver that is actually dealing with the wakeup. Although, does this
> driver's system ->suspend() callback check device_may_wakeup(), before
> it decides to enable wakeup?
> If not, the PM core and genpd don't notice that wakeup is enabled for
> the device.

Actually I saw this with my patches setting GENPD_FLAG_ACTIVE_WAKEUP
for the SYSC PM Domain, which should trigger the same behavior.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds