Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()

[Ulf Hansson]

On 29 November 2017 at 10:43, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> Hi Ulf,
>
> On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
>>>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
>>>> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>>>> <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
>>>> >> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>> <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.

Okay!

Can you perhaps clarify which 6 devices/drivers that are involved, and
perhaps also point out if their child devices?

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

Okay, so the problem remains no matter which solution for wakeup you
pick in genpd.

Then this seems to point to that the driver may be misbehaving in some
way. I can help to check what is going on.

Kind regards
Uffe