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

[Ulf Hansson]

On 5 December 2017 at 04:23, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
>>
>> On 1 December 2017 at 12:03, Yoshihiro Shimoda
>>  wrote:
> 
>> > Sure! I tested your patch, and then the following message disappeared!
>> >
>> >Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
>> > children
>>
>> Great, that confirms my theory.
>>
>> I will re-work the patch and re-post it to see what people thinks about it.
>
> Thank you!
>
>> >
>> > However, the following message still exists.
>> >
>> >Enabling runtime PM for inactive device (ee08.usb) with active 
>> > children
>> >
>> > So, I guess ohci-platform.c also has similar issue.
>>
>> Yes, very likely!
>>
>> However, I need some more time to look into this to be able to suggest
>> a solution.
>
> I found a solution and sent a report as below:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html
>
> What do you think about using pm_runtime_forbid()?

As both Alan and Rafael pointed out in their replies, this is not the
right solution.

However, what is indeed interesting is how it can silence the error
messages. I keep this is mind while later moving forward.

Kind regards
Uffe

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

[Ulf Hansson]

On 5 December 2017 at 04:23, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
>>
>> On 1 December 2017 at 12:03, Yoshihiro Shimoda
>>  wrote:
> 
>> > Sure! I tested your patch, and then the following message disappeared!
>> >
>> >Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
>> > children
>>
>> Great, that confirms my theory.
>>
>> I will re-work the patch and re-post it to see what people thinks about it.
>
> Thank you!
>
>> >
>> > However, the following message still exists.
>> >
>> >Enabling runtime PM for inactive device (ee08.usb) with active 
>> > children
>> >
>> > So, I guess ohci-platform.c also has similar issue.
>>
>> Yes, very likely!
>>
>> However, I need some more time to look into this to be able to suggest
>> a solution.
>
> I found a solution and sent a report as below:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html
>
> What do you think about using pm_runtime_forbid()?

As both Alan and Rafael pointed out in their replies, this is not the
right solution.

However, what is indeed interesting is how it can silence the error
messages. I keep this is mind while later moving forward.

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Tue, Dec 5, 2017 at 4:03 PM, Alan Stern  wrote:
> On Tue, 5 Dec 2017, Yoshihiro Shimoda wrote:
>
>> Hi,
>>
>> > From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
>> >
>> > On 1 December 2017 at 12:03, Yoshihiro Shimoda
>> >  wrote:
>> 
>> > > Sure! I tested your patch, and then the following message disappeared!
>> > >
>> > >Enabling runtime PM for inactive device (ee080200.usb-phy) with 
>> > > active children
>> >
>> > Great, that confirms my theory.
>> >
>> > I will re-work the patch and re-post it to see what people thinks about it.
>>
>> Thank you!
>>
>> > >
>> > > However, the following message still exists.
>> > >
>> > >Enabling runtime PM for inactive device (ee08.usb) with active 
>> > > children
>> > >
>> > > So, I guess ohci-platform.c also has similar issue.
>> >
>> > Yes, very likely!
>> >
>> > However, I need some more time to look into this to be able to suggest
>> > a solution.
>>
>> I found a solution and sent a report as below:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html
>>
>> What do you think about using pm_runtime_forbid()?
>
> In general, drivers should not use pm_runtime_forbid().

I'd rather say that it's not very useful to them. :-)

> It is meant for use by userspace, through the /sys/.../power/control file.

Or when the driver wants to change the default setting.

> Drivers cannot rely on the result of calling pm_runtime_forbid()
> or pm_runtime_allow(), because the user can change it at any time.

Right.

> If you really want to prevent the OHCI controller from going into
> runtime suspend, the proper approach is to call pm_runtime_get() in the
> probe routine and pm_runtime_put() in the release routine.  However,
> this will waste energy because it will force the controller to remain
> at full power even when no active devices are attached.
>
> In this case, there probably is a better to solution to your problem
> (such as fixing the runtime PM support in the phy driver).

Agreed.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Tue, Dec 5, 2017 at 4:03 PM, Alan Stern  wrote:
> On Tue, 5 Dec 2017, Yoshihiro Shimoda wrote:
>
>> Hi,
>>
>> > From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
>> >
>> > On 1 December 2017 at 12:03, Yoshihiro Shimoda
>> >  wrote:
>> 
>> > > Sure! I tested your patch, and then the following message disappeared!
>> > >
>> > >Enabling runtime PM for inactive device (ee080200.usb-phy) with 
>> > > active children
>> >
>> > Great, that confirms my theory.
>> >
>> > I will re-work the patch and re-post it to see what people thinks about it.
>>
>> Thank you!
>>
>> > >
>> > > However, the following message still exists.
>> > >
>> > >Enabling runtime PM for inactive device (ee08.usb) with active 
>> > > children
>> > >
>> > > So, I guess ohci-platform.c also has similar issue.
>> >
>> > Yes, very likely!
>> >
>> > However, I need some more time to look into this to be able to suggest
>> > a solution.
>>
>> I found a solution and sent a report as below:
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html
>>
>> What do you think about using pm_runtime_forbid()?
>
> In general, drivers should not use pm_runtime_forbid().

I'd rather say that it's not very useful to them. :-)

> It is meant for use by userspace, through the /sys/.../power/control file.

Or when the driver wants to change the default setting.

> Drivers cannot rely on the result of calling pm_runtime_forbid()
> or pm_runtime_allow(), because the user can change it at any time.

Right.

> If you really want to prevent the OHCI controller from going into
> runtime suspend, the proper approach is to call pm_runtime_get() in the
> probe routine and pm_runtime_put() in the release routine.  However,
> this will waste energy because it will force the controller to remain
> at full power even when no active devices are attached.
>
> In this case, there probably is a better to solution to your problem
> (such as fixing the runtime PM support in the phy driver).

Agreed.

Thanks,
Rafael

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

[Alan Stern]

On Tue, 5 Dec 2017, Yoshihiro Shimoda wrote:

> Hi,
> 
> > From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
> > 
> > On 1 December 2017 at 12:03, Yoshihiro Shimoda
> >  wrote:
> 
> > > Sure! I tested your patch, and then the following message disappeared!
> > >
> > >Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
> > > children
> > 
> > Great, that confirms my theory.
> > 
> > I will re-work the patch and re-post it to see what people thinks about it.
> 
> Thank you!
> 
> > >
> > > However, the following message still exists.
> > >
> > >Enabling runtime PM for inactive device (ee08.usb) with active 
> > > children
> > >
> > > So, I guess ohci-platform.c also has similar issue.
> > 
> > Yes, very likely!
> > 
> > However, I need some more time to look into this to be able to suggest
> > a solution.
> 
> I found a solution and sent a report as below:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html
> 
> What do you think about using pm_runtime_forbid()?

In general, drivers should not use pm_runtime_forbid().

It is meant for use by userspace, through the /sys/.../power/control 
file.  Drivers cannot rely on the result of calling pm_runtime_forbid() 
or pm_runtime_allow(), because the user can change it at any time.

If you really want to prevent the OHCI controller from going into 
runtime suspend, the proper approach is to call pm_runtime_get() in the 
probe routine and pm_runtime_put() in the release routine.  However, 
this will waste energy because it will force the controller to remain 
at full power even when no active devices are attached.

In this case, there probably is a better to solution to your problem 
(such as fixing the runtime PM support in the phy driver).

Alan Stern

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

[Alan Stern]

On Tue, 5 Dec 2017, Yoshihiro Shimoda wrote:

> Hi,
> 
> > From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
> > 
> > On 1 December 2017 at 12:03, Yoshihiro Shimoda
> >  wrote:
> 
> > > Sure! I tested your patch, and then the following message disappeared!
> > >
> > >Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
> > > children
> > 
> > Great, that confirms my theory.
> > 
> > I will re-work the patch and re-post it to see what people thinks about it.
> 
> Thank you!
> 
> > >
> > > However, the following message still exists.
> > >
> > >Enabling runtime PM for inactive device (ee08.usb) with active 
> > > children
> > >
> > > So, I guess ohci-platform.c also has similar issue.
> > 
> > Yes, very likely!
> > 
> > However, I need some more time to look into this to be able to suggest
> > a solution.
> 
> I found a solution and sent a report as below:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html
> 
> What do you think about using pm_runtime_forbid()?

In general, drivers should not use pm_runtime_forbid().

It is meant for use by userspace, through the /sys/.../power/control 
file.  Drivers cannot rely on the result of calling pm_runtime_forbid() 
or pm_runtime_allow(), because the user can change it at any time.

If you really want to prevent the OHCI controller from going into 
runtime suspend, the proper approach is to call pm_runtime_get() in the 
probe routine and pm_runtime_put() in the release routine.  However, 
this will waste energy because it will force the controller to remain 
at full power even when no active devices are attached.

In this case, there probably is a better to solution to your problem 
(such as fixing the runtime PM support in the phy driver).

Alan Stern

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
> 
> On 1 December 2017 at 12:03, Yoshihiro Shimoda
>  wrote:

> > Sure! I tested your patch, and then the following message disappeared!
> >
> >Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
> > children
> 
> Great, that confirms my theory.
> 
> I will re-work the patch and re-post it to see what people thinks about it.

Thank you!

> >
> > However, the following message still exists.
> >
> >Enabling runtime PM for inactive device (ee08.usb) with active 
> > children
> >
> > So, I guess ohci-platform.c also has similar issue.
> 
> Yes, very likely!
> 
> However, I need some more time to look into this to be able to suggest
> a solution.

I found a solution and sent a report as below:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html

What do you think about using pm_runtime_forbid()?

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM
> 
> On 1 December 2017 at 12:03, Yoshihiro Shimoda
>  wrote:

> > Sure! I tested your patch, and then the following message disappeared!
> >
> >Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
> > children
> 
> Great, that confirms my theory.
> 
> I will re-work the patch and re-post it to see what people thinks about it.

Thank you!

> >
> > However, the following message still exists.
> >
> >Enabling runtime PM for inactive device (ee08.usb) with active 
> > children
> >
> > So, I guess ohci-platform.c also has similar issue.
> 
> Yes, very likely!
> 
> However, I need some more time to look into this to be able to suggest
> a solution.

I found a solution and sent a report as below:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1551146.html

What do you think about using pm_runtime_forbid()?

Best regards,
Yoshihiro Shimoda

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

[Ulf Hansson]

