Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1458546 > unrolled thread

[PATCH] usb: core: Add runtime resume checking

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: core: Add runtime resume checking Baolin Wang <baolin.wang@linaro.org> - 2016-08-09 11:40 +0200
    Re: [PATCH] usb: core: Add runtime resume checking Greg KH <gregkh@linuxfoundation.org> - 2016-08-09 12:30 +0200
      Re: [PATCH] usb: core: Add runtime resume checking Baolin Wang <baolin.wang@linaro.org> - 2016-08-10 04:40 +0200
        Re: [PATCH] usb: core: Add runtime resume checking Baolin Wang <baolin.wang@linaro.org> - 2016-08-10 20:20 +0200
          Re: [PATCH] usb: core: Add runtime resume checking Alan Stern <stern@rowland.harvard.edu> - 2016-08-10 20:40 +0200
        Re: [PATCH] usb: core: Add runtime resume checking Peter Chen <hzpeterchen@gmail.com> - 2016-08-10 20:20 +0200
        Re: [PATCH] usb: core: Add runtime resume checking Peter Chen <hzpeterchen@gmail.com> - 2016-08-10 21:30 +0200
          Re: [PATCH] usb: core: Add runtime resume checking Baolin Wang <baolin.wang@linaro.org> - 2016-08-10 22:20 +0200
        Re: [PATCH] usb: core: Add runtime resume checking Baolin Wang <baolin.wang@linaro.org> - 2016-08-10 21:30 +0200

#1458546 — [PATCH] usb: core: Add runtime resume checking

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(-)

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;
 }
 
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1458579

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?

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

what is "it"?

> if it failed we should clear the runtime errors by pm_runtime_set_suspended()
> function to avoid runtime resume failure.

Again, what is "it"?

> 
> 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)


Why would resume fail?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1459245

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.

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

[toc] | [prev] | [next] | [standalone]

#1459372

On 10 August 2016 at 17:07, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Wed, Aug 10, 2016 at 02:43:50PM +0800, Baolin Wang wrote:
>> 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.
>>
>
> I don't know why you set policy like this. Even the controller is
> resumed, it will still be suspended by system suspend. What's more,
> if you disallow it, how can you change your wakeup setting?
> Eg, at runtime suspend, we enable wakeup by default. But for system
> sleep, we disable wakeup by default.
>
> At runtime, if there is no device on the port, even we resumes the
> controller and roohub, it still will be suspended soon.

For saving power, if there is no slave attached, why we need to power
on usb controller and resume xhci? Especially for mobile device.

Moreover the usb device runtime suspend/resume is separate with xhci
suspend/resume, when xhci entered suspend state, only slave attaching
can resume xhci.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1459487

On Wed, 10 Aug 2016, Baolin Wang wrote:

> >> >> 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.
> >>
> >
> > I don't know why you set policy like this. Even the controller is
> > resumed, it will still be suspended by system suspend. What's more,
> > if you disallow it, how can you change your wakeup setting?
> > Eg, at runtime suspend, we enable wakeup by default. But for system
> > sleep, we disable wakeup by default.
> >
> > At runtime, if there is no device on the port, even we resumes the
> > controller and roohub, it still will be suspended soon.
> 
> For saving power, if there is no slave attached, why we need to power
> on usb controller and resume xhci? Especially for mobile device.

You need to power-on the USB controller in order to change the wakeup
setting.

> Moreover the usb device runtime suspend/resume is separate with xhci
> suspend/resume, when xhci entered suspend state, only slave attaching
> can resume xhci.

That's right.  Suppose the user wants the system to stay asleep when a
slave attaches.  But when you suspended the xHCI controller, it was set
to wake up when a new slave attaches.  So when a slave is attached, the 
controller will wake up the system.  That's bad.

If the wakeup setting needs to be changed when the system suspend
begins, then you have to power-on the controller to make that change.

Given a choice between using a little power or behaving incorrectly, 
you should choose to use the power.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1459377

On Wed, Aug 10, 2016 at 02:43:50PM +0800, Baolin Wang wrote:
> 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.
> 

I don't know why you set policy like this. Even the controller is
resumed, it will still be suspended by system suspend. What's more,
if you disallow it, how can you change your wakeup setting?
Eg, at runtime suspend, we enable wakeup by default. But for system
sleep, we disable wakeup by default.

At runtime, if there is no device on the port, even we resumes the
controller and roohub, it still will be suspended soon.

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1459663

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?

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

[toc] | [prev] | [next] | [standalone]

#1459797

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

[toc] | [prev] | [next] | [standalone]

#1459666

Hi Greg,

On 10 August 2016 at 16:17, Greg KH <gregkh@linuxfoundation.org> 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.
>>
>> 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.
>
> Please provide that explaination in the patch description for everyone
> to see it.

OK. I'll resend the patch with above explaination. Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web