[PATCH v2] usb: core: Add runtime resume checking in choose_wakeup()

[PATCH v2] usb: core: Add runtime resume checking in choose_wakeup()

[Baolin Wang]

Considering strict power management for mobile device, we should also power
off the usb controller if there are no slaves attached even though it is usb
host function, but it will meet usb device resume failure in below situation.

Suppose that no slave attached ----> usb interface runtime suspend ---->
usb device runtime suspend -----> xhci suspend -----> power off usb controller.
After that if the system wants to enter suspend state, then it also will issue
usb_dev_suspend(), then the pm_runtime_resume() issued in choose_wakeup()
function will return '-ESHUTDOWN' due to xhci has been suspend and hardware is
not accessible.

After system entering resume state, if there is slave attached ----> power on
usb controller ----> xhci resume ----> usb device runtime resume ---->
usb interface runtime resume. But usb device will resume failed if runtime
errors is set ('-ESHUTDOWN'), thus we should clear the runtime errors in
choose_wakeup() function to avoid this situation.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
changes since v1:
 - Modify the changelog to explain clearly.
 - Remove 'ret' variable in choose_wakeup().
---
 drivers/usb/core/driver.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index dadd1e8d..bce283c 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1431,8 +1431,11 @@ static void choose_wakeup(struct usb_device *udev, pm_message_t msg)
 	/* If the device is autosuspended with the wrong wakeup setting,
 	 * autoresume now so the setting can be changed.
 	 */
-	if (udev->state == USB_STATE_SUSPENDED && w != udev->do_remote_wakeup)
-		pm_runtime_resume(&udev->dev);
+	if (udev->state == USB_STATE_SUSPENDED && w != udev->do_remote_wakeup) {
+		if (pm_runtime_resume(&udev->dev) == -ESHUTDOWN)
+			pm_runtime_set_suspended(&udev->dev);
+	}
+
 	udev->do_remote_wakeup = w;
 }
 
-- 
1.7.9.5

Re: [PATCH v2] usb: core: Add runtime resume checking in choose_wakeup()

[Alan Stern]

On Wed, 10 Aug 2016, Baolin Wang wrote:

> Considering strict power management for mobile device, we should also power
> off the usb controller if there are no slaves attached even though it is usb
> host function, but it will meet usb device resume failure in below situation.
> 
> Suppose that no slave attached ----> usb interface runtime suspend ---->
> usb device runtime suspend -----> xhci suspend -----> power off usb controller.
> After that if the system wants to enter suspend state, then it also will issue
> usb_dev_suspend(), then the pm_runtime_resume() issued in choose_wakeup()
> function will return '-ESHUTDOWN' due to xhci has been suspend and hardware is
> not accessible.

No, this is wrong.  The pm_runtime_resume in choose_wakeup() should 
cause the xHCI controller to resume also.

> After system entering resume state, if there is slave attached ----> power on
> usb controller ----> xhci resume ----> usb device runtime resume ---->
> usb interface runtime resume. But usb device will resume failed if runtime
> errors is set ('-ESHUTDOWN'), thus we should clear the runtime errors in
> choose_wakeup() function to avoid this situation.

If the controller resumes correctly, this won't be necessary.

Alan Stern

Re: [PATCH v2] usb: core: Add runtime resume checking in choose_wakeup()

[Baolin Wang]

On 10 August 2016 at 22:31, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 10 Aug 2016, Baolin Wang wrote:
>
>> Considering strict power management for mobile device, we should also power
>> off the usb controller if there are no slaves attached even though it is usb
>> host function, but it will meet usb device resume failure in below situation.
>>
>> Suppose that no slave attached ----> usb interface runtime suspend ---->
>> usb device runtime suspend -----> xhci suspend -----> power off usb controller.
>> After that if the system wants to enter suspend state, then it also will issue
>> usb_dev_suspend(), then the pm_runtime_resume() issued in choose_wakeup()
>> function will return '-ESHUTDOWN' due to xhci has been suspend and hardware is
>> not accessible.
>
> No, this is wrong.  The pm_runtime_resume in choose_wakeup() should
> cause the xHCI controller to resume also.

