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:	Mon, 21 Mar 2016 04:22:54 +0000
From:	Rajesh Bhagat <rajesh.bhagat@....com>
To:	Alan Stern <stern@...land.harvard.edu>
CC:	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"mathias.nyman@...el.com" <mathias.nyman@...el.com>,
	Sriram Dash <sriram.dash@....com>
Subject: RE: [PATCH] usb: xhci: Fix incomplete PM resume operation due to XHCI
 commmand timeout



> -----Original Message-----
> From: Alan Stern [mailto:stern@...land.harvard.edu]
> Sent: Friday, March 18, 2016 7:51 PM
> 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
> <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!
> 

My Apologies, I completely missed the above comment which was added before. 

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

I agree to your point. Actually the intent was to check the return status of reset_device which 
is currently being ignored. I encountered the reset_device failure case in resume operation (STR) 
which is increasing the time of resume and causing unexpected crashes if return value is not checked. 

Do you agree it should be checked here? If yes, I can rework this patch. 

> Alan Stern