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@xxxxxxxxxxxxx>
> ---
>  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