Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757846AbcJQILs (ORCPT ); Mon, 17 Oct 2016 04:11:48 -0400 Received: from mga05.intel.com ([192.55.52.43]:58419 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757783AbcJQILf (ORCPT ); Mon, 17 Oct 2016 04:11:35 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,356,1473145200"; d="asc'?scan'208";a="20296162" From: Felipe Balbi To: Alan Stern Cc: Baolin Wang , Greg KH , Mark Brown , USB , LKML Subject: Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq In-Reply-To: References: Date: Mon, 17 Oct 2016 11:10:45 +0300 Message-ID: <871szftmyi.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3292 Lines: 82 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, (I have added you to another thread which is where we'll be collecting discussion about this, however ...) Alan Stern writes: > On Fri, 14 Oct 2016, Felipe Balbi wrote: > >> argh, we have nested spinlocks :-( Well, we shouldn't call >> usb_ep_disable() with locks held AFAICR. So the following should be >> enough: >>=20 >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composi= te.c >> index 919d7d1b611c..2e9359c58eb9 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -732,8 +732,10 @@ static void reset_config(struct usb_composite_dev *= cdev) >> DBG(cdev, "reset config\n"); >>=20=20 >> list_for_each_entry(f, &cdev->config->functions, list) { >> + spin_unlock_irq(&cdev->lock); >> if (f->disable) >> f->disable(f); >> + spin_lock_irq(&cdev->lock); >>=20=20 >> bitmap_zero(f->endpoints, 32); >> } >>=20 >> Alan, do you remember if we have a requirement for not holding locks >> when calling usb_ep_disable()? I can't find Documentation about it, but >> history shows me that usb_ep_disable() was called without locks and IRQs >> enabled. Do you think we should update documentation about this? > > I don't think there is any requirement for interrupts to be enabled=20 > when usb_ep_disable() runs. At least, a quick check shows that both=20 > net2280 and dummy-hcd use spin_lock_irqsave() rather than spin_lock()=20 > in their disable routines. > > Holding locks is a different story. It should be okay for a gadget=20 > driver to hold one of its own locks when disabling an endpoint (which=20 > means that the gadget's disable routine shouldn't wait for a concurrent=20 > giveback to finish), but we might want to avoid holding a lock in the=20 > composite core. Although even that might be okay -- I can't think of=20 > any reason why a udc driver would need to call back into the composite=20 > core while disabling an endpoint. It should be a pretty self-contained=20 > operation. True, but how do we handle controllers which need to wait for an interrupt in order to cancel a transfer? Maybe we should change the calling context of usb_ep_disable() so that it _must_ be called with IRQs enabled? =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYBIeFAAoJEMy+uJnhGpkGKPMP/2SWt/W3Aj/4t3nvKEbkSQ1o gSensOh2lXovQu4NZAUeJAxNUKWWiiMt5j4wl+IxpyozFTu768VwKhc26ZSop0+E ngqQZcXhxSHIwyqEl9+e7qzZtdBekwORRBCqWofGjg8+XOfQA5RGazEz0isggsoP 0RcL52Apt5M/4GWX+TlJBCt27IYSpbXMpN/bJmaQ0VhOvV1XeNVM73au8mEmUW9E I9+C7jdZLUIw/tw5E2lBm7uI38E1jo/gmPs+Av/1TNOpfcpnHXFsmvuBbYd/cpK2 7IQ2PoVEFHDPK4jZNVVlyLbbKESWnB1Opn249ipu2wbZjwRyr/jkmC8WavMHWTOP /1WXZMXLAvYiMTu5Mwd3RzhE/Xye5MxDC6kI5RIUKxfjlkuVkmrFUSo3j9Q0eFID pECYh3mObqGH5Sqb2kSZadHCOT07Y+14PkSUicb9fa/aTSfzTx0p0CbH4wHlqQ0w +7Hu/fZwMo5WGfNdAQePJR2QNJKvtqd/o0yBhOc3UOryLXW6lYfayfR4goAFEbP0 i2FzNu1c09vEmHqwqxIG2IKraACqbKD/vSTz+hcrTrDCl9rzSRRouMeSjEIfBhzi n1imiPJuinPCWTnFh4E2QyqPDpPeFMCc6WijFqwUs/ogStKywdyidRM2UgZXs2WR /tH4NwnET4ziMngTfexI =Z6KS -----END PGP SIGNATURE----- --=-=-=--