> Now that I know that giveback_urb cannot be called reentrantly, I see
> there's no need to worry about double locking. That routine is
> the only place that needs to acquire handler_lock while there is a
> possibility of holding another spinlock, so the issue doesn't arise.
>
> If you feel that my approach still isn't right and want to discuss it some
> more, I'll be happy to oblige.
Hi Alan,
I take it back, there's a need for a TERMINATING state.
Your argument is right. But it's somewhat different still.
usb_hcd_giveback_urb() must be able to tell apart
between usb_unlink_urb() called in state SUBMITTED
and COMPLETING because the transitions are different.
SUBMITTED - usb_unlink_urb() -> CANCELED
COMPLETING - usb_unlink_urb() -> TERMINATING
before handler after handler new state
CANCELED * IDLE
SUBMITTED COMPLETING IDLE
SUBMITTED CANCELED CANCELED
SUBMITTED TERMINATING IDLE
This means that lock order must be handler_lock before lock.
How do you like this version?
Regards
Oliver
# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.1270 -> 1.1275
# include/linux/usb.h 1.122 -> 1.124
# drivers/usb/core/urb.c 1.19 -> 1.21
# drivers/usb/core/hcd.c 1.86 -> 1.91
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/01/14 [EMAIL PROTECTED] 1.1271
# - explicit URB state
# - improvement in unlinking semantics
# --------------------------------------------
# 03/01/14 [EMAIL PROTECTED] 1.1272
# - spinlocks in unlink notification path
# - fix state in case of resubmission
# --------------------------------------------
# 03/01/15 [EMAIL PROTECTED] 1.1273
# - remove incorrect spinlocking
# --------------------------------------------
# 03/01/15 [EMAIL PROTECTED] 1.1274
# - correct state handling in unlink case
# --------------------------------------------
# 03/01/15 [EMAIL PROTECTED] 1.1275
# - clean state transition for unlinking case
# --------------------------------------------
#
diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c Wed Jan 15 23:41:33 2003
+++ b/drivers/usb/core/hcd.c Wed Jan 15 23:41:33 2003
@@ -1138,14 +1138,24 @@
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
+ /*
+ * We decide based upon the internal_state what's to be done
*/
- if (urb->status != -EINPROGRESS) {
+ switch (urb->internal_state) {
+ case URB_SUBMITTED: /* normal case */
+ urb->internal_state = URB_CANCELED;
+ break;
+ default:
+ BUG();
+ case URB_IDLE:
+ case URB_CANCELED:
+ case URB_TERMINATING:
retval = -EBUSY;
goto done;
+ case URB_COMPLETING:
+ retval = -EBUSY;
+ urb->internal_state = URB_TERMINATING;
+ goto err_out_waiting;
}
/* maybe set up to block until the urb's completion fires. the
@@ -1205,6 +1215,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 completing routine 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)) {
+ spin_lock_irq(&urb->handler_lock);
+ spin_unlock_irq(&urb->handler_lock);
+ }
+
+ return retval;
}
/*-------------------------------------------------------------------------*/
@@ -1286,7 +1308,14 @@
*/
void usb_hcd_giveback_urb (struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs)
{
+ int old_state;
+
+ spin_lock(&urb->handler_lock);
urb_unlink (urb);
+ spin_lock(&urb->lock);
+ if ((old_state = urb->internal_state) == URB_SUBMITTED)
+ urb->internal_state = URB_COMPLETING;
+ spin_unlock(&urb->lock);
// NOTE: a generic device/urb monitoring hook would go here.
// hcd_monitor_hook(MONITOR_URB_FINISH, urb, dev)
@@ -1308,6 +1337,16 @@
/* pass ownership to the completion handler */
urb->complete (urb, regs);
+ spin_unlock(&urb->handler_lock);
+ /* we are now idle unless the urb was resubmitted */
+ spin_lock(&urb->lock);
+ if ( urb->internal_state == URB_COMPLETING
+ || old_state == URB_CANCELED
+ || (old_state == URB_SUBMITTED && urb->internal_state ==
+URB_TERMINATING)
+ )
+ urb->internal_state = URB_IDLE;
+ spin_unlock(&urb->lock);
+
usb_put_urb (urb);
}
EXPORT_SYMBOL (usb_hcd_giveback_urb);
diff -Nru a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
--- a/drivers/usb/core/urb.c Wed Jan 15 23:41:33 2003
+++ b/drivers/usb/core/urb.c Wed Jan 15 23:41:33 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,29 @@
urb->interval = temp;
}
+ spin_lock_irqsave(urb->lock, flags);
+ switch (urb->internal_state) {
+ case URB_IDLE:
+ case URB_COMPLETING:
+ urb->internal_state = URB_SUBMITTED;
+ break;
+ case URB_TERMINATING:
+ case URB_CANCELED:
+ r = -EINTR;
+ goto err_out;
+ case URB_SUBMITTED:
+ default:
+ BUG();
+ r = -EBUSY;
+ goto err_out;
+ }
+ spin_unlock_irqrestore(urb->lock, flags);
+
return op->submit_urb (urb, mem_flags);
+
+err_out:
+ spin_unlock_irqrestore(urb->lock, flags);
+ return r;
}
/*-------------------------------------------------------------------*/
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h Wed Jan 15 23:41:33 2003
+++ b/include/linux/usb.h Wed Jan 15 23:41:33 2003
@@ -728,6 +728,8 @@
{
spinlock_t lock; /* lock for the URB */
atomic_t count; /* reference count of the URB */
+ int internal_state; /* state in the URB's lifecycle */
+ 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 */
@@ -750,6 +752,12 @@
usb_complete_t complete; /* (in) completion routine */
struct usb_iso_packet_descriptor iso_frame_desc[0]; /* (in) ISO ONLY */
};
+
+#define URB_IDLE 0
+#define URB_SUBMITTED 1
+#define URB_COMPLETING 2
+#define URB_CANCELED 3
+#define URB_TERMINATING 4
/* -------------------------------------------------------------------------- */
-------------------------------------------------------
This SF.NET email is sponsored by: A Thawte Code Signing Certificate
is essential in establishing user confidence by providing assurance of
authenticity and code integrity. Download our Free Code Signing guide:
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0028en
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
