RE: [PATCH] usb: xhci: Fix incomplete PM resume operation due to XHCI commmand timeout

[Rajesh Bhagat <rajesh.bhagat@nxp.com>]

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Friday, March 18, 2016 7:51 PM
> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> gregkh@linuxfoundation.org; mathias.nyman@intel.com; Sriram Dash
> <sriram.dash@nxp.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