| Message ID | CAPDyKFr8D4jL2vwO5tdBJnNVP=DoAD6H_K=4SQbu7rFAZFd3Xg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi, > From: Ulf Hansson, Sent: Friday, December 1, 2017 6:22 PM > > + Kishon > > On 30 November 2017 at 13:51, Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > Hi, > > > >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM > >> > >> On 29 November 2017 at 10:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> > Hi Ulf, > > <snip> > >> 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-<dev_name>.<id> and <dev_name> 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 (ee080000.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
Hi again, > From: Yoshihiro Shimoda, Sent: Friday, December 1, 2017 8:04 PM > > Hi, > <snip> > However, the following message still exists. > > Enabling runtime PM for inactive device (ee080000.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
On 1 December 2017 at 12:03, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Hi, > >> From: Ulf Hansson, Sent: Friday, December 1, 2017 6:22 PM >> >> + Kishon >> >> On 30 November 2017 at 13:51, Yoshihiro Shimoda >> <yoshihiro.shimoda.uh@renesas.com> wrote: >> > Hi, >> > >> >> From: Ulf Hansson, Sent: Wednesday, November 29, 2017 6:59 PM >> >> >> >> On 29 November 2017 at 10:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> >> > Hi Ulf, >> > <snip> >> >> 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-<dev_name>.<id> and <dev_name> 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 (ee080000.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
Hi, > From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM > > On 1 December 2017 at 12:03, Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: <snip> > > 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 (ee080000.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
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 > > <yoshihiro.shimoda.uh@renesas.com> wrote: > <snip> > > > 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 (ee080000.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
On Tue, Dec 5, 2017 at 4:03 PM, Alan Stern <stern@rowland.harvard.edu> 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 >> > <yoshihiro.shimoda.uh@renesas.com> wrote: >> <snip> >> > > 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 (ee080000.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
On 5 December 2017 at 04:23, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Hi, > >> From: Ulf Hansson, Sent: Monday, December 4, 2017 7:41 PM >> >> On 1 December 2017 at 12:03, Yoshihiro Shimoda >> <yoshihiro.shimoda.uh@renesas.com> wrote: > <snip> >> > 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 (ee080000.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
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 = 0; /* Override possible ret == -ENOTSUPP */ + pm_runtime_get_sync(phy->dev.parent); mutex_lock(&phy->mutex); if (phy->power_count == 0 && phy->ops->power_on) { @@ -306,8 +296,7 @@ int phy_power_on(struct phy *phy) err_pwr_on: mutex_unlock(&phy->mutex); - phy_pm_runtime_put_sync(phy); -err_pm_sync: + pm_runtime_put(phy->dev.parent); if (phy->pwr) regulator_disable(phy->pwr); out: @@ -333,7 +322,7 @@ int phy_power_off(struct phy *phy) } --phy->power_count; mutex_unlock(&phy->mutex); - phy_pm_runtime_put(phy); + pm_runtime_put(phy->dev.parent); if (phy->pwr) regulator_disable(phy->pwr); @@ -774,11 +763,6 @@ struct phy *phy_create(struct device *dev, struct device_node *node, if (ret) goto put_dev; - if (pm_runtime_enabled(dev)) { - pm_runtime_enable(&phy->dev); - pm_runtime_no_callbacks(&phy->dev); - } - return phy;