But now it won't, it just resume usb device not xHCI controller. I
suppose mainline kernel does not sopport this now.

>
>> After system entering resume state, if there is slave attached ----> power on
>> usb controller ----> xhci resume ----> usb device runtime resume ---->
>> usb interface runtime resume. But usb device will resume failed if runtime
>> errors is set ('-ESHUTDOWN'), thus we should clear the runtime errors in
>> choose_wakeup() function to avoid this situation.
>
> If the controller resumes correctly, this won't be necessary.
>
> Alan Stern
>



-- 
Baolin.wang
Best Regards

Re: [PATCH v2] usb: core: Add runtime resume checking in choose_wakeup()

[Peter Chen]

On Thu, Aug 11, 2016 at 11:11:08AM +0800, Baolin Wang wrote:
> On 10 August 2016 at 22:31, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 10 Aug 2016, Baolin Wang wrote:
> >
> >> Considering strict power management for mobile device, we should also power
> >> off the usb controller if there are no slaves attached even though it is usb
> >> host function, but it will meet usb device resume failure in below situation.
> >>
> >> Suppose that no slave attached ----> usb interface runtime suspend ---->
> >> usb device runtime suspend -----> xhci suspend -----> power off usb controller.
> >> After that if the system wants to enter suspend state, then it also will issue
> >> usb_dev_suspend(), then the pm_runtime_resume() issued in choose_wakeup()
> >> function will return '-ESHUTDOWN' due to xhci has been suspend and hardware is
> >> not accessible.
> >
> > No, this is wrong.  The pm_runtime_resume in choose_wakeup() should
> > cause the xHCI controller to resume also.
> 
> But now it won't, it just resume usb device not xHCI controller. I
> suppose mainline kernel does not sopport this now.
> 

Why? It works ok at my environment. The controller is the ancestor of
the USB device, if the USB device would like to be woken up, the
controller will be woken up first.

-- 

Best Regards,
Peter Chen

Re: [PATCH v2] usb: core: Add runtime resume checking in choose_wakeup()

[Baolin Wang]

Hi Peter,

On 11 August 2016 at 14:52, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Thu, Aug 11, 2016 at 11:11:08AM +0800, Baolin Wang wrote:
>> On 10 August 2016 at 22:31, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Wed, 10 Aug 2016, Baolin Wang wrote:
>> >
>> >> Considering strict power management for mobile device, we should also power
>> >> off the usb controller if there are no slaves attached even though it is usb
>> >> host function, but it will meet usb device resume failure in below situation.
>> >>
>> >> Suppose that no slave attached ----> usb interface runtime suspend ---->
>> >> usb device runtime suspend -----> xhci suspend -----> power off usb controller.
>> >> After that if the system wants to enter suspend state, then it also will issue
>> >> usb_dev_suspend(), then the pm_runtime_resume() issued in choose_wakeup()
>> >> function will return '-ESHUTDOWN' due to xhci has been suspend and hardware is
>> >> not accessible.
>> >
>> > No, this is wrong.  The pm_runtime_resume in choose_wakeup() should
>> > cause the xHCI controller to resume also.
>>
>> But now it won't, it just resume usb device not xHCI controller. I
>> suppose mainline kernel does not sopport this now.
>>
>
> Why? It works ok at my environment. The controller is the ancestor of
> the USB device, if the USB device would like to be woken up, the

Yes, the controller is the ancestor of the USB device. The routine
will be like below if we resume USB device:

usb_resume_device() ----> generic_resume() ----> hcd_bus_resume() --->
xhci_bus_resume(), but now xHCI is not accessible due to
xhci_suspend() is issued and USB controller is power off when no slave
attached. If we want to resume USB device sucessfully, we must
power-on USB controller and resume xhci by issuing xhci_resume(), but
now how to trigger the USB controller action and resume xHCI just from
pm_runtime_resume() function?

> controller will be woken up first.

I suppose your xHCI controller will not be suspended and your USB
controller will not be power-off when no slave attached (but now
system is not in suspend state), right?

-- 
Baolin.wang
Best Regards