[PATCH] usb: core: Add runtime resume checking

[PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Greg KH]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Peter Chen]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Peter Chen]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Alan Stern]

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

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

Hi Alan,

On 10 August 2016 at 22:25, Alan Stern <stern@rowland.harvard.edu> wrote:
> 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.

pm_runtime_resume() function issued in choose_wakeup() can just resume
usb device, which can not power-on the USB controller and resume xHCI.
Cause the user won't know you need to power-on usb controller now,
then how to power-on the USB controller when changing 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.

OK. But that is a real problem. It will pm_runtime_resume() falied
(issued in choose_wakeup()), cause usb controller has powered-off and
xHCI controller has suspended and we have no method to notify the user
to power-on USB controller. Any good suggestion to solve this, Alan
and Peter? Thanks.

-- 
Baolin.wang
Best Regards

Re: [PATCH] usb: core: Add runtime resume checking

[Peter Chen]

On Thu, Aug 11, 2016 at 11:08:52AM +0800, Baolin Wang wrote:
> Hi Alan,
> 
> On 10 August 2016 at 22:25, Alan Stern <stern@rowland.harvard.edu> wrote:
> > 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.
> 
> pm_runtime_resume() function issued in choose_wakeup() can just resume
> usb device, which can not power-on the USB controller and resume xHCI.
> Cause the user won't know you need to power-on usb controller now,
> then how to power-on the USB controller when changing 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.
> 
> OK. But that is a real problem. It will pm_runtime_resume() falied
> (issued in choose_wakeup()), cause usb controller has powered-off and
> xHCI controller has suspended and we have no method to notify the user
> to power-on USB controller. Any good suggestion to solve this, Alan
> and Peter? Thanks.
> 

Maybe you can show us the call stack why pm_runtime_resume has failed
at your environment. 

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

Hi Peter,

On 11 August 2016 at 14:54, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Thu, Aug 11, 2016 at 11:08:52AM +0800, Baolin Wang wrote:
>> Hi Alan,
>>
>> On 10 August 2016 at 22:25, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > 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.
>>
>> pm_runtime_resume() function issued in choose_wakeup() can just resume
>> usb device, which can not power-on the USB controller and resume xHCI.
>> Cause the user won't know you need to power-on usb controller now,
>> then how to power-on the USB controller when changing 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.
>>
>> OK. But that is a real problem. It will pm_runtime_resume() falied
>> (issued in choose_wakeup()), cause usb controller has powered-off and
>> xHCI controller has suspended and we have no method to notify the user
>> to power-on USB controller. Any good suggestion to solve this, Alan
>> and Peter? Thanks.
>>
>
> Maybe you can show us the call stack why pm_runtime_resume has failed
> at your environment.

OK. I try to explain it at below:

For example: No slave attached----> usb interface runtime suspend
----> usb device runtime suspend (routine is:
usb_suspend_device()--->generic_suspend() --> hcd_bus_suspend()-->
xhci_bus_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.

We issue the pm_runtime_resume() function routine: usb_resume_device()
----> generic_resume() ----> hcd_bus_resume() ---> xhci_bus_resume(),
but now xHCI is not accessible due to xhci_suspend() is issued and USB
controller is power off when no slave attached. That is why
pm_runtime_resume failed.

-- 
Baolin.wang
Best Regards

Re: [PATCH] usb: core: Add runtime resume checking

[Peter Chen]

On Thu, Aug 11, 2016 at 04:07:21PM +0800, Baolin Wang wrote:
> Hi Peter,
> 
> On 11 August 2016 at 14:54, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Thu, Aug 11, 2016 at 11:08:52AM +0800, Baolin Wang wrote:
> >> Hi Alan,
> >>
> >> On 10 August 2016 at 22:25, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > 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.
> >>
> >> pm_runtime_resume() function issued in choose_wakeup() can just resume
> >> usb device, which can not power-on the USB controller and resume xHCI.
> >> Cause the user won't know you need to power-on usb controller now,
> >> then how to power-on the USB controller when changing 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.
> >>
> >> OK. But that is a real problem. It will pm_runtime_resume() falied
> >> (issued in choose_wakeup()), cause usb controller has powered-off and
> >> xHCI controller has suspended and we have no method to notify the user
> >> to power-on USB controller. Any good suggestion to solve this, Alan
> >> and Peter? Thanks.
> >>
> >
> > Maybe you can show us the call stack why pm_runtime_resume has failed
> > at your environment.
> 
> OK. I try to explain it at below:
> 
> For example: No slave attached----> usb interface runtime suspend
> ----> usb device runtime suspend (routine is:
> usb_suspend_device()--->generic_suspend() --> hcd_bus_suspend()-->
> xhci_bus_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.
> 
> We issue the pm_runtime_resume() function routine: usb_resume_device()
> ----> generic_resume() ----> hcd_bus_resume() ---> xhci_bus_resume(),
> but now xHCI is not accessible due to xhci_suspend() is issued and USB
> controller is power off when no slave attached. That is why
> pm_runtime_resume failed.
> 

What host controller driver you are using? Assume you are using
xhci-plat.c. The problem for you is this driver does not implement
runtime pm operations, so the flag HCD_FLAG_HW_ACCESSIBLE is not set.
Maybe Robert's patch is a good start for you [1]

[1] http://www.spinics.net/lists/linux-usb/msg144602.html

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

On 11 August 2016 at 16:19, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Thu, Aug 11, 2016 at 04:07:21PM +0800, Baolin Wang wrote:
>> Hi Peter,
>>
>> On 11 August 2016 at 14:54, Peter Chen <hzpeterchen@gmail.com> wrote:
>> > On Thu, Aug 11, 2016 at 11:08:52AM +0800, Baolin Wang wrote:
>> >> Hi Alan,
>> >>
>> >> On 10 August 2016 at 22:25, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> > 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.
>> >>
>> >> pm_runtime_resume() function issued in choose_wakeup() can just resume
>> >> usb device, which can not power-on the USB controller and resume xHCI.
>> >> Cause the user won't know you need to power-on usb controller now,
>> >> then how to power-on the USB controller when changing 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.
>> >>
>> >> OK. But that is a real problem. It will pm_runtime_resume() falied
>> >> (issued in choose_wakeup()), cause usb controller has powered-off and
>> >> xHCI controller has suspended and we have no method to notify the user
>> >> to power-on USB controller. Any good suggestion to solve this, Alan
>> >> and Peter? Thanks.
>> >>
>> >
>> > Maybe you can show us the call stack why pm_runtime_resume has failed
>> > at your environment.
>>
>> OK. I try to explain it at below:
>>
>> For example: No slave attached----> usb interface runtime suspend
>> ----> usb device runtime suspend (routine is:
>> usb_suspend_device()--->generic_suspend() --> hcd_bus_suspend()-->
>> xhci_bus_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.
>>
>> We issue the pm_runtime_resume() function routine: usb_resume_device()
>> ----> generic_resume() ----> hcd_bus_resume() ---> xhci_bus_resume(),
>> but now xHCI is not accessible due to xhci_suspend() is issued and USB
>> controller is power off when no slave attached. That is why
>> pm_runtime_resume failed.
>>
>
> What host controller driver you are using? Assume you are using
> xhci-plat.c. The problem for you is this driver does not implement

Yes. I use xhci-plat.c.

> runtime pm operations, so the flag HCD_FLAG_HW_ACCESSIBLE is not set.
> Maybe Robert's patch is a good start for you [1]
>
> [1] http://www.spinics.net/lists/linux-usb/msg144602.html

OK. Maybe I need to implement the runtime PM callbacks for xhci-plat, right?

-- 
Baolin.wang
Best Regards

Re: [PATCH] usb: core: Add runtime resume checking

[Peter Chen]

On Thu, Aug 11, 2016 at 04:41:27PM +0800, Baolin Wang wrote:
> >> >>
> >> >> OK. But that is a real problem. It will pm_runtime_resume() falied
> >> >> (issued in choose_wakeup()), cause usb controller has powered-off and
> >> >> xHCI controller has suspended and we have no method to notify the user
> >> >> to power-on USB controller. Any good suggestion to solve this, Alan
> >> >> and Peter? Thanks.
> >> >>
> >> >
> >> > Maybe you can show us the call stack why pm_runtime_resume has failed
> >> > at your environment.
> >>
> >> OK. I try to explain it at below:
> >>
> >> For example: No slave attached----> usb interface runtime suspend
> >> ----> usb device runtime suspend (routine is:
> >> usb_suspend_device()--->generic_suspend() --> hcd_bus_suspend()-->
> >> xhci_bus_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.
> >>
> >> We issue the pm_runtime_resume() function routine: usb_resume_device()
> >> ----> generic_resume() ----> hcd_bus_resume() ---> xhci_bus_resume(),
> >> but now xHCI is not accessible due to xhci_suspend() is issued and USB
> >> controller is power off when no slave attached. That is why
> >> pm_runtime_resume failed.
> >>
> >
> > What host controller driver you are using? Assume you are using
> > xhci-plat.c. The problem for you is this driver does not implement
> 
> Yes. I use xhci-plat.c.
> 
> > runtime pm operations, so the flag HCD_FLAG_HW_ACCESSIBLE is not set.
> > Maybe Robert's patch is a good start for you [1]
> >
> > [1] http://www.spinics.net/lists/linux-usb/msg144602.html
> 
> OK. Maybe I need to implement the runtime PM callbacks for xhci-plat, right?
> 

I think so.

-- 

Best Regards,
Peter Chen

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

On 11 August 2016 at 16:44, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Thu, Aug 11, 2016 at 04:41:27PM +0800, Baolin Wang wrote:
>> >> >>
>> >> >> OK. But that is a real problem. It will pm_runtime_resume() falied
>> >> >> (issued in choose_wakeup()), cause usb controller has powered-off and
>> >> >> xHCI controller has suspended and we have no method to notify the user
>> >> >> to power-on USB controller. Any good suggestion to solve this, Alan
>> >> >> and Peter? Thanks.
>> >> >>
>> >> >
>> >> > Maybe you can show us the call stack why pm_runtime_resume has failed
>> >> > at your environment.
>> >>
>> >> OK. I try to explain it at below:
>> >>
>> >> For example: No slave attached----> usb interface runtime suspend
>> >> ----> usb device runtime suspend (routine is:
>> >> usb_suspend_device()--->generic_suspend() --> hcd_bus_suspend()-->
>> >> xhci_bus_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.
>> >>
>> >> We issue the pm_runtime_resume() function routine: usb_resume_device()
>> >> ----> generic_resume() ----> hcd_bus_resume() ---> xhci_bus_resume(),
>> >> but now xHCI is not accessible due to xhci_suspend() is issued and USB
>> >> controller is power off when no slave attached. That is why
>> >> pm_runtime_resume failed.
>> >>
>> >
>> > What host controller driver you are using? Assume you are using
>> > xhci-plat.c. The problem for you is this driver does not implement
>>
>> Yes. I use xhci-plat.c.
>>
>> > runtime pm operations, so the flag HCD_FLAG_HW_ACCESSIBLE is not set.
>> > Maybe Robert's patch is a good start for you [1]
>> >
>> > [1] http://www.spinics.net/lists/linux-usb/msg144602.html
>>
>> OK. Maybe I need to implement the runtime PM callbacks for xhci-plat, right?
>>
>
> I think so.

OK. I try to look at it. Thanks.

>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards

Re: [PATCH] usb: core: Add runtime resume checking

[Baolin Wang]

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