From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030401AbcHaQdJ (ORCPT ); Wed, 31 Aug 2016 12:33:09 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:43272 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S935148AbcHaQb2 (ORCPT ); Wed, 31 Aug 2016 12:31:28 -0400 Date: Wed, 31 Aug 2016 12:29:57 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Chuang Dong cc: gregkh@linuxfoundation.org, , , , , Subject: Re: [PATCH] UHCI: Setting remote wakeup capibility is failure In-Reply-To: <1472635761-116393-1-git-send-email-Chuang.Dong@windriver.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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