Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757533AbcJNOgf (ORCPT ); Fri, 14 Oct 2016 10:36:35 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:45224 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753042AbcJNOg1 (ORCPT ); Fri, 14 Oct 2016 10:36:27 -0400 Date: Fri, 14 Oct 2016 10:36:25 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Felipe Balbi 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: <87oa2ntlty.fsf@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1861 Lines: 42 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: > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.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"); > > 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); > > bitmap_zero(f->endpoints, 32); > } > > 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 when usb_ep_disable() runs. At least, a quick check shows that both net2280 and dummy-hcd use spin_lock_irqsave() rather than spin_lock() in their disable routines. Holding locks is a different story. It should be okay for a gadget driver to hold one of its own locks when disabling an endpoint (which means that the gadget's disable routine shouldn't wait for a concurrent giveback to finish), but we might want to avoid holding a lock in the composite core. Although even that might be okay -- I can't think of any reason why a udc driver would need to call back into the composite core while disabling an endpoint. It should be a pretty self-contained operation. Alan Stern