usb: core: Add runtime resume checking

Message ID 653c8e893d4de1ab4dcade2ca2da9f2cfe369c2c.1470735102.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

Baolin Wang Aug. 9, 2016, 9:33 a.m.
When the usb device has entered suspend state by runtime suspend method, and
the sustem also try to enter suspend state by issuing usb_dev_suspend(), it
will issue pm_runtime_resume() function to deal with wrong wakeup setting in
choose_wakeup() function.

But if usb device resumes failed due to xhci has been into suspend state and
hardware is not accessible, which will set runtime errors. Thus when there is
slave attached, usb device will resume failed by runtime resume method due to
previous runtime errors.

Then we should check if it resumes successfully in choose_wakeup() function,
if it failed we should clear the runtime errors by pm_runtime_set_suspended()
function to avoid runtime resume failure.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

---
 drivers/usb/core/driver.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Baolin Wang Aug. 10, 2016, 6:43 a.m. | #1
Hi Peter,

On 10 August 2016 at 14:18, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Wed, Aug 10, 2016 at 10:33:31AM +0800, Baolin Wang wrote:

>> Hi Greg,

>>

>> On 9 August 2016 at 18:26, Greg KH <gregkh@linuxfoundation.org> wrote:

>> > On Tue, Aug 09, 2016 at 05:33:33PM +0800, Baolin Wang wrote:

>> >> When the usb device has entered suspend state by runtime suspend method, and

>> >> the sustem also try to enter suspend state by issuing usb_dev_suspend(), it

>> >> will issue pm_runtime_resume() function to deal with wrong wakeup setting in

>> >> choose_wakeup() function.

>> >>

>> >> But if usb device resumes failed due to xhci has been into suspend state and

>> >> hardware is not accessible, which will set runtime errors. Thus when there is

>> >> slave attached, usb device will resume failed by runtime resume method due to

>> >> previous runtime errors.

>> >

>> > I really can't parse the first sentance in this paragraph, what exactly

>> > makes xhci so "unique" here?

>>

>> Sorry for confusing, I try to explain it clearly. 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.

>>

>> For example: 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() function (issued in choose_wakeup() function) will

>> return -ESHUTDOWN due to xhci has been suspend and hardware is not

>> accessible.

>

> Why the controller does not be resumed when the root hub has issued

> runtime resume?


Because now there is no slave attached, we will not power on usb
controller and resume xhci.

>

> Peter

>

>>

>> After system entering resume state, if there is slave attached ---->

>> power on usb controller -----> xhci resume -----> usb device runtime

>> resume ----> usb interface runtime resume. 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.

>>

>> >

>> >> Then we should check if it resumes successfully in choose_wakeup() function,

>> >

>> > what is "it"?

>>

>> It present pm_runtime_resume() issued in choose_wakeup() function.

>>

>> >

>> >> if it failed we should clear the runtime errors by pm_runtime_set_suspended()

>> >> function to avoid runtime resume failure.

>> >

>> > Again, what is "it"?

>>

>> It present pm_runtime_resume() issued in choose_wakeup() function.

>>

>> >

>> >>

>> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

>> >> ---

>> >>  drivers/usb/core/driver.c |    9 +++++++--

>> >>  1 file changed, 7 insertions(+), 2 deletions(-)

>> >>

>> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c

>> >> index dadd1e8d..a1a0f5f 100644

>> >> --- a/drivers/usb/core/driver.c

>> >> +++ b/drivers/usb/core/driver.c

>> >> @@ -1412,6 +1412,7 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)

>> >>  static void choose_wakeup(struct usb_device *udev, pm_message_t msg)

>> >>  {

>> >>       int     w;

>> >> +     int     ret;

>> >>

>> >>       /* Remote wakeup is needed only when we actually go to sleep.

>> >>        * For things like FREEZE and QUIESCE, if the device is already

>> >> @@ -1431,8 +1432,12 @@ 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) {

>> >> +             ret = pm_runtime_resume(&udev->dev);

>> >> +             if (ret == -ESHUTDOWN)

>> >> +                     pm_runtime_set_suspended(&udev->dev);

>> >

>> > why is 'ret' needed:

>> >                 if (pm_runtime_resume(&udev->dev) == -ESHUTDOWN)

>>

>> OK. I can modify it in next version if you agree this patch.

>>

>> >

>> >

>> > Why would resume fail?

>>

>> Like I explained above. Thanks.

>>

>> --

>> Baolin.wang

>> Best Regards

>> --

>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in

>> the body of a message to majordomo@vger.kernel.org

>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

>

> --

>

> Best Regards,

> Peter Chen




-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index dadd1e8d..a1a0f5f 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1412,6 +1412,7 @@  static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
 static void choose_wakeup(struct usb_device *udev, pm_message_t msg)
 {
 	int	w;
+	int	ret;
 
 	/* Remote wakeup is needed only when we actually go to sleep.
 	 * For things like FREEZE and QUIESCE, if the device is already
@@ -1431,8 +1432,12 @@  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) {
+		ret = pm_runtime_resume(&udev->dev);
+		if (ret == -ESHUTDOWN)
+			pm_runtime_set_suspended(&udev->dev);
+	}
+
 	udev->do_remote_wakeup = w;
 }