Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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()
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()
+ 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(&phy->mutex); if (phy->init_count == 0 && phy->ops->init) { @@ -239,22 +236,19 @@ int phy_init(struct phy *phy) out: mutex_unlock(&phy->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(&phy->mutex); if (phy->init_count == 1 && phy->ops->exit) { @@ -268,7 +262,7 @@ int phy_exit(struct phy *phy) out: mutex_unlock(&phy->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 =
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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()
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()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
Hi Ulf, On Wed, Nov 29, 2017 at 10:24 AM, Ulf Hansson 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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()
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(&dev->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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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(&dev->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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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(&dev->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(&parent->power.child_count, -1, 0); >> - notify_parent = !parent->power.ignore_children; >> - } >> - goto out_set; >> - } >> - >> - if (parent) { >> + atomic_add_unless(&parent->power.child_count, -1, 0); >> + notify_parent = !parent->power.ignore_children; >> + } else { >> spin_lock_nested(&parent->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(&dev->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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] PM / runtime: Drop children check from __pm_runtime_set_status()
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(&dev->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()
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(&dev->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(&parent->power.child_count, -1, 0); > - notify_parent = !parent->power.ignore_children; > - } > - goto out_set; > - } > - > - if (parent) { > + atomic_add_unless(&parent->power.child_count, -1, 0); > + notify_parent = !parent->power.ignore_children; > + } else { > spin_lock_nested(&parent->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(&dev->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+0x22