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]

Date:	Tue, 9 Aug 2016 12:26:31 +0200
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Baolin Wang <baolin.wang@...aro.org>
Cc:	stefan.koch10@...il.com, stern@...land.harvard.edu,
	oneukum@...e.com, falakreyaz@...il.com, broonie@...nel.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: core: Add runtime resume checking

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


Why would resume fail?

thanks,

greg k-h