linux-kernel - Re: [PATCH] usb: xhci: Fix incomplete PM resume operation due to XHCI commmand timeout

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:	Fri, 18 Mar 2016 10:21:21 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Rajesh Bhagat <rajesh.bhagat@....com>
cc:	linux-usb@...r.kernel.org, <linux-kernel@...r.kernel.org>,
	<gregkh@...uxfoundation.org>, <mathias.nyman@...el.com>,
	<sriram.dash@....com>
Subject: Re: [PATCH] usb: xhci: Fix incomplete PM resume operation due to
 XHCI commmand timeout

On Fri, 18 Mar 2016, Rajesh Bhagat wrote:

> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2897,10 +2897,14 @@ done:
>  			/* The xHC may think the device is already reset,
>  			 * so ignore the status.
>  			 */
> -			if (hcd->driver->reset_device)
> -				hcd->driver->reset_device(hcd, udev);
> -
> -			usb_set_device_state(udev, USB_STATE_DEFAULT);
> +			if (hcd->driver->reset_device) {
> +				status = hcd->driver->reset_device(hcd, udev);
> +				if (status == 0)
> +					usb_set_device_state(udev, USB_STATE_DEFAULT);
> +				else
> +					usb_set_device_state(udev, USB_STATE_NOTATTACHED);
> +			} else
> +				usb_set_device_state(udev, USB_STATE_DEFAULT);

This is a really bad patch:

You left in the comment about ignoring the status, but then you changed 
the code so that it doesn't ignore the status!

You also called usb_set_device_state() more times than necessary.  You 
could have done it like this:

			if (hcd->driver->reset_device)
				status = hcd->driver->reset_device(hcd, udev);
			if (status == 0)
				usb_set_device_state(udev, USB_STATE_DEFAULT);
			else
				usb_set_device_state(udev, USB_STATE_NOTATTACHED);

(Even that could be simplified further, by combining it with the code
that follows.)

Finally, you violated the 80-column limit.

Alan Stern