From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1757343AbcCROV0 (ORCPT <rfc822;w@1wt.eu>);
	Fri, 18 Mar 2016 10:21:26 -0400
Received: from iolanthe.rowland.org ([192.131.102.54]:58322 "HELO
	iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with SMTP id S1751820AbcCROVX (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 18 Mar 2016 10:21:23 -0400
Date: Fri, 18 Mar 2016 10:21:21 -0400 (EDT)
From: Alan Stern <stern@rowland.harvard.edu>
X-X-Sender: stern@iolanthe.rowland.org
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@nxp.com>
Subject: Re: [PATCH] usb: xhci: Fix incomplete PM resume operation due to
 XHCI commmand timeout
In-Reply-To: <1458284463-12743-1-git-send-email-rajesh.bhagat@nxp.com>
Message-ID: <Pine.LNX.4.44L0.1603181016140.1866-100000@iolanthe.rowland.org>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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