On 1 December 2017 at 12:03, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Friday, December 1, 2017 6:22 PM
>>
>> + Kishon
>>
>> On 30 November 2017 at 13:51, Yoshihiro Shimoda
>>  wrote:
>> > Hi,
>> >
>> >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
>> >>
>> >> On 29 November 2017 at 10:43, Geert Uytterhoeven  
>> >> wrote:
>> >> > Hi Ulf,
>> > 
>> >> Okay, so the problem remains no matter which solution for wakeup you
>> >> pick in genpd.
>> >
>> > Yes. Today I could reproduce this issue without usb host driver.
>> > - The renesas_usb3 usb peripheral driver has generic phy handling.
>> >   (The peripheral driver uses different generic phy driver 
>> > (phy-rcar-gen3-usb3.c) though.)
>> >  --> If I used the current renesas_usb3 (this means doesn't call 
>> > phy_power_{on,off}(),
>> >  the issue didn't happen.
>> >  --> If I added phy_power_{on,off}() calling, the issue happened.
>> >   --> So, I'm thinking the APIs are related to the issue.
>>
>> Yes.
>>
>> >
>> > - The generic phy APIs are in drivers/phy/phy-core.c.
>> >  --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() 
>> > before devm_phy_create().
>> >   --> The phy-core will call pm_runtime_{get_sync,put}() in 
>> > phy_{init,exit,power_{on,off}}.
>> >--> So, IIUC, both devices of phy-. and  will 
>> > be handled by runtime PM APIs.
>> >  --> The runtime PM implementation of phy-core seems good to me. But...?
>>
>>
>> I have digested the information that you and Geert provided, thanks!
>>
>> So, my conclusions so far is:
>>
>> The phy core is using runtime PM reference counting at
>> phy_power_on|off(). Although it does that on the phy core device,
>> which is a child device of the phy provider device.
>>
>> Because phy_power_off() is called during system suspend from phy
>> consumer drivers like usb, the phy core device (child) and the phy
>> provider device (parent) will never become runtime suspended (because
>> the PM core has invoked pm_runtime_get_no_resume() for all device in
>> the device prepare phase).
>>
>> Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq
>> phase for the phy provider device, the call to
>> pm_runtime_set_suspended() in there, triggers the earlier error
>> message, which is because the child (phy core device) is still runtime
>> resumed.
>
> Thank you very much for the conclusions!
> It's helpful to me about runtime PM behavior.
>
>> >> Then this seems to point to that the driver may be misbehaving in some
>> >> way. I can help to check what is going on.
>> >
>> > I guess so. But, I don't find yet...
>>
>> I think the below patch will help, although I am not sure if that is
>> sufficient as a long term fix.
>
> Thank you very much for your help!
> Also, I'm not sure how to fix for a long term kernels though...
>
>> Can you please try and see if it solves the problems?
>
> Sure! I tested your patch, and then the following message disappeared!
>
>Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
> children

Great, that confirms my theory.

I will re-work the patch and re-post it to see what people thinks about it.

>
> However, the following message still exists.
>
>Enabling runtime PM for inactive device (ee08.usb) with active children
>
> So, I guess ohci-platform.c also has similar issue.

Yes, very likely!

However, I need some more time to look into this to be able to suggest
a solution.

>
> JFYI, the ehci-platform.c doesn't have runtime PM handling.
> So, I think that error message doesn't output from ehci devices.

Right, thanks!

Kind regards
Uffe

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

[Ulf Hansson]

On 1 December 2017 at 12:03, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Friday, December 1, 2017 6:22 PM
>>
>> + Kishon
>>
>> On 30 November 2017 at 13:51, Yoshihiro Shimoda
>>  wrote:
>> > Hi,
>> >
>> >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
>> >>
>> >> On 29 November 2017 at 10:43, Geert Uytterhoeven  
>> >> wrote:
>> >> > Hi Ulf,
>> > 
>> >> Okay, so the problem remains no matter which solution for wakeup you
>> >> pick in genpd.
>> >
>> > Yes. Today I could reproduce this issue without usb host driver.
>> > - The renesas_usb3 usb peripheral driver has generic phy handling.
>> >   (The peripheral driver uses different generic phy driver 
>> > (phy-rcar-gen3-usb3.c) though.)
>> >  --> If I used the current renesas_usb3 (this means doesn't call 
>> > phy_power_{on,off}(),
>> >  the issue didn't happen.
>> >  --> If I added phy_power_{on,off}() calling, the issue happened.
>> >   --> So, I'm thinking the APIs are related to the issue.
>>
>> Yes.
>>
>> >
>> > - The generic phy APIs are in drivers/phy/phy-core.c.
>> >  --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() 
>> > before devm_phy_create().
>> >   --> The phy-core will call pm_runtime_{get_sync,put}() in 
>> > phy_{init,exit,power_{on,off}}.
>> >--> So, IIUC, both devices of phy-. and  will 
>> > be handled by runtime PM APIs.
>> >  --> The runtime PM implementation of phy-core seems good to me. But...?
>>
>>
>> I have digested the information that you and Geert provided, thanks!
>>
>> So, my conclusions so far is:
>>
>> The phy core is using runtime PM reference counting at
>> phy_power_on|off(). Although it does that on the phy core device,
>> which is a child device of the phy provider device.
>>
>> Because phy_power_off() is called during system suspend from phy
>> consumer drivers like usb, the phy core device (child) and the phy
>> provider device (parent) will never become runtime suspended (because
>> the PM core has invoked pm_runtime_get_no_resume() for all device in
>> the device prepare phase).
>>
>> Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq
>> phase for the phy provider device, the call to
>> pm_runtime_set_suspended() in there, triggers the earlier error
>> message, which is because the child (phy core device) is still runtime
>> resumed.
>
> Thank you very much for the conclusions!
> It's helpful to me about runtime PM behavior.
>
>> >> Then this seems to point to that the driver may be misbehaving in some
>> >> way. I can help to check what is going on.
>> >
>> > I guess so. But, I don't find yet...
>>
>> I think the below patch will help, although I am not sure if that is
>> sufficient as a long term fix.
>
> Thank you very much for your help!
> Also, I'm not sure how to fix for a long term kernels though...
>
>> Can you please try and see if it solves the problems?
>
> Sure! I tested your patch, and then the following message disappeared!
>
>Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
> children

Great, that confirms my theory.

I will re-work the patch and re-post it to see what people thinks about it.

>
> However, the following message still exists.
>
>Enabling runtime PM for inactive device (ee08.usb) with active children
>
> So, I guess ohci-platform.c also has similar issue.

Yes, very likely!

However, I need some more time to look into this to be able to suggest
a solution.

>
> JFYI, the ehci-platform.c doesn't have runtime PM handling.
> So, I think that error message doesn't output from ehci devices.

Right, thanks!

Kind regards
Uffe

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

[Yoshihiro Shimoda]

Hi again,

> From: Yoshihiro Shimoda, Sent: Friday, December 1, 2017 8:04 PM
> 
> Hi,
> 

> However, the following message still exists.
> 
>Enabling runtime PM for inactive device (ee08.usb) with active children
> 
> So, I guess ohci-platform.c also has similar issue.
> 
> JFYI, the ehci-platform.c doesn't have runtime PM handling.
> So, I think that error message doesn't output from ehci devices.

I have update.
If I added to call pm_runtime_forbid() in ohci-platform.c like xhci-plat.c,
the issue disappeared. I don't understand the pm_runtime_forbid() behavior yet,
but is this acceptable?

Best regards,
Yoshihiro Shimoda

> Best regards,
> Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi again,

> From: Yoshihiro Shimoda, Sent: Friday, December 1, 2017 8:04 PM
> 
> Hi,
> 

> However, the following message still exists.
> 
>Enabling runtime PM for inactive device (ee08.usb) with active children
> 
> So, I guess ohci-platform.c also has similar issue.
> 
> JFYI, the ehci-platform.c doesn't have runtime PM handling.
> So, I think that error message doesn't output from ehci devices.

I have update.
If I added to call pm_runtime_forbid() in ohci-platform.c like xhci-plat.c,
the issue disappeared. I don't understand the pm_runtime_forbid() behavior yet,
but is this acceptable?

Best regards,
Yoshihiro Shimoda

> Best regards,
> Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Friday, December 1, 2017 6:22 PM
> 
> + Kishon
> 
> On 30 November 2017 at 13:51, Yoshihiro Shimoda
>  wrote:
> > Hi,
> >
> >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
> >>
> >> On 29 November 2017 at 10:43, Geert Uytterhoeven  
> >> wrote:
> >> > Hi Ulf,
> > 
> >> Okay, so the problem remains no matter which solution for wakeup you
> >> pick in genpd.
> >
> > Yes. Today I could reproduce this issue without usb host driver.
> > - The renesas_usb3 usb peripheral driver has generic phy handling.
> >   (The peripheral driver uses different generic phy driver 
> > (phy-rcar-gen3-usb3.c) though.)
> >  --> If I used the current renesas_usb3 (this means doesn't call 
> > phy_power_{on,off}(),
> >  the issue didn't happen.
> >  --> If I added phy_power_{on,off}() calling, the issue happened.
> >   --> So, I'm thinking the APIs are related to the issue.
> 
> Yes.
> 
> >
> > - The generic phy APIs are in drivers/phy/phy-core.c.
> >  --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before 
> > devm_phy_create().
> >   --> The phy-core will call pm_runtime_{get_sync,put}() in 
> > phy_{init,exit,power_{on,off}}.
> >--> So, IIUC, both devices of phy-. and  will be 
> > handled by runtime PM APIs.
> >  --> The runtime PM implementation of phy-core seems good to me. But...?
> 
> 
> I have digested the information that you and Geert provided, thanks!
> 
> So, my conclusions so far is:
> 
> The phy core is using runtime PM reference counting at
> phy_power_on|off(). Although it does that on the phy core device,
> which is a child device of the phy provider device.
> 
> Because phy_power_off() is called during system suspend from phy
> consumer drivers like usb, the phy core device (child) and the phy
> provider device (parent) will never become runtime suspended (because
> the PM core has invoked pm_runtime_get_no_resume() for all device in
> the device prepare phase).
> 
> Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq
> phase for the phy provider device, the call to
> pm_runtime_set_suspended() in there, triggers the earlier error
> message, which is because the child (phy core device) is still runtime
> resumed.

Thank you very much for the conclusions!
It's helpful to me about runtime PM behavior.

> >> Then this seems to point to that the driver may be misbehaving in some
> >> way. I can help to check what is going on.
> >
> > I guess so. But, I don't find yet...
> 
> I think the below patch will help, although I am not sure if that is
> sufficient as a long term fix.

Thank you very much for your help!
Also, I'm not sure how to fix for a long term kernels though...

> Can you please try and see if it solves the problems?

Sure! I tested your patch, and then the following message disappeared!

   Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
children

However, the following message still exists.

   Enabling runtime PM for inactive device (ee08.usb) with active children

So, I guess ohci-platform.c also has similar issue.

JFYI, the ehci-platform.c doesn't have runtime PM handling.
So, I think that error message doesn't output from ehci devices.

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Friday, December 1, 2017 6:22 PM
> 
> + Kishon
> 
> On 30 November 2017 at 13:51, Yoshihiro Shimoda
>  wrote:
> > Hi,
> >
> >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
> >>
> >> On 29 November 2017 at 10:43, Geert Uytterhoeven  
> >> wrote:
> >> > Hi Ulf,
> > 
> >> Okay, so the problem remains no matter which solution for wakeup you
> >> pick in genpd.
> >
> > Yes. Today I could reproduce this issue without usb host driver.
> > - The renesas_usb3 usb peripheral driver has generic phy handling.
> >   (The peripheral driver uses different generic phy driver 
> > (phy-rcar-gen3-usb3.c) though.)
> >  --> If I used the current renesas_usb3 (this means doesn't call 
> > phy_power_{on,off}(),
> >  the issue didn't happen.
> >  --> If I added phy_power_{on,off}() calling, the issue happened.
> >   --> So, I'm thinking the APIs are related to the issue.
> 
> Yes.
> 
> >
> > - The generic phy APIs are in drivers/phy/phy-core.c.
> >  --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before 
> > devm_phy_create().
> >   --> The phy-core will call pm_runtime_{get_sync,put}() in 
> > phy_{init,exit,power_{on,off}}.
> >--> So, IIUC, both devices of phy-. and  will be 
> > handled by runtime PM APIs.
> >  --> The runtime PM implementation of phy-core seems good to me. But...?
> 
> 
> I have digested the information that you and Geert provided, thanks!
> 
> So, my conclusions so far is:
> 
> The phy core is using runtime PM reference counting at
> phy_power_on|off(). Although it does that on the phy core device,
> which is a child device of the phy provider device.
> 
> Because phy_power_off() is called during system suspend from phy
> consumer drivers like usb, the phy core device (child) and the phy
> provider device (parent) will never become runtime suspended (because
> the PM core has invoked pm_runtime_get_no_resume() for all device in
> the device prepare phase).
> 
> Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq
> phase for the phy provider device, the call to
> pm_runtime_set_suspended() in there, triggers the earlier error
> message, which is because the child (phy core device) is still runtime
> resumed.

Thank you very much for the conclusions!
It's helpful to me about runtime PM behavior.

> >> Then this seems to point to that the driver may be misbehaving in some
> >> way. I can help to check what is going on.
> >
> > I guess so. But, I don't find yet...
> 
> I think the below patch will help, although I am not sure if that is
> sufficient as a long term fix.

Thank you very much for your help!
Also, I'm not sure how to fix for a long term kernels though...

> Can you please try and see if it solves the problems?

Sure! I tested your patch, and then the following message disappeared!

   Enabling runtime PM for inactive device (ee080200.usb-phy) with active 
children

However, the following message still exists.

   Enabling runtime PM for inactive device (ee08.usb) with active children

So, I guess ohci-platform.c also has similar issue.

JFYI, the ehci-platform.c doesn't have runtime PM handling.
So, I think that error message doesn't output from ehci devices.

Best regards,
Yoshihiro Shimoda

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

[Ulf Hansson]

+ Kishon

On 30 November 2017 at 13:51, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
>>
>> On 29 November 2017 at 10:43, Geert Uytterhoeven  
>> wrote:
>> > Hi Ulf,
> 
>> Okay, so the problem remains no matter which solution for wakeup you
>> pick in genpd.
>
> Yes. Today I could reproduce this issue without usb host driver.
> - The renesas_usb3 usb peripheral driver has generic phy handling.
>   (The peripheral driver uses different generic phy driver 
> (phy-rcar-gen3-usb3.c) though.)
>  --> If I used the current renesas_usb3 (this means doesn't call 
> phy_power_{on,off}(),
>  the issue didn't happen.
>  --> If I added phy_power_{on,off}() calling, the issue happened.
>   --> So, I'm thinking the APIs are related to the issue.

Yes.

>
> - The generic phy APIs are in drivers/phy/phy-core.c.
>  --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before 
> devm_phy_create().
>   --> The phy-core will call pm_runtime_{get_sync,put}() in 
> phy_{init,exit,power_{on,off}}.
>--> So, IIUC, both devices of phy-. and  will be 
> handled by runtime PM APIs.
>  --> The runtime PM implementation of phy-core seems good to me. But...?


I have digested the information that you and Geert provided, thanks!

So, my conclusions so far is:

The phy core is using runtime PM reference counting at
phy_power_on|off(). Although it does that on the phy core device,
which is a child device of the phy provider device.

Because phy_power_off() is called during system suspend from phy
consumer drivers like usb, the phy core device (child) and the phy
provider device (parent) will never become runtime suspended (because
the PM core has invoked pm_runtime_get_no_resume() for all device in
the device prepare phase).

Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq
phase for the phy provider device, the call to
pm_runtime_set_suspended() in there, triggers the earlier error
message, which is because the child (phy core device) is still runtime
resumed.

>
>> Then this seems to point to that the driver may be misbehaving in some
>> way. I can help to check what is going on.
>
> I guess so. But, I don't find yet...

I think the below patch will help, although I am not sure if that is
sufficient as a long term fix.

Can you please try and see if it solves the problems?

From: Ulf Hansson 
Date: Fri, 1 Dec 2017 09:07:54 +0100
Subject: [PATCH] phy: core: Move runtime PM reference counting to be done on
 the parent

This is temporary hack, do not merge!

The runtime PM deployment in the phy core is a bit unnecessary complicated,
due to enabling runtime PM for the phy device. Moreover it causes problems
for parent devices (phy providers) when dealing with system wide PM.

Therefore, move the runtime PM reference counting to be done on the phy's
parent device and drop to enable runtime PM for the phy device altogether.
This simplifies for the phy provider driver, to deal with system wide PM,
in case it also cares about keeping the runtime PM status of the device in
sync with the state of the HW.

Signed-off-by: Ulf Hansson 
---
 drivers/phy/phy-core.c | 35 +--
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b4964b0..837e50d 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -217,15 +217,12 @@ EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);

 int phy_init(struct phy *phy)
 {
-   int ret;
+   int ret = 0;

if (!phy)
return 0;

-   ret = phy_pm_runtime_get_sync(phy);
-   if (ret < 0 && ret != -ENOTSUPP)
-   return ret;
-   ret = 0; /* Override possible ret == -ENOTSUPP */
+   pm_runtime_get_sync(phy->dev.parent);

mutex_lock(>mutex);
if (phy->init_count == 0 && phy->ops->init) {
@@ -239,22 +236,19 @@ int phy_init(struct phy *phy)

 out:
mutex_unlock(>mutex);
-   phy_pm_runtime_put(phy);
+   pm_runtime_put(phy->dev.parent);
return ret;
 }
 EXPORT_SYMBOL_GPL(phy_init);

 int phy_exit(struct phy *phy)
 {
-   int ret;
+   int ret = 0;

if (!phy)
return 0;

-   ret = phy_pm_runtime_get_sync(phy);
-   if (ret < 0 && ret != -ENOTSUPP)
-   return ret;
-   ret = 0; /* Override possible ret == -ENOTSUPP */
+   pm_runtime_get_sync(phy->dev.parent);

mutex_lock(>mutex);
if (phy->init_count == 1 && phy->ops->exit) {
@@ -268,7 +262,7 @@ int phy_exit(struct phy *phy)

 out:
mutex_unlock(>mutex);
-   phy_pm_runtime_put(phy);
+   pm_runtime_put(phy->dev.parent);
return ret;
 }
 EXPORT_SYMBOL_GPL(phy_exit);
@@ -286,11 +280,7 @@ int phy_power_on(struct phy *phy)
goto out;
}

-   ret = phy_pm_runtime_get_sync(phy);
-  

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

[Ulf Hansson]

+ Kishon

On 30 November 2017 at 13:51, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
>>
>> On 29 November 2017 at 10:43, Geert Uytterhoeven  
>> wrote:
>> > Hi Ulf,
> 
>> Okay, so the problem remains no matter which solution for wakeup you
>> pick in genpd.
>
> Yes. Today I could reproduce this issue without usb host driver.
> - The renesas_usb3 usb peripheral driver has generic phy handling.
>   (The peripheral driver uses different generic phy driver 
> (phy-rcar-gen3-usb3.c) though.)
>  --> If I used the current renesas_usb3 (this means doesn't call 
> phy_power_{on,off}(),
>  the issue didn't happen.
>  --> If I added phy_power_{on,off}() calling, the issue happened.
>   --> So, I'm thinking the APIs are related to the issue.

Yes.

>
> - The generic phy APIs are in drivers/phy/phy-core.c.
>  --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before 
> devm_phy_create().
>   --> The phy-core will call pm_runtime_{get_sync,put}() in 
> phy_{init,exit,power_{on,off}}.
>--> So, IIUC, both devices of phy-. and  will be 
> handled by runtime PM APIs.
>  --> The runtime PM implementation of phy-core seems good to me. But...?


I have digested the information that you and Geert provided, thanks!

So, my conclusions so far is:

The phy core is using runtime PM reference counting at
phy_power_on|off(). Although it does that on the phy core device,
which is a child device of the phy provider device.

Because phy_power_off() is called during system suspend from phy
consumer drivers like usb, the phy core device (child) and the phy
provider device (parent) will never become runtime suspended (because
the PM core has invoked pm_runtime_get_no_resume() for all device in
the device prepare phase).

Then, when genpd calls pm_runtime_force_suspend() at the suspend noirq
phase for the phy provider device, the call to
pm_runtime_set_suspended() in there, triggers the earlier error
message, which is because the child (phy core device) is still runtime
resumed.

>
>> Then this seems to point to that the driver may be misbehaving in some
>> way. I can help to check what is going on.
>
> I guess so. But, I don't find yet...

I think the below patch will help, although I am not sure if that is
sufficient as a long term fix.

Can you please try and see if it solves the problems?

From: Ulf Hansson 
Date: Fri, 1 Dec 2017 09:07:54 +0100
Subject: [PATCH] phy: core: Move runtime PM reference counting to be done on
 the parent

This is temporary hack, do not merge!

The runtime PM deployment in the phy core is a bit unnecessary complicated,
due to enabling runtime PM for the phy device. Moreover it causes problems
for parent devices (phy providers) when dealing with system wide PM.

Therefore, move the runtime PM reference counting to be done on the phy's
parent device and drop to enable runtime PM for the phy device altogether.
This simplifies for the phy provider driver, to deal with system wide PM,
in case it also cares about keeping the runtime PM status of the device in
sync with the state of the HW.

Signed-off-by: Ulf Hansson 
---
 drivers/phy/phy-core.c | 35 +--
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b4964b0..837e50d 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -217,15 +217,12 @@ EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);

 int phy_init(struct phy *phy)
 {
-   int ret;
+   int ret = 0;

if (!phy)
return 0;

-   ret = phy_pm_runtime_get_sync(phy);
-   if (ret < 0 && ret != -ENOTSUPP)
-   return ret;
-   ret = 0; /* Override possible ret == -ENOTSUPP */
+   pm_runtime_get_sync(phy->dev.parent);

mutex_lock(>mutex);
if (phy->init_count == 0 && phy->ops->init) {
@@ -239,22 +236,19 @@ int phy_init(struct phy *phy)

 out:
mutex_unlock(>mutex);
-   phy_pm_runtime_put(phy);
+   pm_runtime_put(phy->dev.parent);
return ret;
 }
 EXPORT_SYMBOL_GPL(phy_init);

 int phy_exit(struct phy *phy)
 {
-   int ret;
+   int ret = 0;

if (!phy)
return 0;

-   ret = phy_pm_runtime_get_sync(phy);
-   if (ret < 0 && ret != -ENOTSUPP)
-   return ret;
-   ret = 0; /* Override possible ret == -ENOTSUPP */
+   pm_runtime_get_sync(phy->dev.parent);

mutex_lock(>mutex);
if (phy->init_count == 1 && phy->ops->exit) {
@@ -268,7 +262,7 @@ int phy_exit(struct phy *phy)

 out:
mutex_unlock(>mutex);
-   phy_pm_runtime_put(phy);
+   pm_runtime_put(phy->dev.parent);
return ret;
 }
 EXPORT_SYMBOL_GPL(phy_exit);
@@ -286,11 +280,7 @@ int phy_power_on(struct phy *phy)
goto out;
}

-   ret = phy_pm_runtime_get_sync(phy);
-   if (ret < 0 && ret != -ENOTSUPP)
-   goto err_pm_sync;
-
-   ret = 0; /* Override 

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
> 
> On 29 November 2017 at 10:43, Geert Uytterhoeven  wrote:
> > Hi Ulf,

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

Yes. Today I could reproduce this issue without usb host driver.
- The renesas_usb3 usb peripheral driver has generic phy handling.
  (The peripheral driver uses different generic phy driver 
(phy-rcar-gen3-usb3.c) though.)
 --> If I used the current renesas_usb3 (this means doesn't call 
phy_power_{on,off}(),
 the issue didn't happen.
 --> If I added phy_power_{on,off}() calling, the issue happened.
  --> So, I'm thinking the APIs are related to the issue.

- The generic phy APIs are in drivers/phy/phy-core.c.
 --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before 
devm_phy_create().
  --> The phy-core will call pm_runtime_{get_sync,put}() in 
phy_{init,exit,power_{on,off}}.
   --> So, IIUC, both devices of phy-. and  will be 
handled by runtime PM APIs.
 --> The runtime PM implementation of phy-core seems good to me. But...?

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

I guess so. But, I don't find yet...

Best regards,
Yoshihiro Shimoda

> Kind regards
> Uffe

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM
> 
> On 29 November 2017 at 10:43, Geert Uytterhoeven  wrote:
> > Hi Ulf,

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

Yes. Today I could reproduce this issue without usb host driver.
- The renesas_usb3 usb peripheral driver has generic phy handling.
  (The peripheral driver uses different generic phy driver 
(phy-rcar-gen3-usb3.c) though.)
 --> If I used the current renesas_usb3 (this means doesn't call 
phy_power_{on,off}(),
 the issue didn't happen.
 --> If I added phy_power_{on,off}() calling, the issue happened.
  --> So, I'm thinking the APIs are related to the issue.

- The generic phy APIs are in drivers/phy/phy-core.c.
 --> The phy-rcar-gen3-usb[23] drivers call only pm_runtime_enable() before 
devm_phy_create().
  --> The phy-core will call pm_runtime_{get_sync,put}() in 
phy_{init,exit,power_{on,off}}.
   --> So, IIUC, both devices of phy-. and  will be 
handled by runtime PM APIs.
 --> The runtime PM implementation of phy-core seems good to me. But...?

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

I guess so. But, I don't find yet...

Best regards,
Yoshihiro Shimoda

> Kind regards
> Uffe

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

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Nov 29, 2017 at 10:59 AM, Ulf Hansson  wrote:
> On 29 November 2017 at 10:43, Geert Uytterhoeven  wrote:
>> On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson  wrote:
>>> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>>>  wrote:
> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>  wrote:
> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki 
> >>  wrote:
> >> > From: Rafael J. Wysocki 
 
> >> JFTR, this triggered before during system resume on e.g. Salvator-XS 
> >> with
> >> R-Car H3:
> >>
> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> >> but active child
> >> ohci-platform ee0a.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?

/sys/devices/platform/soc/ee08.usb
/sys/devices/platform/soc/ee0c.usb
/sys/devices/platform/soc/ee0a.usb

Driver: ohci-platform

The children are usb6/6-0:1.0, usb3/3-0:1.0, resp. usb4/4-0:1.0, all using
the usb "hub" driver

/sys/devices/platform/soc/ee080200.usb-phy
/sys/devices/platform/soc/ee0a0200.usb-phy
/sys/devices/platform/soc/ee0c0200.usb-phy

Driver: phy_rcar_gen3_usb2

The children are:

phy/phy-ee080200.usb-phy.2
phy/phy-ee0a0200.usb-phy.0
phy/phy-ee0c0200.usb-phy.1

all without a driver, according to sysfs.

Note that at first I had missed them, as printing the children using
device_for_each_child() does not print them, unlike the hub devices that
are children of the usb hosts.

With some debug code added, logging inc/dec of child_count:

USB driver init:

 ehci-pci: EHCI PCI platform driver
 ehci-platform: EHCI generic platform driver
+phy_rcar_gen3_usb2 ee0a0200.usb-phy: rpm_resume:830: inc child_count
of parent soc
+phy phy-ee0a0200.usb-phy.0: rpm_resume:830: inc child_count of parent
ee0a0200.usb-phy
+phy phy-ee0a0200.usb-phy.0: rpm_suspend:606: dec child_count of
parent ee0a0200.usb-phy
+phy phy-ee0a0200.usb-phy.0: rpm_resume:759: inc child_count of parent
ee0a0200.usb-phy

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

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Nov 29, 2017 at 10:59 AM, Ulf Hansson  wrote:
> On 29 November 2017 at 10:43, Geert Uytterhoeven  wrote:
>> On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson  wrote:
>>> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>>>  wrote:
> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>  wrote:
> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki 
> >>  wrote:
> >> > From: Rafael J. Wysocki 
 
> >> JFTR, this triggered before during system resume on e.g. Salvator-XS 
> >> with
> >> R-Car H3:
> >>
> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> >> but active child
> >> ohci-platform ee0a.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?

/sys/devices/platform/soc/ee08.usb
/sys/devices/platform/soc/ee0c.usb
/sys/devices/platform/soc/ee0a.usb

Driver: ohci-platform

The children are usb6/6-0:1.0, usb3/3-0:1.0, resp. usb4/4-0:1.0, all using
the usb "hub" driver

/sys/devices/platform/soc/ee080200.usb-phy
/sys/devices/platform/soc/ee0a0200.usb-phy
/sys/devices/platform/soc/ee0c0200.usb-phy

Driver: phy_rcar_gen3_usb2

The children are:

phy/phy-ee080200.usb-phy.2
phy/phy-ee0a0200.usb-phy.0
phy/phy-ee0c0200.usb-phy.1

all without a driver, according to sysfs.

Note that at first I had missed them, as printing the children using
device_for_each_child() does not print them, unlike the hub devices that
are children of the usb hosts.

With some debug code added, logging inc/dec of child_count:

USB driver init:

 ehci-pci: EHCI PCI platform driver
 ehci-platform: EHCI generic platform driver
+phy_rcar_gen3_usb2 ee0a0200.usb-phy: rpm_resume:830: inc child_count
of parent soc
+phy phy-ee0a0200.usb-phy.0: rpm_resume:830: inc child_count of parent
ee0a0200.usb-phy
+phy phy-ee0a0200.usb-phy.0: rpm_suspend:606: dec child_count of
parent ee0a0200.usb-phy
+phy phy-ee0a0200.usb-phy.0: rpm_resume:759: inc child_count of parent
ee0a0200.usb-phy

+phy_rcar_gen3_usb2 ee0c0200.usb-phy: rpm_resume:830: inc child_count
of parent soc
+phy phy-ee0c0200.usb-phy.1: rpm_resume:830: inc child_count of parent
ee0c0200.usb-phy
+phy phy-ee0c0200.usb-phy.1: 

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

[Ulf Hansson]

On 29 November 2017 at 10:43, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson  wrote:
>> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>>  wrote:
 From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
 On 28 November 2017 at 13:48, Yoshihiro Shimoda
  wrote:
 >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
 >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
 >> wrote:
 >> > From: Rafael J. Wysocki 
>>> 
 >> JFTR, this triggered before during system resume on e.g. Salvator-XS 
 >> with
 >> R-Car H3:
 >>
 >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
 >> but active child
 >> ohci-platform ee0a.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

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

[Ulf Hansson]

On 29 November 2017 at 10:43, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson  wrote:
>> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>>  wrote:
 From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
 On 28 November 2017 at 13:48, Yoshihiro Shimoda
  wrote:
 >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
 >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
 >> wrote:
 >> > From: Rafael J. Wysocki 
>>> 
 >> JFTR, this triggered before during system resume on e.g. Salvator-XS 
 >> with
 >> R-Car H3:
 >>
 >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
 >> but active child
 >> ohci-platform ee0a.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

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

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson  wrote:
> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>  wrote:
>>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
>>> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>>>  wrote:
>>> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>>> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
>>> >> wrote:
>>> >> > From: Rafael J. Wysocki 
>> 
>>> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>>> >> R-Car H3:
>>> >>
>>> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
>>> >> but active child
>>> >> ohci-platform ee0a.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 -- ge...@linux-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

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

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson  wrote:
> On 29 November 2017 at 09:21, Yoshihiro Shimoda
>  wrote:
>>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
>>> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>>>  wrote:
>>> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>>> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
>>> >> wrote:
>>> >> > From: Rafael J. Wysocki 
>> 
>>> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>>> >> R-Car H3:
>>> >>
>>> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
>>> >> but active child
>>> >> ohci-platform ee0a.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 -- ge...@linux-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

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

[Ulf Hansson]

On 29 November 2017 at 09:21, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
>>
>> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>>  wrote:
>> > Hi Geert-san,
>> >
>> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>> >>
>> >> Hi Rafael, Shimoda-san,
>> >>
>> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
>> >> wrote:
>> >> > From: Rafael J. Wysocki 
> 
>> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>> >> R-Car H3:
>> >>
>> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
>> >> but active child
>> >> ohci-platform ee0a.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?

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.

>
>> > The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
>> > --> If I only used the renesas_usbhs driver (in other words, I don't 
>> > install
>> > [eo]hci-{hcd,platform} drivers), the issue disappeared.
>> >  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
>> > (But, it is possible to be related though.)
>> >
>> > I'll continue to investigate this issue tomorrow.
>>
>> Please keep me posted, I am interested about the why the problem exists. :-)
>
> Sure! :)

Great, thanks.

Kind regards
Uffe

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

[Ulf Hansson]

On 29 November 2017 at 09:21, Yoshihiro Shimoda
 wrote:
> Hi,
>
>> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
>>
>> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>>  wrote:
>> > Hi Geert-san,
>> >
>> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>> >>
>> >> Hi Rafael, Shimoda-san,
>> >>
>> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
>> >> wrote:
>> >> > From: Rafael J. Wysocki 
> 
>> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>> >> R-Car H3:
>> >>
>> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
>> >> but active child
>> >> ohci-platform ee0a.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?

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.

>
>> > The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
>> > --> If I only used the renesas_usbhs driver (in other words, I don't 
>> > install
>> > [eo]hci-{hcd,platform} drivers), the issue disappeared.
>> >  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
>> > (But, it is possible to be related though.)
>> >
>> > I'll continue to investigate this issue tomorrow.
>>
>> Please keep me posted, I am interested about the why the problem exists. :-)
>
> Sure! :)

Great, thanks.

Kind regards
Uffe

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

[Yoshihiro Shimoda]

Hi,

> From: Alan Stern, Sent: Wednesday, November 29, 2017 12:07 AM
> 
> On Tue, 28 Nov 2017, Yoshihiro Shimoda wrote:
> 
> > Hi Geert-san,
> >
> > > From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> > >
> > > Hi Rafael, Shimoda-san,
> > >
> > > On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
> > > wrote:
> > > > From: Rafael J. Wysocki 

> > > JFTR, this triggered before during system resume on e.g. Salvator-XS with
> > > R-Car H3:
> > >
> > > ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> > > but active child
> > > ohci-platform ee0a.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.
> >
> > The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.

I'm so sorry, but this is mistake.
The renesas_usbhs doesn't use the phy_rcar_gen3_usb2 driver.
So,

> > --> If I only used the renesas_usbhs driver (in other words, I don't install
> > [eo]hci-{hcd,platform} drivers), the issue disappeared.
> >  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> > (But, it is possible to be related though.)

They are also mistake.

> > I'll continue to investigate this issue tomorrow.
> 
> Does the phy_rcar_gen3_usb2 driver use runtime PM?

Yes, the phy_rcar_gen3_usb2 uses runtime PM.

>  It looks like the
> phy device somehow gets enabled for runtime PM when it shouldn't be.

I also think that now.
I don't find why for now, but the usage_count of a phy device was not 1 just 
before suspend.
(This "a phy device" means the child of ee0a0200.usb-phy device.)

> (And by the way, what device is the child of ee0a0200.usb-phy?)

It's a phy device:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/phy-core.c?h=v4.15-rc1#n773

Best regards,
Yoshihiro Shimoda

> Alan Stern

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

[Yoshihiro Shimoda]

Hi,

> From: Alan Stern, Sent: Wednesday, November 29, 2017 12:07 AM
> 
> On Tue, 28 Nov 2017, Yoshihiro Shimoda wrote:
> 
> > Hi Geert-san,
> >
> > > From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> > >
> > > Hi Rafael, Shimoda-san,
> > >
> > > On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
> > > wrote:
> > > > From: Rafael J. Wysocki 

> > > JFTR, this triggered before during system resume on e.g. Salvator-XS with
> > > R-Car H3:
> > >
> > > ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> > > but active child
> > > ohci-platform ee0a.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.
> >
> > The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.

I'm so sorry, but this is mistake.
The renesas_usbhs doesn't use the phy_rcar_gen3_usb2 driver.
So,

> > --> If I only used the renesas_usbhs driver (in other words, I don't install
> > [eo]hci-{hcd,platform} drivers), the issue disappeared.
> >  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> > (But, it is possible to be related though.)

They are also mistake.

> > I'll continue to investigate this issue tomorrow.
> 
> Does the phy_rcar_gen3_usb2 driver use runtime PM?

Yes, the phy_rcar_gen3_usb2 uses runtime PM.

>  It looks like the
> phy device somehow gets enabled for runtime PM when it shouldn't be.

I also think that now.
I don't find why for now, but the usage_count of a phy device was not 1 just 
before suspend.
(This "a phy device" means the child of ee0a0200.usb-phy device.)

> (And by the way, what device is the child of ee0a0200.usb-phy?)

It's a phy device:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/phy-core.c?h=v4.15-rc1#n773

Best regards,
Yoshihiro Shimoda

> Alan Stern

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
> 
> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>  wrote:
> > Hi Geert-san,
> >
> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> >>
> >> Hi Rafael, Shimoda-san,
> >>
> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
> >> wrote:
> >> > From: Rafael J. Wysocki 

> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> >> R-Car H3:
> >>
> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> >> but active child
> >> ohci-platform ee0a.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.

> > The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
> > --> If I only used the renesas_usbhs driver (in other words, I don't install
> > [eo]hci-{hcd,platform} drivers), the issue disappeared.
> >  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> > (But, it is possible to be related though.)
> >
> > I'll continue to investigate this issue tomorrow.
> 
> Please keep me posted, I am interested about the why the problem exists. :-)

Sure! :)

Best regards,
Yoshihiro Shimoda

> Kind regards
> Uffe

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

[Yoshihiro Shimoda]

Hi,

> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 2:23 AM
> 
> On 28 November 2017 at 13:48, Yoshihiro Shimoda
>  wrote:
> > Hi Geert-san,
> >
> >> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> >>
> >> Hi Rafael, Shimoda-san,
> >>
> >> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
> >> wrote:
> >> > From: Rafael J. Wysocki 

> >> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> >> R-Car H3:
> >>
> >> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> >> but active child
> >> ohci-platform ee0a.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.

> > The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
> > --> If I only used the renesas_usbhs driver (in other words, I don't install
> > [eo]hci-{hcd,platform} drivers), the issue disappeared.
> >  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> > (But, it is possible to be related though.)
> >
> > I'll continue to investigate this issue tomorrow.
> 
> Please keep me posted, I am interested about the why the problem exists. :-)

Sure! :)

Best regards,
Yoshihiro Shimoda

> Kind regards
> Uffe

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

[Ulf Hansson]

On 28 November 2017 at 13:48, Yoshihiro Shimoda
 wrote:
> Hi Geert-san,
>
>> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>>
>> Hi Rafael, Shimoda-san,
>>
>> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
>> wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > The check for "active" children in __pm_runtime_set_status(), when
>> > trying to set the parent device status to "suspended", doesn't
>> > really make sense, because in fact it is not invalid to set the
>> > status of a device with runtime PM disabled to "suspended" in any
>> > case.  It is invalid to enable runtime PM for a device with its
>> > status set to "suspended" while its child_count reference counter
>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>> > really cover that situation.
>> >
>> > For this reason, drop the children check from __pm_runtime_set_status()
>> > and add a check against child_count reference counters of "suspended"
>> > devices to pm_runtime_enable().
>> >
>> > Signed-off-by: Rafael J. Wysocki 
>> > ---
>> >  drivers/base/power/runtime.c |   30 ++
>> >  1 file changed, 10 insertions(+), 20 deletions(-)
>> >
>> > Index: linux-pm/drivers/base/power/runtime.c
>> > ===
>> > --- linux-pm.orig/drivers/base/power/runtime.c
>> > +++ linux-pm/drivers/base/power/runtime.c
>> > @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
>> > goto out;
>> > }
>> >
>> > -   if (dev->power.runtime_status == status)
>> > +   if (dev->power.runtime_status == status || !parent)
>> > goto out_set;
>> >
>> > if (status == RPM_SUSPENDED) {
>> > -   /*
>> > -* It is invalid to suspend a device with an active child,
>> > -* unless it has been set to ignore its children.
>> > -*/
>> > -   if (!dev->power.ignore_children &&
>> > -   atomic_read(>power.child_count)) {
>> > -   dev_err(dev, "runtime PM trying to suspend device 
>> > but active child\n");
>>
>> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>> R-Car H3:
>>
>> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
>> but active child
>> ohci-platform ee0a.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!

>
> The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
> --> If I only used the renesas_usbhs driver (in other words, I don't install
> [eo]hci-{hcd,platform} drivers), the issue disappeared.
>  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> (But, it is possible to be related though.)
>
> I'll continue to investigate this issue tomorrow.

Please keep me posted, I am interested about the why the problem exists. :-)

Kind regards
Uffe

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

[Ulf Hansson]

On 28 November 2017 at 13:48, Yoshihiro Shimoda
 wrote:
> Hi Geert-san,
>
>> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
>>
>> Hi Rafael, Shimoda-san,
>>
>> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
>> wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > The check for "active" children in __pm_runtime_set_status(), when
>> > trying to set the parent device status to "suspended", doesn't
>> > really make sense, because in fact it is not invalid to set the
>> > status of a device with runtime PM disabled to "suspended" in any
>> > case.  It is invalid to enable runtime PM for a device with its
>> > status set to "suspended" while its child_count reference counter
>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>> > really cover that situation.
>> >
>> > For this reason, drop the children check from __pm_runtime_set_status()
>> > and add a check against child_count reference counters of "suspended"
>> > devices to pm_runtime_enable().
>> >
>> > Signed-off-by: Rafael J. Wysocki 
>> > ---
>> >  drivers/base/power/runtime.c |   30 ++
>> >  1 file changed, 10 insertions(+), 20 deletions(-)
>> >
>> > Index: linux-pm/drivers/base/power/runtime.c
>> > ===
>> > --- linux-pm.orig/drivers/base/power/runtime.c
>> > +++ linux-pm/drivers/base/power/runtime.c
>> > @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
>> > goto out;
>> > }
>> >
>> > -   if (dev->power.runtime_status == status)
>> > +   if (dev->power.runtime_status == status || !parent)
>> > goto out_set;
>> >
>> > if (status == RPM_SUSPENDED) {
>> > -   /*
>> > -* It is invalid to suspend a device with an active child,
>> > -* unless it has been set to ignore its children.
>> > -*/
>> > -   if (!dev->power.ignore_children &&
>> > -   atomic_read(>power.child_count)) {
>> > -   dev_err(dev, "runtime PM trying to suspend device 
>> > but active child\n");
>>
>> JFTR, this triggered before during system resume on e.g. Salvator-XS with
>> R-Car H3:
>>
>> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
>> but active child
>> ohci-platform ee0a.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!

>
> The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
> --> If I only used the renesas_usbhs driver (in other words, I don't install
> [eo]hci-{hcd,platform} drivers), the issue disappeared.
>  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> (But, it is possible to be related though.)
>
> I'll continue to investigate this issue tomorrow.

Please keep me posted, I am interested about the why the problem exists. :-)

Kind regards
Uffe

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

[Alan Stern]

On Tue, 28 Nov 2017, Yoshihiro Shimoda wrote:

> Hi Geert-san,
> 
> > From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> > 
> > Hi Rafael, Shimoda-san,
> > 
> > On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
> > wrote:
> > > From: Rafael J. Wysocki 
> > >
> > > The check for "active" children in __pm_runtime_set_status(), when
> > > trying to set the parent device status to "suspended", doesn't
> > > really make sense, because in fact it is not invalid to set the
> > > status of a device with runtime PM disabled to "suspended" in any
> > > case.  It is invalid to enable runtime PM for a device with its
> > > status set to "suspended" while its child_count reference counter
> > > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > > really cover that situation.
> > >
> > > For this reason, drop the children check from __pm_runtime_set_status()
> > > and add a check against child_count reference counters of "suspended"
> > > devices to pm_runtime_enable().
> > >
> > > Signed-off-by: Rafael J. Wysocki 
> > > ---
> > >  drivers/base/power/runtime.c |   30 ++
> > >  1 file changed, 10 insertions(+), 20 deletions(-)
> > >
> > > Index: linux-pm/drivers/base/power/runtime.c
> > > ===
> > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > +++ linux-pm/drivers/base/power/runtime.c
> > > @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> > > goto out;
> > > }
> > >
> > > -   if (dev->power.runtime_status == status)
> > > +   if (dev->power.runtime_status == status || !parent)
> > > goto out_set;
> > >
> > > if (status == RPM_SUSPENDED) {
> > > -   /*
> > > -* It is invalid to suspend a device with an active child,
> > > -* unless it has been set to ignore its children.
> > > -*/
> > > -   if (!dev->power.ignore_children &&
> > > -   atomic_read(>power.child_count)) {
> > > -   dev_err(dev, "runtime PM trying to suspend device 
> > > but active child\n");
> > 
> > JFTR, this triggered before during system resume on e.g. Salvator-XS with
> > R-Car H3:
> > 
> > ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> > but active child
> > ohci-platform ee0a.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.
> 
> The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
> --> If I only used the renesas_usbhs driver (in other words, I don't install
> [eo]hci-{hcd,platform} drivers), the issue disappeared.
>  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> (But, it is possible to be related though.)
> 
> I'll continue to investigate this issue tomorrow.

Does the phy_rcar_gen3_usb2 driver use runtime PM?  It looks like the 
phy device somehow gets enabled for runtime PM when it shouldn't be.

(And by the way, what device is the child of ee0a0200.usb-phy?)

Alan Stern

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

[Alan Stern]

On Tue, 28 Nov 2017, Yoshihiro Shimoda wrote:

> Hi Geert-san,
> 
> > From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> > 
> > Hi Rafael, Shimoda-san,
> > 
> > On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  
> > wrote:
> > > From: Rafael J. Wysocki 
> > >
> > > The check for "active" children in __pm_runtime_set_status(), when
> > > trying to set the parent device status to "suspended", doesn't
> > > really make sense, because in fact it is not invalid to set the
> > > status of a device with runtime PM disabled to "suspended" in any
> > > case.  It is invalid to enable runtime PM for a device with its
> > > status set to "suspended" while its child_count reference counter
> > > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > > really cover that situation.
> > >
> > > For this reason, drop the children check from __pm_runtime_set_status()
> > > and add a check against child_count reference counters of "suspended"
> > > devices to pm_runtime_enable().
> > >
> > > Signed-off-by: Rafael J. Wysocki 
> > > ---
> > >  drivers/base/power/runtime.c |   30 ++
> > >  1 file changed, 10 insertions(+), 20 deletions(-)
> > >
> > > Index: linux-pm/drivers/base/power/runtime.c
> > > ===
> > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > +++ linux-pm/drivers/base/power/runtime.c
> > > @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> > > goto out;
> > > }
> > >
> > > -   if (dev->power.runtime_status == status)
> > > +   if (dev->power.runtime_status == status || !parent)
> > > goto out_set;
> > >
> > > if (status == RPM_SUSPENDED) {
> > > -   /*
> > > -* It is invalid to suspend a device with an active child,
> > > -* unless it has been set to ignore its children.
> > > -*/
> > > -   if (!dev->power.ignore_children &&
> > > -   atomic_read(>power.child_count)) {
> > > -   dev_err(dev, "runtime PM trying to suspend device 
> > > but active child\n");
> > 
> > JFTR, this triggered before during system resume on e.g. Salvator-XS with
> > R-Car H3:
> > 
> > ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> > but active child
> > ohci-platform ee0a.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.
> 
> The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
> --> If I only used the renesas_usbhs driver (in other words, I don't install
> [eo]hci-{hcd,platform} drivers), the issue disappeared.
>  --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
> (But, it is possible to be related though.)
> 
> I'll continue to investigate this issue tomorrow.

Does the phy_rcar_gen3_usb2 driver use runtime PM?  It looks like the 
phy device somehow gets enabled for runtime PM when it shouldn't be.

(And by the way, what device is the child of ee0a0200.usb-phy?)

Alan Stern

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

[Rafael J. Wysocki]

On Tue, Nov 28, 2017 at 11:58 AM, Geert Uytterhoeven
 wrote:
> Hi Rafael, Shimoda-san,
>
> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  wrote:
>> From: Rafael J. Wysocki 
>>
>> The check for "active" children in __pm_runtime_set_status(), when
>> trying to set the parent device status to "suspended", doesn't
>> really make sense, because in fact it is not invalid to set the
>> status of a device with runtime PM disabled to "suspended" in any
>> case.  It is invalid to enable runtime PM for a device with its
>> status set to "suspended" while its child_count reference counter
>> is nonzero, but the check in __pm_runtime_set_status() doesn't
>> really cover that situation.
>>
>> For this reason, drop the children check from __pm_runtime_set_status()
>> and add a check against child_count reference counters of "suspended"
>> devices to pm_runtime_enable().
>>
>> Signed-off-by: Rafael J. Wysocki 
>> ---
>>  drivers/base/power/runtime.c |   30 ++
>>  1 file changed, 10 insertions(+), 20 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/runtime.c
>> ===
>> --- linux-pm.orig/drivers/base/power/runtime.c
>> +++ linux-pm/drivers/base/power/runtime.c
>> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
>> goto out;
>> }
>>
>> -   if (dev->power.runtime_status == status)
>> +   if (dev->power.runtime_status == status || !parent)
>> goto out_set;
>>
>> if (status == RPM_SUSPENDED) {
>> -   /*
>> -* It is invalid to suspend a device with an active child,
>> -* unless it has been set to ignore its children.
>> -*/
>> -   if (!dev->power.ignore_children &&
>> -   atomic_read(>power.child_count)) {
>> -   dev_err(dev, "runtime PM trying to suspend device 
>> but active child\n");
>
> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> R-Car H3:
>
> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> but active child
> ohci-platform ee0a.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.
>
>> -   error = -EBUSY;
>> -   goto out;
>> -   }
>> -
>> -   if (parent) {
>> -   atomic_add_unless(>power.child_count, -1, 0);
>> -   notify_parent = !parent->power.ignore_children;
>> -   }
>> -   goto out_set;
>> -   }
>> -
>> -   if (parent) {
>> +   atomic_add_unless(>power.child_count, -1, 0);
>> +   notify_parent = !parent->power.ignore_children;
>> +   } else {
>> spin_lock_nested(>power.lock, SINGLE_DEPTH_NESTING);
>>
>> /*
>> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
>> else
>> dev_warn(dev, "Unbalanced %s!\n", __func__);
>>
>> +   WARN(dev->power.runtime_status == RPM_SUSPENDED &&
>> +!dev->power.ignore_children &&
>> +atomic_read(>power.child_count) > 0,
>> +"Enabling runtime PM for inactive device (%s) with active 
>> children\n",
>> +dev_name(dev));
>
> And now it became a bit more noisy:

Well, it's all existing issues, although the WARN() doesn't provide
additional information in this particular case.

I'm considering changing it to print a message without a stack trace.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Tue, Nov 28, 2017 at 11:58 AM, Geert Uytterhoeven
 wrote:
> Hi Rafael, Shimoda-san,
>
> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  wrote:
>> From: Rafael J. Wysocki 
>>
>> The check for "active" children in __pm_runtime_set_status(), when
>> trying to set the parent device status to "suspended", doesn't
>> really make sense, because in fact it is not invalid to set the
>> status of a device with runtime PM disabled to "suspended" in any
>> case.  It is invalid to enable runtime PM for a device with its
>> status set to "suspended" while its child_count reference counter
>> is nonzero, but the check in __pm_runtime_set_status() doesn't
>> really cover that situation.
>>
>> For this reason, drop the children check from __pm_runtime_set_status()
>> and add a check against child_count reference counters of "suspended"
>> devices to pm_runtime_enable().
>>
>> Signed-off-by: Rafael J. Wysocki 
>> ---
>>  drivers/base/power/runtime.c |   30 ++
>>  1 file changed, 10 insertions(+), 20 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/runtime.c
>> ===
>> --- linux-pm.orig/drivers/base/power/runtime.c
>> +++ linux-pm/drivers/base/power/runtime.c
>> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
>> goto out;
>> }
>>
>> -   if (dev->power.runtime_status == status)
>> +   if (dev->power.runtime_status == status || !parent)
>> goto out_set;
>>
>> if (status == RPM_SUSPENDED) {
>> -   /*
>> -* It is invalid to suspend a device with an active child,
>> -* unless it has been set to ignore its children.
>> -*/
>> -   if (!dev->power.ignore_children &&
>> -   atomic_read(>power.child_count)) {
>> -   dev_err(dev, "runtime PM trying to suspend device 
>> but active child\n");
>
> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> R-Car H3:
>
> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> but active child
> ohci-platform ee0a.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.
>
>> -   error = -EBUSY;
>> -   goto out;
>> -   }
>> -
>> -   if (parent) {
>> -   atomic_add_unless(>power.child_count, -1, 0);
>> -   notify_parent = !parent->power.ignore_children;
>> -   }
>> -   goto out_set;
>> -   }
>> -
>> -   if (parent) {
>> +   atomic_add_unless(>power.child_count, -1, 0);
>> +   notify_parent = !parent->power.ignore_children;
>> +   } else {
>> spin_lock_nested(>power.lock, SINGLE_DEPTH_NESTING);
>>
>> /*
>> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
>> else
>> dev_warn(dev, "Unbalanced %s!\n", __func__);
>>
>> +   WARN(dev->power.runtime_status == RPM_SUSPENDED &&
>> +!dev->power.ignore_children &&
>> +atomic_read(>power.child_count) > 0,
>> +"Enabling runtime PM for inactive device (%s) with active 
>> children\n",
>> +dev_name(dev));
>
> And now it became a bit more noisy:

Well, it's all existing issues, although the WARN() doesn't provide
additional information in this particular case.

I'm considering changing it to print a message without a stack trace.

Thanks,
Rafael

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

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> 
> Hi Rafael, Shimoda-san,
> 
> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  wrote:
> > From: Rafael J. Wysocki 
> >
> > The check for "active" children in __pm_runtime_set_status(), when
> > trying to set the parent device status to "suspended", doesn't
> > really make sense, because in fact it is not invalid to set the
> > status of a device with runtime PM disabled to "suspended" in any
> > case.  It is invalid to enable runtime PM for a device with its
> > status set to "suspended" while its child_count reference counter
> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > really cover that situation.
> >
> > For this reason, drop the children check from __pm_runtime_set_status()
> > and add a check against child_count reference counters of "suspended"
> > devices to pm_runtime_enable().
> >
> > Signed-off-by: Rafael J. Wysocki 
> > ---
> >  drivers/base/power/runtime.c |   30 ++
> >  1 file changed, 10 insertions(+), 20 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> > goto out;
> > }
> >
> > -   if (dev->power.runtime_status == status)
> > +   if (dev->power.runtime_status == status || !parent)
> > goto out_set;
> >
> > if (status == RPM_SUSPENDED) {
> > -   /*
> > -* It is invalid to suspend a device with an active child,
> > -* unless it has been set to ignore its children.
> > -*/
> > -   if (!dev->power.ignore_children &&
> > -   atomic_read(>power.child_count)) {
> > -   dev_err(dev, "runtime PM trying to suspend device 
> > but active child\n");
> 
> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> R-Car H3:
> 
> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> but active child
> ohci-platform ee0a.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.

The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
--> If I only used the renesas_usbhs driver (in other words, I don't install
[eo]hci-{hcd,platform} drivers), the issue disappeared.
 --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
(But, it is possible to be related though.)

I'll continue to investigate this issue tomorrow.

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, November 28, 2017 7:58 PM
> 
> Hi Rafael, Shimoda-san,
> 
> On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  wrote:
> > From: Rafael J. Wysocki 
> >
> > The check for "active" children in __pm_runtime_set_status(), when
> > trying to set the parent device status to "suspended", doesn't
> > really make sense, because in fact it is not invalid to set the
> > status of a device with runtime PM disabled to "suspended" in any
> > case.  It is invalid to enable runtime PM for a device with its
> > status set to "suspended" while its child_count reference counter
> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > really cover that situation.
> >
> > For this reason, drop the children check from __pm_runtime_set_status()
> > and add a check against child_count reference counters of "suspended"
> > devices to pm_runtime_enable().
> >
> > Signed-off-by: Rafael J. Wysocki 
> > ---
> >  drivers/base/power/runtime.c |   30 ++
> >  1 file changed, 10 insertions(+), 20 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> > goto out;
> > }
> >
> > -   if (dev->power.runtime_status == status)
> > +   if (dev->power.runtime_status == status || !parent)
> > goto out_set;
> >
> > if (status == RPM_SUSPENDED) {
> > -   /*
> > -* It is invalid to suspend a device with an active child,
> > -* unless it has been set to ignore its children.
> > -*/
> > -   if (!dev->power.ignore_children &&
> > -   atomic_read(>power.child_count)) {
> > -   dev_err(dev, "runtime PM trying to suspend device 
> > but active child\n");
> 
> JFTR, this triggered before during system resume on e.g. Salvator-XS with
> R-Car H3:
> 
> ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
> but active child
> ohci-platform ee0a.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.

The renesas_usbhs also uses the phy_rcar_gen3_usb2 driver.
--> If I only used the renesas_usbhs driver (in other words, I don't install
[eo]hci-{hcd,platform} drivers), the issue disappeared.
 --> So, I think the phy_rcar_gen3_usb2 driver doesn't cause this issue.
(But, it is possible to be related though.)

I'll continue to investigate this issue tomorrow.

Best regards,
Yoshihiro Shimoda

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

[Geert Uytterhoeven]

Hi Rafael, Shimoda-san,

On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> The check for "active" children in __pm_runtime_set_status(), when
> trying to set the parent device status to "suspended", doesn't
> really make sense, because in fact it is not invalid to set the
> status of a device with runtime PM disabled to "suspended" in any
> case.  It is invalid to enable runtime PM for a device with its
> status set to "suspended" while its child_count reference counter
> is nonzero, but the check in __pm_runtime_set_status() doesn't
> really cover that situation.
>
> For this reason, drop the children check from __pm_runtime_set_status()
> and add a check against child_count reference counters of "suspended"
> devices to pm_runtime_enable().
>
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/runtime.c |   30 ++
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> goto out;
> }
>
> -   if (dev->power.runtime_status == status)
> +   if (dev->power.runtime_status == status || !parent)
> goto out_set;
>
> if (status == RPM_SUSPENDED) {
> -   /*
> -* It is invalid to suspend a device with an active child,
> -* unless it has been set to ignore its children.
> -*/
> -   if (!dev->power.ignore_children &&
> -   atomic_read(>power.child_count)) {
> -   dev_err(dev, "runtime PM trying to suspend device but 
> active child\n");

JFTR, this triggered before during system resume on e.g. Salvator-XS with
R-Car H3:

ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
but active child
ohci-platform ee0a.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.

> -   error = -EBUSY;
> -   goto out;
> -   }
> -
> -   if (parent) {
> -   atomic_add_unless(>power.child_count, -1, 0);
> -   notify_parent = !parent->power.ignore_children;
> -   }
> -   goto out_set;
> -   }
> -
> -   if (parent) {
> +   atomic_add_unless(>power.child_count, -1, 0);
> +   notify_parent = !parent->power.ignore_children;
> +   } else {
> spin_lock_nested(>power.lock, SINGLE_DEPTH_NESTING);
>
> /*
> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
> else
> dev_warn(dev, "Unbalanced %s!\n", __func__);
>
> +   WARN(dev->power.runtime_status == RPM_SUSPENDED &&
> +!dev->power.ignore_children &&
> +atomic_read(>power.child_count) > 0,
> +"Enabling runtime PM for inactive device (%s) with active 
> children\n",
> +dev_name(dev));

And now it became a bit more noisy:

Enabling runtime PM for inactive device (ee0a0200.usb-phy) with active children
WARNING: CPU: 0 PID: 1697 at drivers/base/power/runtime.c:1299
pm_runtime_enable+0x94/0xd8
CPU: 0 PID: 1697 Comm: s2idle Not tainted
4.15.0-rc1-arm64-renesas-00381-g40d152b966c941dd #41
Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT)
task: 8006f81bb100 task.stack: 0aa8
pstate: 6085 (nZCv daIf -PAN -UAO)
pc : pm_runtime_enable+0x94/0xd8
lr : pm_runtime_enable+0x94/0xd8
sp : 0aa83b50
x29: 0aa83b50 x28: 8006f81bb100
x27: 08841000 x26: 08b4b640
x25: 08b7f6e0 x24: 097a2f90
x23: 08b7f000 x22: 0010
x21:  x20: 8006fa9ad158
x19: 8006fa9ad010 x18: 013c6577
x17: 0947ea80 x16: 6370
x15: 636e x14: 646c696863206576
x13: 0001 x12: 8006f81bbaa8
x11: 8006f9479230 x10: 8006f97a63e0
x9 :  x8 : 8006f97a6408
x7 : 8006f97a63c0 x6 : 0975de80
x5 :  x4 : 
x3 :  x2 : 08b4bbf0
x1 : 8006f81bb100 x0 : 004f
Call trace:
 pm_runtime_enable+0x94/0xd8
 device_resume_early+0x50/0xec
 dpm_resume_early+0x118/0x204
 

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

[Geert Uytterhoeven]

Hi Rafael, Shimoda-san,

On Sun, Nov 12, 2017 at 1:27 AM, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> The check for "active" children in __pm_runtime_set_status(), when
> trying to set the parent device status to "suspended", doesn't
> really make sense, because in fact it is not invalid to set the
> status of a device with runtime PM disabled to "suspended" in any
> case.  It is invalid to enable runtime PM for a device with its
> status set to "suspended" while its child_count reference counter
> is nonzero, but the check in __pm_runtime_set_status() doesn't
> really cover that situation.
>
> For this reason, drop the children check from __pm_runtime_set_status()
> and add a check against child_count reference counters of "suspended"
> devices to pm_runtime_enable().
>
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/runtime.c |   30 ++
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> goto out;
> }
>
> -   if (dev->power.runtime_status == status)
> +   if (dev->power.runtime_status == status || !parent)
> goto out_set;
>
> if (status == RPM_SUSPENDED) {
> -   /*
> -* It is invalid to suspend a device with an active child,
> -* unless it has been set to ignore its children.
> -*/
> -   if (!dev->power.ignore_children &&
> -   atomic_read(>power.child_count)) {
> -   dev_err(dev, "runtime PM trying to suspend device but 
> active child\n");

JFTR, this triggered before during system resume on e.g. Salvator-XS with
R-Car H3:

ohci-platform ee08.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 ee0c.usb: runtime PM trying to suspend device
but active child
ohci-platform ee0a.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.

> -   error = -EBUSY;
> -   goto out;
> -   }
> -
> -   if (parent) {
> -   atomic_add_unless(>power.child_count, -1, 0);
> -   notify_parent = !parent->power.ignore_children;
> -   }
> -   goto out_set;
> -   }
> -
> -   if (parent) {
> +   atomic_add_unless(>power.child_count, -1, 0);
> +   notify_parent = !parent->power.ignore_children;
> +   } else {
> spin_lock_nested(>power.lock, SINGLE_DEPTH_NESTING);
>
> /*
> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
> else
> dev_warn(dev, "Unbalanced %s!\n", __func__);
>
> +   WARN(dev->power.runtime_status == RPM_SUSPENDED &&
> +!dev->power.ignore_children &&
> +atomic_read(>power.child_count) > 0,
> +"Enabling runtime PM for inactive device (%s) with active 
> children\n",
> +dev_name(dev));

And now it became a bit more noisy:

Enabling runtime PM for inactive device (ee0a0200.usb-phy) with active children
WARNING: CPU: 0 PID: 1697 at drivers/base/power/runtime.c:1299
pm_runtime_enable+0x94/0xd8
CPU: 0 PID: 1697 Comm: s2idle Not tainted
4.15.0-rc1-arm64-renesas-00381-g40d152b966c941dd #41
Hardware name: Renesas Salvator-X 2nd version board based on r8a7795 ES2.0+ (DT)
task: 8006f81bb100 task.stack: 0aa8
pstate: 6085 (nZCv daIf -PAN -UAO)
pc : pm_runtime_enable+0x94/0xd8
lr : pm_runtime_enable+0x94/0xd8
sp : 0aa83b50
x29: 0aa83b50 x28: 8006f81bb100
x27: 08841000 x26: 08b4b640
x25: 08b7f6e0 x24: 097a2f90
x23: 08b7f000 x22: 0010
x21:  x20: 8006fa9ad158
x19: 8006fa9ad010 x18: 013c6577
x17: 0947ea80 x16: 6370
x15: 636e x14: 646c696863206576
x13: 0001 x12: 8006f81bbaa8
x11: 8006f9479230 x10: 8006f97a63e0
x9 :  x8 : 8006f97a6408
x7 : 8006f97a63c0 x6 : 0975de80
x5 :  x4 : 
x3 :  x2 : 08b4bbf0
x1 : 8006f81bb100 x0 : 004f
Call trace:
 pm_runtime_enable+0x94/0xd8
 device_resume_early+0x50/0xec
 dpm_resume_early+0x118/0x204
 suspend_devices_and_enter+0x2a8/0x4b0
 pm_suspend+0x22c/0x27c
 state_store+0x84/0x108
 

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

[Rafael J. Wysocki]

On Thursday, November 16, 2017 10:22:41 AM CET Johan Hovold wrote:
> On Sun, Nov 12, 2017 at 01:27:30AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > 
> > The check for "active" children in __pm_runtime_set_status(), when
> > trying to set the parent device status to "suspended", doesn't
> > really make sense, because in fact it is not invalid to set the
> > status of a device with runtime PM disabled to "suspended" in any
> > case.  It is invalid to enable runtime PM for a device with its
> > status set to "suspended" while its child_count reference counter
> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > really cover that situation.
> > 
> > For this reason, drop the children check from __pm_runtime_set_status()
> > and add a check against child_count reference counters of "suspended"
> > devices to pm_runtime_enable().
> > 
> > Signed-off-by: Rafael J. Wysocki 
> 
> Looks good to me, but you should also fix
> Documentation/power/runtime_pm.txt which was updated to reflect the
> constraint that is now being reverted.

Thanks for pointing that out.

> Reviewed-by: Johan Hovold 

Thanks!

Rafael

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

[Rafael J. Wysocki]

On Thursday, November 16, 2017 10:22:41 AM CET Johan Hovold wrote:
> On Sun, Nov 12, 2017 at 01:27:30AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > 
> > The check for "active" children in __pm_runtime_set_status(), when
> > trying to set the parent device status to "suspended", doesn't
> > really make sense, because in fact it is not invalid to set the
> > status of a device with runtime PM disabled to "suspended" in any
> > case.  It is invalid to enable runtime PM for a device with its
> > status set to "suspended" while its child_count reference counter
> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > really cover that situation.
> > 
> > For this reason, drop the children check from __pm_runtime_set_status()
> > and add a check against child_count reference counters of "suspended"
> > devices to pm_runtime_enable().
> > 
> > Signed-off-by: Rafael J. Wysocki 
> 
> Looks good to me, but you should also fix
> Documentation/power/runtime_pm.txt which was updated to reflect the
> constraint that is now being reverted.

Thanks for pointing that out.

> Reviewed-by: Johan Hovold 

Thanks!

Rafael

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

[Johan Hovold]

On Sun, Nov 12, 2017 at 01:27:30AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The check for "active" children in __pm_runtime_set_status(), when
> trying to set the parent device status to "suspended", doesn't
> really make sense, because in fact it is not invalid to set the
> status of a device with runtime PM disabled to "suspended" in any
> case.  It is invalid to enable runtime PM for a device with its
> status set to "suspended" while its child_count reference counter
> is nonzero, but the check in __pm_runtime_set_status() doesn't
> really cover that situation.
> 
> For this reason, drop the children check from __pm_runtime_set_status()
> and add a check against child_count reference counters of "suspended"
> devices to pm_runtime_enable().
> 
> Signed-off-by: Rafael J. Wysocki 

Looks good to me, but you should also fix
Documentation/power/runtime_pm.txt which was updated to reflect the
constraint that is now being reverted.

Reviewed-by: Johan Hovold 

Thanks,
Johan

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

[Johan Hovold]

On Sun, Nov 12, 2017 at 01:27:30AM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> The check for "active" children in __pm_runtime_set_status(), when
> trying to set the parent device status to "suspended", doesn't
> really make sense, because in fact it is not invalid to set the
> status of a device with runtime PM disabled to "suspended" in any
> case.  It is invalid to enable runtime PM for a device with its
> status set to "suspended" while its child_count reference counter
> is nonzero, but the check in __pm_runtime_set_status() doesn't
> really cover that situation.
> 
> For this reason, drop the children check from __pm_runtime_set_status()
> and add a check against child_count reference counters of "suspended"
> devices to pm_runtime_enable().
> 
> Signed-off-by: Rafael J. Wysocki 

Looks good to me, but you should also fix
Documentation/power/runtime_pm.txt which was updated to reflect the
constraint that is now being reverted.

Reviewed-by: Johan Hovold 

Thanks,
Johan

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

[Ulf Hansson]

[...]

>>
>> When pm_runtime_set_suspended(dev) is called, dev's child device may
>> still be runtime PM enabled and active.
>> I was suggesting to add a check for this scenario, to see if dev's
>> child device is runtime PM is enabled, as and additional constraint
>> before deciding to return an error code.
>
> Well, that's sort of difficult to do, however, because the code would need to
> walk all of the children of the device and the child power lock cannot be
> acquired under the one of the parent, so it would be fragile and ugly.

Yeah, you have a point.

>
>> The idea was to get a consistent behavior, from the
>> pm_runtime_set_active|suspended() APIs point of view, and not from the
>> runtime PM core point of view.
>
> Yes, but the cost is high and the benefit is shallow.
>
> The enable-time WARN() should cover the really broken cases without that
> much complexity.

Fair enough!

Feel free to add:
Reviewed-by: Ulf Hansson 

Kind regards
Uffe

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

[Ulf Hansson]

[...]

>>
>> When pm_runtime_set_suspended(dev) is called, dev's child device may
>> still be runtime PM enabled and active.
>> I was suggesting to add a check for this scenario, to see if dev's
>> child device is runtime PM is enabled, as and additional constraint
>> before deciding to return an error code.
>
> Well, that's sort of difficult to do, however, because the code would need to
> walk all of the children of the device and the child power lock cannot be
> acquired under the one of the parent, so it would be fragile and ugly.

Yeah, you have a point.

>
>> The idea was to get a consistent behavior, from the
>> pm_runtime_set_active|suspended() APIs point of view, and not from the
>> runtime PM core point of view.
>
> Yes, but the cost is high and the benefit is shallow.
>
> The enable-time WARN() should cover the really broken cases without that
> much complexity.

Fair enough!

Feel free to add:
Reviewed-by: Ulf Hansson 

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Tuesday, November 14, 2017 10:56:39 AM CET Ulf Hansson wrote:
> On 14 November 2017 at 10:13, Ulf Hansson  wrote:
> > On 13 November 2017 at 22:50, Rafael J. Wysocki  wrote:
> >> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
> >>> On 12 November 2017 at 01:27, Rafael J. Wysocki  
> >>> wrote:
> >>> > From: Rafael J. Wysocki 
> >>> >
> >>> > The check for "active" children in __pm_runtime_set_status(), when
> >>> > trying to set the parent device status to "suspended", doesn't
> >>> > really make sense, because in fact it is not invalid to set the
> >>> > status of a device with runtime PM disabled to "suspended" in any
> >>> > case.  It is invalid to enable runtime PM for a device with its
> >>> > status set to "suspended" while its child_count reference counter
> >>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> >>> > really cover that situation.
> >>>
> >>> The reason to why I changed this in commit a8636c89648a ("PM /
> >>> Runtime: Don't allow to suspend a device with an active child") was
> >>> because to get a consistent behavior.
> >>
> >> Well, it causes the function to return an error in a non-error situation,
> >> which IMnsHO is a bug.
> >>
> >>> Because, setting the device's status to active (RPM_ACTIVE) via
> >>> __pm_runtime_set_status(), requires its parent to also be active (in
> >>> case the parent has runtime PM enabled).
> >>
> >> No!!!
> >>
> >> The check is in there, because the parent's usage_count is affected by that
> >> code and incrementing it in case the parent has runtime PM enabled and is
> >> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  
> >> IOW,
> >> it would confuse the framework.
> >
> > Right, I do understand the reasons *why* it is like this.
> >
> >>
> >> There's no such issue if the runtime PM status of a child is set to 
> >> RPM_SUSPENDED.
> >>
> >> It is all consistent without the check I'm removing and is made 
> >> inconsistent
> >> by that very check.
> >>
> >>> I would prefer to try maintain this consistency, but I also I realize
> >>> that commit a8636c89648a, should also have been checking if the parent
> >>> has runtime PM enabled (again for consistency), which it doesn't.
> >>>
> >>> What about fixing that instead?
> >>
> >> Runtime PM is *disabled* for the parent at this point, guaranteed, so 
> >> there's
> >> nothing to check, I'm afraid ...
> >
> > Where and how is that guarantee made?
> 
> Oh, just realize that it should be "child" instead of "parent", in the
> above reasoning. Apologize for giving the wrong argument.
> 
> So, let's me take this once again, to make it clear.
> 
> When pm_runtime_set_suspended(dev) is called, dev's child device may
> still be runtime PM enabled and active.
> I was suggesting to add a check for this scenario, to see if dev's
> child device is runtime PM is enabled, as and additional constraint
> before deciding to return an error code.

Well, that's sort of difficult to do, however, because the code would need to
walk all of the children of the device and the child power lock cannot be
acquired under the one of the parent, so it would be fragile and ugly.

> The idea was to get a consistent behavior, from the
> pm_runtime_set_active|suspended() APIs point of view, and not from the
> runtime PM core point of view.

Yes, but the cost is high and the benefit is shallow.

The enable-time WARN() should cover the really broken cases without that
much complexity.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Tuesday, November 14, 2017 10:56:39 AM CET Ulf Hansson wrote:
> On 14 November 2017 at 10:13, Ulf Hansson  wrote:
> > On 13 November 2017 at 22:50, Rafael J. Wysocki  wrote:
> >> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
> >>> On 12 November 2017 at 01:27, Rafael J. Wysocki  
> >>> wrote:
> >>> > From: Rafael J. Wysocki 
> >>> >
> >>> > The check for "active" children in __pm_runtime_set_status(), when
> >>> > trying to set the parent device status to "suspended", doesn't
> >>> > really make sense, because in fact it is not invalid to set the
> >>> > status of a device with runtime PM disabled to "suspended" in any
> >>> > case.  It is invalid to enable runtime PM for a device with its
> >>> > status set to "suspended" while its child_count reference counter
> >>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> >>> > really cover that situation.
> >>>
> >>> The reason to why I changed this in commit a8636c89648a ("PM /
> >>> Runtime: Don't allow to suspend a device with an active child") was
> >>> because to get a consistent behavior.
> >>
> >> Well, it causes the function to return an error in a non-error situation,
> >> which IMnsHO is a bug.
> >>
> >>> Because, setting the device's status to active (RPM_ACTIVE) via
> >>> __pm_runtime_set_status(), requires its parent to also be active (in
> >>> case the parent has runtime PM enabled).
> >>
> >> No!!!
> >>
> >> The check is in there, because the parent's usage_count is affected by that
> >> code and incrementing it in case the parent has runtime PM enabled and is
> >> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  
> >> IOW,
> >> it would confuse the framework.
> >
> > Right, I do understand the reasons *why* it is like this.
> >
> >>
> >> There's no such issue if the runtime PM status of a child is set to 
> >> RPM_SUSPENDED.
> >>
> >> It is all consistent without the check I'm removing and is made 
> >> inconsistent
> >> by that very check.
> >>
> >>> I would prefer to try maintain this consistency, but I also I realize
> >>> that commit a8636c89648a, should also have been checking if the parent
> >>> has runtime PM enabled (again for consistency), which it doesn't.
> >>>
> >>> What about fixing that instead?
> >>
> >> Runtime PM is *disabled* for the parent at this point, guaranteed, so 
> >> there's
> >> nothing to check, I'm afraid ...
> >
> > Where and how is that guarantee made?
> 
> Oh, just realize that it should be "child" instead of "parent", in the
> above reasoning. Apologize for giving the wrong argument.
> 
> So, let's me take this once again, to make it clear.
> 
> When pm_runtime_set_suspended(dev) is called, dev's child device may
> still be runtime PM enabled and active.
> I was suggesting to add a check for this scenario, to see if dev's
> child device is runtime PM is enabled, as and additional constraint
> before deciding to return an error code.

Well, that's sort of difficult to do, however, because the code would need to
walk all of the children of the device and the child power lock cannot be
acquired under the one of the parent, so it would be fragile and ugly.

> The idea was to get a consistent behavior, from the
> pm_runtime_set_active|suspended() APIs point of view, and not from the
> runtime PM core point of view.

Yes, but the cost is high and the benefit is shallow.

The enable-time WARN() should cover the really broken cases without that
much complexity.

Thanks,
Rafael

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

[Ulf Hansson]

On 14 November 2017 at 10:13, Ulf Hansson  wrote:
> On 13 November 2017 at 22:50, Rafael J. Wysocki  wrote:
>> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
>>> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
>>> > From: Rafael J. Wysocki 
>>> >
>>> > The check for "active" children in __pm_runtime_set_status(), when
>>> > trying to set the parent device status to "suspended", doesn't
>>> > really make sense, because in fact it is not invalid to set the
>>> > status of a device with runtime PM disabled to "suspended" in any
>>> > case.  It is invalid to enable runtime PM for a device with its
>>> > status set to "suspended" while its child_count reference counter
>>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>>> > really cover that situation.
>>>
>>> The reason to why I changed this in commit a8636c89648a ("PM /
>>> Runtime: Don't allow to suspend a device with an active child") was
>>> because to get a consistent behavior.
>>
>> Well, it causes the function to return an error in a non-error situation,
>> which IMnsHO is a bug.
>>
>>> Because, setting the device's status to active (RPM_ACTIVE) via
>>> __pm_runtime_set_status(), requires its parent to also be active (in
>>> case the parent has runtime PM enabled).
>>
>> No!!!
>>
>> The check is in there, because the parent's usage_count is affected by that
>> code and incrementing it in case the parent has runtime PM enabled and is
>> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
>> it would confuse the framework.
>
> Right, I do understand the reasons *why* it is like this.
>
>>
>> There's no such issue if the runtime PM status of a child is set to 
>> RPM_SUSPENDED.
>>
>> It is all consistent without the check I'm removing and is made inconsistent
>> by that very check.
>>
>>> I would prefer to try maintain this consistency, but I also I realize
>>> that commit a8636c89648a, should also have been checking if the parent
>>> has runtime PM enabled (again for consistency), which it doesn't.
>>>
>>> What about fixing that instead?
>>
>> Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
>> nothing to check, I'm afraid ...
>
> Where and how is that guarantee made?

Oh, just realize that it should be "child" instead of "parent", in the
above reasoning. Apologize for giving the wrong argument.

So, let's me take this once again, to make it clear.

When pm_runtime_set_suspended(dev) is called, dev's child device may
still be runtime PM enabled and active.
I was suggesting to add a check for this scenario, to see if dev's
child device is runtime PM is enabled, as and additional constraint
before deciding to return an error code.

The idea was to get a consistent behavior, from the
pm_runtime_set_active|suspended() APIs point of view, and not from the
runtime PM core point of view.

Of course, because dev->power.child_count, is maintained properly even
when runtime PM is disabled for dev's child device, it works as you
have suggested in $subject patch as well.

[...]

Kind regards
Uffe

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

[Ulf Hansson]

On 14 November 2017 at 10:13, Ulf Hansson  wrote:
> On 13 November 2017 at 22:50, Rafael J. Wysocki  wrote:
>> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
>>> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
>>> > From: Rafael J. Wysocki 
>>> >
>>> > The check for "active" children in __pm_runtime_set_status(), when
>>> > trying to set the parent device status to "suspended", doesn't
>>> > really make sense, because in fact it is not invalid to set the
>>> > status of a device with runtime PM disabled to "suspended" in any
>>> > case.  It is invalid to enable runtime PM for a device with its
>>> > status set to "suspended" while its child_count reference counter
>>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>>> > really cover that situation.
>>>
>>> The reason to why I changed this in commit a8636c89648a ("PM /
>>> Runtime: Don't allow to suspend a device with an active child") was
>>> because to get a consistent behavior.
>>
>> Well, it causes the function to return an error in a non-error situation,
>> which IMnsHO is a bug.
>>
>>> Because, setting the device's status to active (RPM_ACTIVE) via
>>> __pm_runtime_set_status(), requires its parent to also be active (in
>>> case the parent has runtime PM enabled).
>>
>> No!!!
>>
>> The check is in there, because the parent's usage_count is affected by that
>> code and incrementing it in case the parent has runtime PM enabled and is
>> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
>> it would confuse the framework.
>
> Right, I do understand the reasons *why* it is like this.
>
>>
>> There's no such issue if the runtime PM status of a child is set to 
>> RPM_SUSPENDED.
>>
>> It is all consistent without the check I'm removing and is made inconsistent
>> by that very check.
>>
>>> I would prefer to try maintain this consistency, but I also I realize
>>> that commit a8636c89648a, should also have been checking if the parent
>>> has runtime PM enabled (again for consistency), which it doesn't.
>>>
>>> What about fixing that instead?
>>
>> Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
>> nothing to check, I'm afraid ...
>
> Where and how is that guarantee made?

Oh, just realize that it should be "child" instead of "parent", in the
above reasoning. Apologize for giving the wrong argument.

So, let's me take this once again, to make it clear.

When pm_runtime_set_suspended(dev) is called, dev's child device may
still be runtime PM enabled and active.
I was suggesting to add a check for this scenario, to see if dev's
child device is runtime PM is enabled, as and additional constraint
before deciding to return an error code.

The idea was to get a consistent behavior, from the
pm_runtime_set_active|suspended() APIs point of view, and not from the
runtime PM core point of view.

Of course, because dev->power.child_count, is maintained properly even
when runtime PM is disabled for dev's child device, it works as you
have suggested in $subject patch as well.

[...]

Kind regards
Uffe

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

[Ulf Hansson]

On 13 November 2017 at 22:50, Rafael J. Wysocki  wrote:
> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
>> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > The check for "active" children in __pm_runtime_set_status(), when
>> > trying to set the parent device status to "suspended", doesn't
>> > really make sense, because in fact it is not invalid to set the
>> > status of a device with runtime PM disabled to "suspended" in any
>> > case.  It is invalid to enable runtime PM for a device with its
>> > status set to "suspended" while its child_count reference counter
>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>> > really cover that situation.
>>
>> The reason to why I changed this in commit a8636c89648a ("PM /
>> Runtime: Don't allow to suspend a device with an active child") was
>> because to get a consistent behavior.
>
> Well, it causes the function to return an error in a non-error situation,
> which IMnsHO is a bug.
>
>> Because, setting the device's status to active (RPM_ACTIVE) via
>> __pm_runtime_set_status(), requires its parent to also be active (in
>> case the parent has runtime PM enabled).
>
> No!!!
>
> The check is in there, because the parent's usage_count is affected by that
> code and incrementing it in case the parent has runtime PM enabled and is
> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
> it would confuse the framework.

Right, I do understand the reasons *why* it is like this.

>
> There's no such issue if the runtime PM status of a child is set to 
> RPM_SUSPENDED.
>
> It is all consistent without the check I'm removing and is made inconsistent
> by that very check.
>
>> I would prefer to try maintain this consistency, but I also I realize
>> that commit a8636c89648a, should also have been checking if the parent
>> has runtime PM enabled (again for consistency), which it doesn't.
>>
>> What about fixing that instead?
>
> Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
> nothing to check, I'm afraid ...

Where and how is that guarantee made?

[...]

Kind regards
Uffe

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

[Ulf Hansson]

On 13 November 2017 at 22:50, Rafael J. Wysocki  wrote:
> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
>> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > The check for "active" children in __pm_runtime_set_status(), when
>> > trying to set the parent device status to "suspended", doesn't
>> > really make sense, because in fact it is not invalid to set the
>> > status of a device with runtime PM disabled to "suspended" in any
>> > case.  It is invalid to enable runtime PM for a device with its
>> > status set to "suspended" while its child_count reference counter
>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>> > really cover that situation.
>>
>> The reason to why I changed this in commit a8636c89648a ("PM /
>> Runtime: Don't allow to suspend a device with an active child") was
>> because to get a consistent behavior.
>
> Well, it causes the function to return an error in a non-error situation,
> which IMnsHO is a bug.
>
>> Because, setting the device's status to active (RPM_ACTIVE) via
>> __pm_runtime_set_status(), requires its parent to also be active (in
>> case the parent has runtime PM enabled).
>
> No!!!
>
> The check is in there, because the parent's usage_count is affected by that
> code and incrementing it in case the parent has runtime PM enabled and is
> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
> it would confuse the framework.

Right, I do understand the reasons *why* it is like this.

>
> There's no such issue if the runtime PM status of a child is set to 
> RPM_SUSPENDED.
>
> It is all consistent without the check I'm removing and is made inconsistent
> by that very check.
>
>> I would prefer to try maintain this consistency, but I also I realize
>> that commit a8636c89648a, should also have been checking if the parent
>> has runtime PM enabled (again for consistency), which it doesn't.
>>
>> What about fixing that instead?
>
> Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
> nothing to check, I'm afraid ...

Where and how is that guarantee made?

[...]

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Mon, Nov 13, 2017 at 10:50 PM, Rafael J. Wysocki  wrote:
> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
>> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > The check for "active" children in __pm_runtime_set_status(), when
>> > trying to set the parent device status to "suspended", doesn't
>> > really make sense, because in fact it is not invalid to set the
>> > status of a device with runtime PM disabled to "suspended" in any
>> > case.  It is invalid to enable runtime PM for a device with its
>> > status set to "suspended" while its child_count reference counter
>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>> > really cover that situation.
>>
>> The reason to why I changed this in commit a8636c89648a ("PM /
>> Runtime: Don't allow to suspend a device with an active child") was
>> because to get a consistent behavior.
>
> Well, it causes the function to return an error in a non-error situation,
> which IMnsHO is a bug.
>
>> Because, setting the device's status to active (RPM_ACTIVE) via
>> __pm_runtime_set_status(), requires its parent to also be active (in
>> case the parent has runtime PM enabled).
>
> No!!!
>
> The check is in there, because the parent's usage_count is affected by that

Actually, the parent's child_count is affected, but that doesn't matter here.

> code and incrementing it in case the parent has runtime PM enabled and is
> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
> it would confuse the framework.
>
> There's no such issue if the runtime PM status of a child is set to 
> RPM_SUSPENDED.
>
> It is all consistent without the check I'm removing and is made inconsistent
> by that very check.
>
>> I would prefer to try maintain this consistency, but I also I realize
>> that commit a8636c89648a, should also have been checking if the parent
>> has runtime PM enabled (again for consistency), which it doesn't.
>>
>> What about fixing that instead?
>
> Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
> nothing to check, I'm afraid ...
>
> OTOH, the runtime PM status of the children doesn't matter here, because their
> reference counters are not updated.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Mon, Nov 13, 2017 at 10:50 PM, Rafael J. Wysocki  wrote:
> On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
>> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > The check for "active" children in __pm_runtime_set_status(), when
>> > trying to set the parent device status to "suspended", doesn't
>> > really make sense, because in fact it is not invalid to set the
>> > status of a device with runtime PM disabled to "suspended" in any
>> > case.  It is invalid to enable runtime PM for a device with its
>> > status set to "suspended" while its child_count reference counter
>> > is nonzero, but the check in __pm_runtime_set_status() doesn't
>> > really cover that situation.
>>
>> The reason to why I changed this in commit a8636c89648a ("PM /
>> Runtime: Don't allow to suspend a device with an active child") was
>> because to get a consistent behavior.
>
> Well, it causes the function to return an error in a non-error situation,
> which IMnsHO is a bug.
>
>> Because, setting the device's status to active (RPM_ACTIVE) via
>> __pm_runtime_set_status(), requires its parent to also be active (in
>> case the parent has runtime PM enabled).
>
> No!!!
>
> The check is in there, because the parent's usage_count is affected by that

Actually, the parent's child_count is affected, but that doesn't matter here.

> code and incrementing it in case the parent has runtime PM enabled and is
> RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
> it would confuse the framework.
>
> There's no such issue if the runtime PM status of a child is set to 
> RPM_SUSPENDED.
>
> It is all consistent without the check I'm removing and is made inconsistent
> by that very check.
>
>> I would prefer to try maintain this consistency, but I also I realize
>> that commit a8636c89648a, should also have been checking if the parent
>> has runtime PM enabled (again for consistency), which it doesn't.
>>
>> What about fixing that instead?
>
> Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
> nothing to check, I'm afraid ...
>
> OTOH, the runtime PM status of the children doesn't matter here, because their
> reference counters are not updated.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
> > From: Rafael J. Wysocki 
> >
> > The check for "active" children in __pm_runtime_set_status(), when
> > trying to set the parent device status to "suspended", doesn't
> > really make sense, because in fact it is not invalid to set the
> > status of a device with runtime PM disabled to "suspended" in any
> > case.  It is invalid to enable runtime PM for a device with its
> > status set to "suspended" while its child_count reference counter
> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > really cover that situation.
> 
> The reason to why I changed this in commit a8636c89648a ("PM /
> Runtime: Don't allow to suspend a device with an active child") was
> because to get a consistent behavior.

Well, it causes the function to return an error in a non-error situation,
which IMnsHO is a bug.

> Because, setting the device's status to active (RPM_ACTIVE) via
> __pm_runtime_set_status(), requires its parent to also be active (in
> case the parent has runtime PM enabled).

No!!!

The check is in there, because the parent's usage_count is affected by that
code and incrementing it in case the parent has runtime PM enabled and is
RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
it would confuse the framework.

There's no such issue if the runtime PM status of a child is set to 
RPM_SUSPENDED.

It is all consistent without the check I'm removing and is made inconsistent
by that very check.

> I would prefer to try maintain this consistency, but I also I realize
> that commit a8636c89648a, should also have been checking if the parent
> has runtime PM enabled (again for consistency), which it doesn't.
> 
> What about fixing that instead?

Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
nothing to check, I'm afraid ...

OTOH, the runtime PM status of the children doesn't matter here, because their
reference counters are not updated.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Monday, November 13, 2017 2:26:28 PM CET Ulf Hansson wrote:
> On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
> > From: Rafael J. Wysocki 
> >
> > The check for "active" children in __pm_runtime_set_status(), when
> > trying to set the parent device status to "suspended", doesn't
> > really make sense, because in fact it is not invalid to set the
> > status of a device with runtime PM disabled to "suspended" in any
> > case.  It is invalid to enable runtime PM for a device with its
> > status set to "suspended" while its child_count reference counter
> > is nonzero, but the check in __pm_runtime_set_status() doesn't
> > really cover that situation.
> 
> The reason to why I changed this in commit a8636c89648a ("PM /
> Runtime: Don't allow to suspend a device with an active child") was
> because to get a consistent behavior.

Well, it causes the function to return an error in a non-error situation,
which IMnsHO is a bug.

> Because, setting the device's status to active (RPM_ACTIVE) via
> __pm_runtime_set_status(), requires its parent to also be active (in
> case the parent has runtime PM enabled).

No!!!

The check is in there, because the parent's usage_count is affected by that
code and incrementing it in case the parent has runtime PM enabled and is
RPM_SUSPENDED leads to an inconsistent runtime PM state of the parent.  IOW,
it would confuse the framework.

There's no such issue if the runtime PM status of a child is set to 
RPM_SUSPENDED.

It is all consistent without the check I'm removing and is made inconsistent
by that very check.

> I would prefer to try maintain this consistency, but I also I realize
> that commit a8636c89648a, should also have been checking if the parent
> has runtime PM enabled (again for consistency), which it doesn't.
> 
> What about fixing that instead?

Runtime PM is *disabled* for the parent at this point, guaranteed, so there's
nothing to check, I'm afraid ...

OTOH, the runtime PM status of the children doesn't matter here, because their
reference counters are not updated.

Thanks,
Rafael

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

[Ulf Hansson]

On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> The check for "active" children in __pm_runtime_set_status(), when
> trying to set the parent device status to "suspended", doesn't
> really make sense, because in fact it is not invalid to set the
> status of a device with runtime PM disabled to "suspended" in any
> case.  It is invalid to enable runtime PM for a device with its
> status set to "suspended" while its child_count reference counter
> is nonzero, but the check in __pm_runtime_set_status() doesn't
> really cover that situation.

The reason to why I changed this in commit a8636c89648a ("PM /
Runtime: Don't allow to suspend a device with an active child") was
because to get a consistent behavior.

Because, setting the device's status to active (RPM_ACTIVE) via
__pm_runtime_set_status(), requires its parent to also be active (in
case the parent has runtime PM enabled).

I would prefer to try maintain this consistency, but I also I realize
that commit a8636c89648a, should also have been checking if the parent
has runtime PM enabled (again for consistency), which it doesn't.

What about fixing that instead?

Kind regards
Uffe

>
> For this reason, drop the children check from __pm_runtime_set_status()
> and add a check against child_count reference counters of "suspended"
> devices to pm_runtime_enable().
>
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/runtime.c |   30 ++
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> goto out;
> }
>
> -   if (dev->power.runtime_status == status)
> +   if (dev->power.runtime_status == status || !parent)
> goto out_set;
>
> if (status == RPM_SUSPENDED) {
> -   /*
> -* It is invalid to suspend a device with an active child,
> -* unless it has been set to ignore its children.
> -*/
> -   if (!dev->power.ignore_children &&
> -   atomic_read(>power.child_count)) {
> -   dev_err(dev, "runtime PM trying to suspend device but 
> active child\n");
> -   error = -EBUSY;
> -   goto out;
> -   }
> -
> -   if (parent) {
> -   atomic_add_unless(>power.child_count, -1, 0);
> -   notify_parent = !parent->power.ignore_children;
> -   }
> -   goto out_set;
> -   }
> -
> -   if (parent) {
> +   atomic_add_unless(>power.child_count, -1, 0);
> +   notify_parent = !parent->power.ignore_children;
> +   } else {
> spin_lock_nested(>power.lock, SINGLE_DEPTH_NESTING);
>
> /*
> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
> else
> dev_warn(dev, "Unbalanced %s!\n", __func__);
>
> +   WARN(dev->power.runtime_status == RPM_SUSPENDED &&
> +!dev->power.ignore_children &&
> +atomic_read(>power.child_count) > 0,
> +"Enabling runtime PM for inactive device (%s) with active 
> children\n",
> +dev_name(dev));
> +
> spin_unlock_irqrestore(>power.lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_enable);
>

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

[Ulf Hansson]

On 12 November 2017 at 01:27, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> The check for "active" children in __pm_runtime_set_status(), when
> trying to set the parent device status to "suspended", doesn't
> really make sense, because in fact it is not invalid to set the
> status of a device with runtime PM disabled to "suspended" in any
> case.  It is invalid to enable runtime PM for a device with its
> status set to "suspended" while its child_count reference counter
> is nonzero, but the check in __pm_runtime_set_status() doesn't
> really cover that situation.

The reason to why I changed this in commit a8636c89648a ("PM /
Runtime: Don't allow to suspend a device with an active child") was
because to get a consistent behavior.

Because, setting the device's status to active (RPM_ACTIVE) via
__pm_runtime_set_status(), requires its parent to also be active (in
case the parent has runtime PM enabled).

I would prefer to try maintain this consistency, but I also I realize
that commit a8636c89648a, should also have been checking if the parent
has runtime PM enabled (again for consistency), which it doesn't.

What about fixing that instead?

Kind regards
Uffe

>
> For this reason, drop the children check from __pm_runtime_set_status()
> and add a check against child_count reference counters of "suspended"
> devices to pm_runtime_enable().
>
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/base/power/runtime.c |   30 ++
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1101,29 +1101,13 @@ int __pm_runtime_set_status(struct devic
> goto out;
> }
>
> -   if (dev->power.runtime_status == status)
> +   if (dev->power.runtime_status == status || !parent)
> goto out_set;
>
> if (status == RPM_SUSPENDED) {
> -   /*
> -* It is invalid to suspend a device with an active child,
> -* unless it has been set to ignore its children.
> -*/
> -   if (!dev->power.ignore_children &&
> -   atomic_read(>power.child_count)) {
> -   dev_err(dev, "runtime PM trying to suspend device but 
> active child\n");
> -   error = -EBUSY;
> -   goto out;
> -   }
> -
> -   if (parent) {
> -   atomic_add_unless(>power.child_count, -1, 0);
> -   notify_parent = !parent->power.ignore_children;
> -   }
> -   goto out_set;
> -   }
> -
> -   if (parent) {
> +   atomic_add_unless(>power.child_count, -1, 0);
> +   notify_parent = !parent->power.ignore_children;
> +   } else {
> spin_lock_nested(>power.lock, SINGLE_DEPTH_NESTING);
>
> /*
> @@ -1307,6 +1291,12 @@ void pm_runtime_enable(struct device *de
> else
> dev_warn(dev, "Unbalanced %s!\n", __func__);
>
> +   WARN(dev->power.runtime_status == RPM_SUSPENDED &&
> +!dev->power.ignore_children &&
> +atomic_read(>power.child_count) > 0,
> +"Enabling runtime PM for inactive device (%s) with active 
> children\n",
> +dev_name(dev));
> +
> spin_unlock_irqrestore(>power.lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_enable);
>