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>] [day] [month] [year] [list]
Message-ID: <CAMz4kuL2u59B+tDEv2gW0Ko_WpTpcAq6CvbKVHG4KJf5rPMyEA@mail.gmail.com>
Date:	Wed, 10 Aug 2016 16:23:02 +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>,
	Muhammad Falak R Wani <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 10 August 2016 at 16:17, Greg KH <gregkh@...uxfoundation.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@...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.
>
> 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