From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752459AbcCUEhy (ORCPT ); Mon, 21 Mar 2016 00:37:54 -0400 Received: from mail-am1on0086.outbound.protection.outlook.com ([157.56.112.86]:31992 "EHLO emea01-am1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751043AbcCUEhp convert rfc822-to-8bit (ORCPT ); Mon, 21 Mar 2016 00:37:45 -0400 From: Rajesh Bhagat To: Alan Stern CC: "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "gregkh@linuxfoundation.org" , "mathias.nyman@intel.com" , Sriram Dash Subject: RE: [PATCH] usb: xhci: Fix incomplete PM resume operation due to XHCI commmand timeout Thread-Topic: [PATCH] usb: xhci: Fix incomplete PM resume operation due to XHCI commmand timeout Thread-Index: AQHRgOP0koj0An7NrUiFwLeaADZ9zZ9fQRuAgAQOjQA= Date: Mon, 21 Mar 2016 04:22:54 +0000 Message-ID: References: <1458284463-12743-1-git-send-email-rajesh.bhagat@nxp.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: rowland.harvard.edu; dkim=none (message not signed) header.d=none;rowland.harvard.edu; dmarc=none action=none header.from=nxp.com; x-originating-ip: [192.88.169.1] x-ms-office365-filtering-correlation-id: 467b53ac-5155-48ee-1454-08d35140797b x-microsoft-exchange-diagnostics: 1;AM2PR04MB0548;5:B0GP9vmGKgPbF8nS4bAsgPyZw4/cIgfJfTbTshVC6DCdnG/cfmlwDgRLwzQiQtC0ROd9apuAVvkPfX66fBrNNG5CYZDHPhYD8vcD6mvLcIa2OQLrfShflnQIaWjilORC3VQ8WgJoRbQ9KDFxcSoZdw==;24:c+nOi1EoPMRn/xAu8bFmhyr2YDOxevpBZE1MdYYF3LjnxCUL5BI7E9aFO8kakMktce8yRGXi6z5e3y5y/TqJmLsdqqm7r2c+g8lMh2s9J48= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM2PR04MB0548; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001);SRVR:AM2PR04MB0548;BCL:0;PCL:0;RULEID:;SRVR:AM2PR04MB0548; x-forefront-prvs: 0888B1D284 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(377454003)(24454002)(1096002)(5002640100001)(50986999)(5008740100001)(33656002)(110136002)(92566002)(2171001)(102836003)(76176999)(6116002)(3846002)(19580395003)(19580405001)(189998001)(5004730100002)(3660700001)(4326007)(54356999)(1220700001)(74316001)(3280700002)(586003)(2906002)(5003600100002)(10400500002)(66066001)(122556002)(76576001)(77096005)(81166005)(106116001)(2950100001)(87936001)(86362001)(49343001);DIR:OUT;SFP:1101;SCL:1;SRVR:AM2PR04MB0548;H:HE1PR0401MB2028.eurprd04.prod.outlook.com;FPR:;SPF:None;MLV:sfv;LANG:en; spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Mar 2016 04:22:54.6144 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM2PR04MB0548 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Alan Stern [mailto:stern@rowland.harvard.edu] > Sent: Friday, March 18, 2016 7:51 PM > To: Rajesh Bhagat > Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; > gregkh@linuxfoundation.org; mathias.nyman@intel.com; Sriram Dash > > 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