Oliver et al.:
In response to one of your previous messages, I tried reducing the scope
of the changes required for properly tracking urb states and synchronous
unlinking. It turns out that in addition to the extra spinlock, I needed
2 bits: one to indicate when the urb is in use (owned by the core) and
one to indicate that it has been unlinked.
Just for kicks I created a patch (included below) that allocates those
bits from urb->transfer_flags. That's not such a great place, because
it's liable to be overwritten by device drivers. But the only alternative
is to add a whole extra field (like internal_state).
Functionally this does pretty much what we have been discussing. Overall
the changes are pretty minimal, just what is necessary to make things
work.
Anyway, tell me how you like the patch.
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 Tue Jan 21 12:12:47 2003
@@ -1138,14 +1138,23 @@
goto done;
}
- /* Any status except -EINPROGRESS means something already started to
- * unlink this URB from the hardware. So there's no more work to do.
- *
- * FIXME use better explicit urb state
+ /*
+ * If the urb has already been unlinked, return an error code.
+ * If the urb is in use, proceed with a normal unlink.
+ * If the completion handler is running, mark the urb as unlinked.
+ * Otherwise, the urb is idle so return an error code.
*/
- if (urb->status != -EINPROGRESS) {
+ if (urb->transfer_flags & URB_UNLINKED) {
retval = -EBUSY;
goto done;
+ } else if (urb->transfer_flags & URB_IN_USE) {
+ urb->transfer_flags |= URB_UNLINKED;
+ } else if (spin_is_locked(&urb->handler_lock)) {
+ urb->transfer_flags |= URB_UNLINKED;
+ goto err_out_waiting;
+ } else {
+ retval = -ENOMSG;
+ goto done;
}
/* maybe set up to block until the urb's completion fires. the
@@ -1196,6 +1205,8 @@
return -EINPROGRESS;
wait_for_completion (&splice.done);
+ spin_lock_irq (&urb->handler_lock);
+ spin_unlock_irq (&urb->handler_lock);
return 0;
done:
@@ -1205,6 +1216,18 @@
if (retval && sys)
dev_dbg (sys, "hcd_unlink_urb %p fail %d\n", urb, retval);
return retval;
+
+err_out_waiting:
+ /* We are asked to unlink while the completion handler is already
+ running. We drop the locks and spin in case of synchronous unlink. */
+ spin_unlock (&hcd_data_lock);
+ spin_unlock_irqrestore (&urb->lock, flags);
+ if (urb->transfer_flags & URB_ASYNC_UNLINK))
+ return -EINPROGRESS;
+
+ spin_lock_irq (&urb->handler_lock);
+ spin_unlock_irq (&urb->handler_lock);
+ return 0;
}
/*-------------------------------------------------------------------------*/
@@ -1287,6 +1310,10 @@
void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs)
{
urb_unlink (urb);
+ spin_lock (&urb->handler_lock);
+ spin_lock (&urb->lock);
+ urb->transfer_flags &= ~URB_IN_USE;
+ spin_unlock (&urb->lock);
// NOTE: a generic device/urb monitoring hook would go here.
// hcd_monitor_hook(MONITOR_URB_FINISH, urb, dev)
@@ -1308,6 +1335,13 @@
/* pass ownership to the completion handler */
urb->complete (urb, regs);
+ /* we are now idle unless the urb was resubmitted */
+ spin_lock (&urb->lock);
+ if (!(urb->transfer_flags & URB_IN_USE))
+ urb->transfer_flags &= ~URB_UNLINKED;
+ spin_unlock (&urb->lock);
+ 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 Tue Jan 21 12:15:52 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,6 +193,8 @@
struct usb_device *dev;
struct usb_operations *op;
int is_out;
+ int r;
+ unsigned long flags;
if (!urb || urb->hcpriv || !urb->complete)
return -EINVAL;
@@ -346,7 +349,28 @@
urb->interval = temp;
}
+ /*
+ * Fail if the urb is already in use, or if it was unlinked and
+ * therefore may not be resubmitted.
+ */
+ spin_lock_irqsave (&urb->lock, flags);
+ if (urb->transfer_flags & URB_IN_USE) {
+ BUG();
+ r = -EBUSY;
+ goto err_out;
+ }
+ if (urb->transfer_flags & URB_UNLINKED) {
+ r = -EINTR;
+ goto err_out;
+ }
+ urb->transfer_flags |= URB_IN_USE;
+ spin_unlock_irqrestore (&urb->lock, flags);
+
return op->submit_urb (urb, mem_flags);
+
+err_out:
+ spin_unlock_irqrestore (&urb->lock, flags);
+ return r;
}
/*-------------------------------------------------------------------*/
@@ -380,6 +404,26 @@
return urb->dev->bus->op->unlink_urb(urb);
else
return -ENODEV;
+}
+
+/**
+ * usb_allow_resubmission - permit an unlinked urb to be resubmitted
+ * @urb: pointer to urb describing a previously unlinked request
+ *
+ * This routine should only be called by a completion handler that needs
+ * to resubmit an unlinked urb. Normally such a resubmission is not
+ * permitted; this function "unlocks" the urb so that it can be resubmitted.
+ */
+void usb_allow_resubmission(struct urb *urb)
+{
+ unsigned long flags;
+
+ if (urb) {
+ spin_lock_irqsave (&urb->lock, flags);
+ if (!(urb->transfer_flags & URB_IN_USE))
+ urb->transfer_flags &= ~URB_UNLINKED;
+ spin_unlock_irqrestore (&urb->lock, flags);
+ }
}
// asynchronous request completion model
===== 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 Tue Jan 21 11:30:23 2003
@@ -554,7 +554,10 @@
#define URB_NO_FSBR 0x0020 /* UHCI-specific */
#define URB_ZERO_PACKET 0x0040 /* Finish bulk OUTs with short packet
*/
#define URB_NO_INTERRUPT 0x0080 /* HINT: no non-error interrupt needed */
+ // The next several flags are for internal use only
#define URB_TIMEOUT_KILLED 0x1000 /* only set by HCD! */
+#define URB_IN_USE 0x2000 /* urb is owned by usb core */
+#define URB_UNLINKED 0x4000 /* urb was unlinked, can't resubmit */
struct usb_iso_packet_descriptor {
unsigned int offset;
@@ -627,7 +630,8 @@
* request-specific driver context.
* @complete: Completion handler. This URB is passed as the parameter to the
* completion function. The completion function may then do what
- * it likes with the URB, including resubmitting or freeing it.
+ * it likes with the URB, including resubmitting it. However, it may
+ * not free the URB.
* @iso_frame_desc: Used to provide arrays of ISO transfer buffers and to
* collect the transfer status for each buffer.
*
@@ -723,11 +727,16 @@
* of the iso_frame_desc array, and the number of errors is reported in
* error_count. Completion callbacks for ISO transfers will normally
* (re)submit URBs to ensure a constant transfer rate.
+ *
+ * Note that if an URB is canceled by usb_unlink_urb(), any attempt at
+ * resubmission by the completion handler will fail. If the handler truly
+ * needs to resubmit the URB, it must first call usb_allow_resubmission().
*/
struct urb
{
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 */
@@ -863,6 +872,7 @@
extern struct urb *usb_get_urb(struct urb *urb);
extern int usb_submit_urb(struct urb *urb, int mem_flags);
extern int usb_unlink_urb(struct urb *urb);
+extern void usb_allow_resubmission(struct urb *urb);
#define HAVE_USB_BUFFERS
void *usb_buffer_alloc (struct usb_device *dev, size_t size,
-------------------------------------------------------
This SF.net email is sponsored by: Scholarships for Techies!
Can't afford IT training? All 2003 ictp students receive scholarships.
Get hands-on training in Microsoft, Cisco, Sun, Linux/UNIX, and more.
www.ictp.com/training/sourceforge.asp
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
