| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51ED30A8.10200@ti.com>
Date: Mon, 22 Jul 2013 16:16:24 +0300
From: Roger Quadros <rogerq@...com>
To: Alan Stern <stern@...land.harvard.edu>
CC: <gregkh@...uxfoundation.org>, <balbi@...com>,
<sergei.shtylyov@...entembedded.com>, <khilman@...aro.org>,
<tony@...mide.com>, <ruslan.bilovol@...com>,
<linux-usb@...r.kernel.org>, <linux-omap@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume
Hi Alan,
On 07/11/2013 06:14 PM, Alan Stern wrote:
> On Thu, 11 Jul 2013, Roger Quadros wrote:
>
>>> The other two problems are both related to the interaction between
>>> system PM and runtime PM. Suppose the controller is already runtime
>>> suspended when the system goes to sleep. Because it is runtime
>>> suspended, it is enabled for wakeup. But device_may_wakeup() could
>>> return false; if this happens then you have to do a runtime-resume in
>>> omap_ehci_suspend() before calling ehci_suspend(), so that the
>>> controller can be suspended again with wakeups disabled. (Or you could
>>> choose an alternative method for accomplishing the same result, such as
>>> disabling the wakeup signal from the pad without going through a whole
>>> EHCI resume/suspend cycle.) Conversely, if device_may_wakeup() returns
>>> true then you shouldn't do anything at all, because the controller is
>>> already suspended with the correct wakeup setting.
>>
>> I think this case is taken care of by the Runtime PM core at least for the OMAP
>> platform according to the documentation
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647
>
> No; that section refers only to races, not to wakeup settings.
>
>> At the end of this mail is the log during system suspend/resume
>>
>> You can notice the following sequence
>>
>> -ehci runtime suspends
>> -system suspend triggered
>> -ehci runtime resumes
>> -ehci suspends (uses new wakeup settings)
>> -system wakeup triggered
>> -ehci resumes
>> -ehci runtime suspends
>
> This is because the root hub was runtime suspended with the wrong
> wakeup setting. The USB core, which is careful about these things,
> resumed and re-suspended it with the proper wakeup setting. In the
> process, the controller had to be runtime resumed as well.
>
> Try doing the test over again, but this time with the root hub enabled
> for wakeup and the controller disabled. (I know this is a bizarre
> combination, but try it anyway.) Also, after the system wakes up, see
> whether the root hub and controller get runtime suspended.
>
The first part of the test caught the problem were we were trying to access
EHCI registers when HW is not accessible. So this was a good test case.
>>> For the third problem, suppose the controller was runtime suspended
>>> when the system went to sleep. When the system wakes up, the
>>> controller will become active, so you have to inform the runtime PM
>>> core about its change of state. Basically, if omap_ehci_resume() sees
>>> that ehci_resume() returned 0 then it must do:
>>>
>>> pm_runtime_disable(dev);
>>> pm_runtime_set_active(dev);
>>> pm_runtime_enable(dev);
>>>
>>> All of these issues are discussed (among lots of other material) in
>>> Documentation/power/runtime_pm.txt.
>>
>> Is this still applicable? Documentation claims
>>
>> "During system resume it calls pm_runtime_enable() and pm_runtime_put_sync()
>> for every device right after executing the subsystem-level .resume_early()
>> callback and right after executing the subsystem-level .resume() callback
>> for it, respectively."
>
> Yes, this is applicable, but it is irrelevant to the problem I
> described. You still have to tell the runtime PM core that the device
> is now active.
Right, I understand it now. How does the below code look?
+static int omap_ehci_suspend(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ bool do_wakeup = device_may_wakeup(dev);
+ int ret;
+
+ dev_dbg(dev, "%s may_wakeup %d\n", __func__, do_wakeup);
+
+ if (pm_runtime_status_suspended(dev)) {
+ pm_runtime_get_sync(dev);
+ ehci_resume(hcd, false);
+ ret = ehci_suspend(hcd, do_wakeup);
+ pm_runtime_put_sync(dev);
+
+ } else {
+ ret = ehci_suspend(hcd, do_wakeup);
+ }
+
+ return ret;
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ int ret;
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ ret = ehci_resume(hcd, false);
+ if (!ret) {
+ /*
+ * Controller was powered ON so reflect state
+ */
+ pm_runtime_disable(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ }
+
+ return ret;
+}
+
+static int omap_ehci_runtime_suspend(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ if (omap->bound)
+ ehci_suspend(hcd, true);
+
+ return 0;
+}
+
+static int omap_ehci_runtime_resume(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv;
+
+ dev_dbg(dev, "%s\n", __func__);
+
+ if (omap->bound)
+ ehci_resume(hcd, false);
+
+ return 0;
+}
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists