linux-kernel - Re: [PATCH] usb: core: Add runtime resume checking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMz4kuKH_gvyObrthqvyWPN0x+62pLq-F0Fk-08Jk0Er5-Pv6Q@mail.gmail.com>
Date:	Wed, 10 Aug 2016 10:33:31 +0800
From:	Baolin Wang <baolin.wang@...aro.org>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	stefan.koch10@...il.com, Alan Stern <stern@...land.harvard.edu>,
	Oliver Neukum <oneukum@...e.com>, falakreyaz@...il.com,
	Mark Brown <broonie@...nel.org>,
	USB <linux-usb@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usb: core: Add runtime resume checking

Hi Greg,

On 9 August 2016 at 18:26, Greg KH <gregkh@...uxfoundation.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@...aro.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