[PATCH] UHCI: Setting remote wakeup capibility is failure

[PATCH] UHCI: Setting remote wakeup capibility is failure

[Chuang Dong]

When a system is put into S3 sleep, a usb terminal connected to the
UHCI host port can't wake up the system.

For example: if the command "pm-suspend" is used to put the system into
S3 mode, the system cannot be woken up via a keyboard connected to the
UCHI port.

The reason is the suspend_rh() setting UHCI to sleep mode fails.
suspend_rh() always checks "rhdev->do_remote_wakeup == 0", the checking
determines the value setting the registers EGSM and RD. These registers
can be used for setting sleep mode. If rhdev->do_remote_wakeup == 0, it
means the root hub can't do remote wakeup operation, the registers
EGSM and RD can't be set to sleep mode. As rhdev->do_remote_wakeup
always equals 0, the relavant registers can't be set, neither can the 
sleep mode.

As a result,the UHCI S3 sleep mode is set to failure, accordingly,
waking system through a usb terminal connected to UHCI host port fails
as well, because the usb teminal's wakeup irq can not be detected by
UHCI host.

The sleep test is incorrect, since rhdev->do_remote_wakeup is evaluated
in choose_wakeup(), as shown in the following code block:

choose_wakeup()
{
    ...
    w = device_may_wakeup(&udev->dev);
    dev->do_remote_wakeup = w;
    ...
}

device_may_wakeup() is based on device_wakeup_enable().
As root hub is not a wakeup resource, which is a part of usb host, thus
device_wakeup_enable() is not called. This causes the
"udev->do_remote_wakeup == 0" to forever be in S3 sleep mode, and hence
the sleep mode is incorrectly setup, which in turn causes the wakeup
failure.

The solution is to change the S3 sleep condition to use the usb host
wakeup capability instead of the root hub's capability. Since the root
hub is part of usb host, most of the operations and data are initiated
by the usb host, and also the root hub doesn't have any upstream ports
to suspend(and hence shutdown their downstream HC-to-USB),we can safely
use the host controller to setup sleep mode in suspend_rh().

Similarly we can use the host controller wakeup capability as the 
conditional test.

Signed-off-by: Chuang Dong <Chuang.Dong@windriver.com>
---
 drivers/usb/host/uhci-hcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index a7de8e8..6363463 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -287,6 +287,7 @@ __acquires(uhci->lock)
 	int auto_stop;
 	int int_enable, egsm_enable, wakeup_enable;
 	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
+	struct device *controller = uhci_to_hcd(uhci)->self.controller;
 
 	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
 	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
@@ -315,7 +316,7 @@ __acquires(uhci->lock)
 	 * for the root hub.
 	 */
 	else {
-		if (!rhdev->do_remote_wakeup)
+		if (!device_may_wakeup(controller))
 			wakeup_enable = 0;
 	}
 #endif
-- 
1.9.1

Re: [PATCH] UHCI: Setting remote wakeup capibility is failure

[Alan Stern]

On Wed, 31 Aug 2016, Chuang Dong wrote:

> When a system is put into S3 sleep, a usb terminal connected to the
> UHCI host port can't wake up the system.
> 
> For example: if the command "pm-suspend" is used to put the system into
> S3 mode, the system cannot be woken up via a keyboard connected to the
> UCHI port.
> 
> The reason is the suspend_rh() setting UHCI to sleep mode fails.
> suspend_rh() always checks "rhdev->do_remote_wakeup == 0", the checking
> determines the value setting the registers EGSM and RD. These registers
> can be used for setting sleep mode. If rhdev->do_remote_wakeup == 0, it
> means the root hub can't do remote wakeup operation, the registers
> EGSM and RD can't be set to sleep mode. As rhdev->do_remote_wakeup
> always equals 0, the relavant registers can't be set, neither can the 
> sleep mode.
> 
> As a result,the UHCI S3 sleep mode is set to failure, accordingly,
> waking system through a usb terminal connected to UHCI host port fails
> as well, because the usb teminal's wakeup irq can not be detected by
> UHCI host.
> 
> The sleep test is incorrect, since rhdev->do_remote_wakeup is evaluated
> in choose_wakeup(), as shown in the following code block:
> 
> choose_wakeup()
> {
>     ...
>     w = device_may_wakeup(&udev->dev);
>     dev->do_remote_wakeup = w;
>     ...
> }
> 
> device_may_wakeup() is based on device_wakeup_enable().
> As root hub is not a wakeup resource,

What makes you say that?  Have you written "enabled" to the root hub's
power/wakeup sysfs file?

>  which is a part of usb host, thus
> device_wakeup_enable() is not called. This causes the
> "udev->do_remote_wakeup == 0" to forever be in S3 sleep mode, and hence
> the sleep mode is incorrectly setup, which in turn causes the wakeup
> failure.
> 
> The solution is to change the S3 sleep condition to use the usb host
> wakeup capability instead of the root hub's capability. Since the root
> hub is part of usb host, most of the operations and data are initiated
> by the usb host, and also the root hub doesn't have any upstream ports
> to suspend(and hence shutdown their downstream HC-to-USB),we can safely
> use the host controller to setup sleep mode in suspend_rh().
> 
> Similarly we can use the host controller wakeup capability as the 
> conditional test.
> 
> Signed-off-by: Chuang Dong <Chuang.Dong@windriver.com>
> ---
>  drivers/usb/host/uhci-hcd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> index a7de8e8..6363463 100644
> --- a/drivers/usb/host/uhci-hcd.c
> +++ b/drivers/usb/host/uhci-hcd.c
> @@ -287,6 +287,7 @@ __acquires(uhci->lock)
>  	int auto_stop;
>  	int int_enable, egsm_enable, wakeup_enable;
>  	struct usb_device *rhdev = uhci_to_hcd(uhci)->self.root_hub;
> +	struct device *controller = uhci_to_hcd(uhci)->self.controller;
>  
>  	auto_stop = (new_state == UHCI_RH_AUTO_STOPPED);
>  	dev_dbg(&rhdev->dev, "%s%s\n", __func__,
> @@ -315,7 +316,7 @@ __acquires(uhci->lock)
>  	 * for the root hub.
>  	 */
>  	else {
> -		if (!rhdev->do_remote_wakeup)
> +		if (!device_may_wakeup(controller))
>  			wakeup_enable = 0;
>  	}
>  #endif

This change isn't needed.

Alan Stern