From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752167AbcHIA5d (ORCPT ); Mon, 8 Aug 2016 20:57:33 -0400 Received: from mail-db5eur01on0073.outbound.protection.outlook.com ([104.47.2.73]:62048 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751433AbcHIA5a (ORCPT ); Mon, 8 Aug 2016 20:57:30 -0400 From: Peter Chen To: Stephen Boyd , Peter Chen CC: "linux-usb@vger.kernel.org" , Felipe Balbi , Arnd Bergmann , Neil Armstrong , "linux-arm-msm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Bjorn Andersson , Greg Kroah-Hartman , Andy Gross , "linux-arm-kernel@lists.infradead.org" Subject: RE: [PATCH v2 08/22] usb: chipidea: Remove locking in ci_udc_start() Thread-Topic: [PATCH v2 08/22] usb: chipidea: Remove locking in ci_udc_start() Thread-Index: AQHR2J3qfJphw3HtKku6tXM15Q0xkqAOSa8AgCzMzwCAAKfSgIAELwQAgAAOO4A= Date: Tue, 9 Aug 2016 00:42:07 +0000 Message-ID: References: <20160707222114.1673-1-stephen.boyd@linaro.org> <20160707222114.1673-9-stephen.boyd@linaro.org> <20160708094528.GF20485@shlinux2> <147043403652.26915.10865210145828633146@sboyd-linaro> <20160806075435.GB12298@shlinux2> <147070007426.10825.11324896309403492272@sboyd-linaro> In-Reply-To: <147070007426.10825.11324896309403492272@sboyd-linaro> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=peter.chen@nxp.com; x-originating-ip: [192.158.241.86] x-ms-office365-filtering-correlation-id: a3e25d17-c075-4fa5-cd85-08d3bfedfde2 x-microsoft-exchange-diagnostics: 1;VI1PR04MB1453;6:2oLacFoDwa8JwnkbBNkGDB3lzDjv4uIyiXy4TxJpbj51xhHS41GR7px9V/QLjY+grzO3N+q7+nT96eVPjosNwj7MmodU1neRoOHWxYy9isORNyCackkSuWiriko2HkXhbrEXFuWQDcs/Bj0cT7gN7V9HPc4G2Uk2hUJLVgeiKkTWwz40nDhxFz+lNTNG/WoiTfrnr/W4kCoqJ1JmI1kMN4IOMAByfA+skfAlHjYxgusvuUT+LOwvbOnNgyV74zp7brWhS50pojfF8JYM8PjkynkyypSY4eovfh4UH2GMULQ+25fOzd05TZwvhy0kTbIe+QViQjpSHQnd+1zXcSC8gQ==;5:ROlElR84CfgZn8zv4BWBtUN0864jnBZKoeUBWSlsYJCUoc3/bxmXyGJEHRrzcYZgopksaj9tKZxKcy4CX0KU/JYGt3nB3vapxgrSJ9y3NLrvLhntsXDyiiMrHqn+NHIjLUbAF50n282j14lZ4zn/YA==;24:euCno+G035JIxYkbs3V7bbw1Vhn/mF1nCQbIWp5uENb/0QrYrvtzJsBnoKBcsXeyZJX3oHgmErooOCx+je8KtGUrxkJOHqsXO4bVYCDOzgo=;7:BGqt3ao88+ZIuUojzm4KFdt3HKEYb6Gfh9YJvR1ZVSiO0h9kDkwnYpYIiGGt+cFiN4PkbobnU7mwJ6UYgz9rDvwTRFtuP3hFyEgHNkGeiOI+fdtoLN2uXnFx0RMFPKYwcpyuPbGeK74chiLkPSsMTjBtliNkkpaYaaqgUuS9Vj7u7d10H3mmMOf/J9e0Q5i4pyqQMSoxSvF3CJ1+Lx7iSYRBwtMnPcJJBw+AZWXFcfcqT581tQJRPQlQafmaWjNA x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:VI1PR04MB1453; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(185117386973197); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026);SRVR:VI1PR04MB1453;BCL:0;PCL:0;RULEID:;SRVR:VI1PR04MB1453; x-forefront-prvs: 0029F17A3F x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(7916002)(199003)(189002)(377424004)(24454002)(9686002)(81156014)(76576001)(81166006)(8676002)(8936002)(122556002)(2906002)(68736007)(4326007)(7416002)(33656002)(101416001)(76176999)(50986999)(3660700001)(54356999)(3280700002)(74316002)(7846002)(7736002)(7696003)(305945005)(586003)(5002640100001)(10400500002)(11100500001)(105586002)(5001770100001)(2900100001)(97736004)(93886004)(92566002)(77096005)(87936001)(66066001)(6116002)(86362001)(2950100001)(189998001)(3846002)(102836003)(575784001)(106116001)(106356001)(19580395003)(19580405001);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR04MB1453;H:VI1PR04MB1455.eurprd04.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Aug 2016 00:42:07.6174 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB1453 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u790vc2F024472 >Quoting Peter Chen (2016-08-06 00:54:35) >> On Fri, Aug 05, 2016 at 02:53:56PM -0700, Stephen Boyd wrote: >> > Quoting Peter Chen (2016-07-08 02:45:28) >> > > On Thu, Jul 07, 2016 at 03:20:59PM -0700, Stephen Boyd wrote: >> > > > We don't call hw_device_reset() with the ci->lock held, so it >> > > > doesn't seem like this lock here is protecting anything. Let's >> > > > just remove it. This allows us to call sleeping functions like >> > > > phy_init() from within the CI_HDRC_CONTROLLER_RESET_EVENT hook. >> > > > >> > > > Cc: Peter Chen >> > > > Cc: Greg Kroah-Hartman >> > > > Signed-off-by: Stephen Boyd >> > > > --- >> > > > drivers/usb/chipidea/udc.c | 3 --- >> > > > 1 file changed, 3 deletions(-) >> > > > >> > > > diff --git a/drivers/usb/chipidea/udc.c >> > > > b/drivers/usb/chipidea/udc.c index 065f5d97aa67..f16be4710cdb >> > > > 100644 >> > > > --- a/drivers/usb/chipidea/udc.c >> > > > +++ b/drivers/usb/chipidea/udc.c >> > > > @@ -1719,7 +1719,6 @@ static int ci_udc_start(struct usb_gadget *gadget, >> > > > struct usb_gadget_driver *driver) { >> > > > struct ci_hdrc *ci = container_of(gadget, struct ci_hdrc, gadget); >> > > > - unsigned long flags; >> > > > int retval = -ENOMEM; >> > > > >> > > > if (driver->disconnect == NULL) @@ -1746,7 +1745,6 @@ >> > > > static int ci_udc_start(struct usb_gadget *gadget, >> > > > >> > > > pm_runtime_get_sync(&ci->gadget.dev); >> > > > if (ci->vbus_active) { >> > > > - spin_lock_irqsave(&ci->lock, flags); >> > > > hw_device_reset(ci); >> > > > } else { >> > > > usb_udc_vbus_handler(&ci->gadget, false); @@ >> > > > -1755,7 +1753,6 @@ static int ci_udc_start(struct usb_gadget *gadget, >> > > > } >> > > > >> > > > retval = hw_device_state(ci, ci->ep0out->qh.dma); >> > > > - spin_unlock_irqrestore(&ci->lock, flags); >> > > > if (retval) >> > > > pm_runtime_put_sync(&ci->gadget.dev); >> > > > >> > > >> > > The main purpose for this is disabling interrupt when reset >> > > controller, in case the unexpected interrupts occur. >> > > >> > > You can move this between hw_device_state. >> > > >> > >> > Ok, but we don't hold the ci->lock in ci_udc_vbus_session(). Is that >> > a bug as well? >> >> I agree with your patch. In fact, during the reset controller, the >> interrupt has not been enabled, it should no unexpected interrupt. >> > >So then we can leave this patch as is? It still isn't clear to me what sequence of >events that would cause a problem if we don't hold the >ci->lock around hw_device_state(). Yes. I am ok with this patch. After thinking more, it is safe to be without lock. And we have this unlock code in nxp internal tree, and without any problems during 1-2 years. Acked-by: Peter Chen Peter