On Thursday 23 April 2009, Alan Stern wrote:
> Dave:
>
> A bug in gadgetfs has persisted for quite a while: nested spin_locks
> when used with net2280.
Did you just happen to notice this when trying the latest
PTP code? :)
> This is because inode.c calls usb_ep_queue()
> for ep0 while holding the device lock, and ep0_complete() tries to
> acquire the lock. Since net2280 will call the completion routine
> directly during submission if the request is small enough to fit
> entirely in a FIFO, we end up in deadlock. (Dummy_hcd may behave the
> same way; I haven't checked.)
Yeah, locking there has always been pretty nasty.
There were a surprising number of code paths that
could tromp on each other ... some of them may now
have vanished, since the ability to implement a
userspace gadget without ioctls has been removed.
Note that your subject is misleading. There's
only *one* spinlock ... :)
> The patch below attempts to fix the problem by dropping the lock around
> the calls to usb_ep_queue() for ep0, but I don't know whether doing
> this might lead to other problems.
The comment in the "struct ep_data" definition is that
the spinlock must be held to access ep or req.
Now, digging back into the foggier parts of my memory,
the really nasty cases there involved cleanup paths.
As in, close file descriptor ==> gadgetfs_unbind(),
unplug cable from host ==> gadgetfs_disconnect(), and
both of those could happen concurrently with respect
to request completion or submission... yep, three
concurrent nastinesses to defend against. Luckily
it was easy enough to test each singly, and along
with any one of the others.
A better solution than that spinlock may be needed.
Maybe a parallel state machine.
> I suppose the places inside
> gadgetfs_setup() are probably okay, but what about the call inside
> ep0_read()? There's a possibility that the host might send a new
> SETUP packet while waiting for the userspace program to get around to
> reading the OUT data from the previous transfer.
Yeah, ep0 in particular has additional nasty cases that
none of the other endpoints had. Like those.
> Can you look this over and figure out what more (if anything) needs to
> be done?
It'll hurt, but ... I'll try.
>
> Thanks,
>
> Alan Stern
>
>
>
> Index: usb-2.6/drivers/usb/gadget/inode.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/gadget/inode.c
> +++ usb-2.6/drivers/usb/gadget/inode.c
> @@ -1000,8 +1000,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 */
> @@ -1535,8 +1538,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;
> @@ -1559,7 +1564,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
