On Thu, 2 Aug 2012, Dan Rittersdorf wrote:
> > It looks like the lesson is not to have one thread accessing an
> > endpoint at the same time as another thread issues a CLEAR_HALT.
>
> Thank you -- that spawned a couple thoughts on a potential solution.
>
> And I appreciate you refraining from simply saying "doh!" :-)
> I should have seen the problem as an application level issue, but was
> blind to it because I presumed that "usb.c" was known to "work".
If you have any bug fixes for usb.c, please send them in.
> > There are some known problems related to locking in gadgetfs (it
> > submits requests while holding a lock that the completion routine will
> > take). So far nobody has cared about it enough to do a careful audit
> > and fix.
>
> Yes, I ran into that issue quickly on this project. I addressed one
> case in ep0_read() by unlocking around the usb_ep_queue() call.
> I have not yet discovered any other instances. ep0_write() doesn't make
> the same mistake.
I have had this patch hanging around for a long time. I don't know if
it's entirely right (I don't use gadgetfs much), but see what you
think.
Alan Stern
Index: usb-3.3/drivers/usb/gadget/inode.c
===================================================================
--- usb-3.3.orig/drivers/usb/gadget/inode.c
+++ usb-3.3/drivers/usb/gadget/inode.c
@@ -998,8 +998,11 @@ ep0_read (struct file *fd, char __user *
struct usb_ep *ep = dev->gadget->ep0;
struct usb_request *req = dev->req;
- if ((retval = setup_req (ep, req, 0)) == 0)
+ if ((retval = setup_req (ep, req, 0)) == 0) {
+ spin_unlock(&dev->lock);
retval = usb_ep_queue (ep, req, GFP_ATOMIC);
+ spin_lock(&dev->lock);
+ }
dev->state = STATE_DEV_CONNECTED;
/* assume that was SET_CONFIGURATION */
@@ -1533,8 +1536,10 @@ delegate:
w_length);
if (value < 0)
break;
+ spin_unlock(&dev->lock);
value = usb_ep_queue (gadget->ep0, dev->req,
GFP_ATOMIC);
+ spin_lock(&dev->lock);
if (value < 0) {
clean_req (gadget->ep0, dev->req);
break;
@@ -1557,7 +1562,9 @@ delegate:
if (value >= 0 && dev->state != STATE_DEV_SETUP) {
req->length = value;
req->zero = value < w_length;
+ spin_unlock(&dev->lock);
value = usb_ep_queue (gadget->ep0, req, GFP_ATOMIC);
+ spin_lock(&dev->lock);
if (value < 0) {
DBG (dev, "ep_queue --> %d\n", value);
req->status = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
