On Wed, 22 Jan 2003, Oliver Neukum wrote:
> Am Mittwoch, 22. Januar 2003 22:32 schrieb Alan Stern:
> > Here is my patch to implement proper timing of synchronous unlinks.
> > Basically it just uses a spinlock to cause unlink requests during a
> > completion handler to wait until the handler has returned before carrying
> > out the unlink. This seems like a pretty small and uninvasive sort of
> > change.
>
> But sadly, not quite right. In the code path leading to calling the
> completion handler, you are grabbing the lock too late. In that window
> unlinking can fail.
Damn -- you're right. Okay, here's another version. This one has the
following two properties:
An unlink request while the completion handler is running
will wait for the handler to finish before doing anything.
A synchronous unlink doesn't return until the urb's completion
handler has exited.
I think that does just what is needed. Unfortunately as a consequence of
this, if a completion handler tries to unlink its urb for any reason
(following a resubmit, for example), the unlink call will spin forever.
I suggest that drivers try to avoid doing this.
The new patch uses a status bit from the transfer_flags. I don't think
it's much of a problem here, because the bit is only set during the time
that the urb is owned by the usb core (i.e., set during the call to
usb_submit_urb() and reset before the call to the completion handler).
Alan Stern
===== drivers/usb/core/hcd.c 1.89 vs edited =====
--- 1.89/drivers/usb/core/hcd.c Thu Jan 16 04:29:28 2003
+++ edited/drivers/usb/core/hcd.c Thu Jan 23 16:40:12 2003
@@ -1102,6 +1102,7 @@
unsigned long flags;
struct completion_splice splice;
int retval;
+ int just_waiting = 0;
if (!urb)
return -EINVAL;
@@ -1116,8 +1117,13 @@
* lock sequence needed for the usb_hcd_giveback_urb() code paths
* (urb lock, then hcd_data_lock) in case some other CPU is now
* unlinking it.
+ *
+ * In case the completion handler is already running, we first
+ * acquire urb->handler_lock; this will cause us to wait until
+ * the handler has returned.
*/
- spin_lock_irqsave (&urb->lock, flags);
+ spin_lock_irqsave (&urb->handler_lock, flags);
+ spin_lock (&urb->lock);
spin_lock (&hcd_data_lock);
if (!urb->dev || !urb->dev->bus) {
@@ -1139,13 +1145,19 @@
}
/* Any status except -EINPROGRESS means something already started to
- * unlink this URB from the hardware. So there's no more work to do.
+ * unlink this URB from the hardware. For an async request there's
+ * no more work to do. For a synchronous request, if the completion
+ * handler has not yet run then we must still wait for it.
*
* FIXME use better explicit urb state
*/
if (urb->status != -EINPROGRESS) {
retval = -EBUSY;
- goto done;
+ if (urb->transfer_flags & URB_ASYNC_UNLINK)
+ goto done;
+ if (!(urb->transfer_flags & URB_NOT_COMPLETED))
+ goto done;
+ just_waiting = 1;
}
/* maybe set up to block until the urb's completion fires. the
@@ -1164,15 +1176,23 @@
splice.context = urb->context;
urb->complete = unlink_complete;
urb->context = &splice;
- urb->status = -ENOENT;
+ /* If the request arrived after the urb had finished anyway,
+ * don't indicate that it was unlinked -- it wasn't.
+ */
+ if (!just_waiting)
+ urb->status = -ENOENT;
} else {
/* asynchronous unlink */
urb->status = -ECONNRESET;
}
spin_unlock (&hcd_data_lock);
- spin_unlock_irqrestore (&urb->lock, flags);
+ spin_unlock (&urb->lock);
+ spin_unlock_irqrestore (&urb->handler_lock, flags);
- if (urb == (struct urb *) hcd->rh_timer.data) {
+ if (just_waiting)
+ /* no need to unlink; the urb is already finishing up */
+ ;
+ else if (urb == (struct urb *) hcd->rh_timer.data) {
usb_rh_status_dequeue (hcd, urb);
retval = 0;
} else {
@@ -1196,11 +1216,12 @@
return -EINPROGRESS;
wait_for_completion (&splice.done);
- return 0;
+ return retval;
done:
spin_unlock (&hcd_data_lock);
- spin_unlock_irqrestore (&urb->lock, flags);
+ spin_unlock (&urb->lock);
+ spin_unlock_irqrestore (&urb->handler_lock, flags);
bye:
if (retval && sys)
dev_dbg (sys, "hcd_unlink_urb %p fail %d\n", urb, retval);
@@ -1307,7 +1328,10 @@
}
/* pass ownership to the completion handler */
+ spin_lock (&urb->handler_lock);
+ urb->transfer_flags &= ~URB_NOT_COMPLETED;
urb->complete (urb, regs);
+ spin_unlock (&urb->handler_lock);
usb_put_urb (urb);
}
EXPORT_SYMBOL (usb_hcd_giveback_urb);
===== drivers/usb/core/urb.c 1.19 vs edited =====
--- 1.19/drivers/usb/core/urb.c Wed Oct 30 22:55:08 2002
+++ edited/drivers/usb/core/urb.c Thu Jan 23 16:36:13 2003
@@ -44,6 +44,7 @@
memset(urb, 0, sizeof(*urb));
urb->count = (atomic_t)ATOMIC_INIT(1);
spin_lock_init(&urb->lock);
+ spin_lock_init(&urb->handler_lock);
return urb;
}
@@ -192,9 +193,14 @@
struct usb_device *dev;
struct usb_operations *op;
int is_out;
+ int status;
if (!urb || urb->hcpriv || !urb->complete)
return -EINVAL;
+ if (urb->transfer_flags & URB_NOT_COMPLETED) {
+ BUG();
+ return -EINPROGRESS;
+ }
if (!(dev = urb->dev) || !dev->bus || dev->devnum <= 0)
return -ENODEV;
if (!(op = dev->bus->op) || !op->submit_urb)
@@ -346,7 +352,10 @@
urb->interval = temp;
}
- return op->submit_urb (urb, mem_flags);
+ status = op->submit_urb (urb, mem_flags);
+ if (!status)
+ urb->transfer_flags |= URB_NOT_COMPLETED;
+ return status;
}
/*-------------------------------------------------------------------*/
===== include/linux/usb.h 1.123 vs edited =====
--- 1.123/include/linux/usb.h Fri Jan 17 13:21:23 2003
+++ edited/include/linux/usb.h Thu Jan 23 16:35:51 2003
@@ -555,6 +555,7 @@
#define URB_ZERO_PACKET 0x0040 /* Finish bulk OUTs with short packet
*/
#define URB_NO_INTERRUPT 0x0080 /* HINT: no non-error interrupt needed */
#define URB_TIMEOUT_KILLED 0x1000 /* only set by HCD! */
+#define URB_NOT_COMPLETED 0x2000 /* only set by HCD! */
struct usb_iso_packet_descriptor {
unsigned int offset;
@@ -728,6 +729,7 @@
{
spinlock_t lock; /* lock for the URB */
atomic_t count; /* reference count of the URB */
+ spinlock_t handler_lock; /* lock during completion routine */
void *hcpriv; /* private data for host controller */
struct list_head urb_list; /* list pointer to all active urbs */
struct usb_device *dev; /* (in) pointer to associated device */
-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
