uhci.c didn't work well with USB storage. It would tend to stall

relatively quickly and sometimes locked up the system. It usually only
took me a couple of tries ripping a CD to reproduce the problem.

I took a long hard look at the locking in uhci.c and decided to clean
it up, fixing a couple of bugs along the way as well as documenting the
locking strategy.

With this patch applies, where I could only rip a CD a couple of times
before causing problems, I was able to rip a CD 12,000 times in a row
successfully, before I stopped it. Not a single error :)

drivers/usb/uhci.h

@@ -121,15 +121,16 @@ struct uhci_qh {
  * for TD <info>: (a.k.a. Token)
  */
 #define TD_TOKEN_TOGGLE		19
-#define TD_PID			0xFF
+#define TD_TOKEN_PID_MASK	0xFF
+#define TD_TOKEN_EXPLEN_MASK	0x7FF		/* expected length, encoded as n - 1 */
 
 #define uhci_maxlen(token)	((token) >> 21)
-#define uhci_expected_length(info) (((info >> 21) + 1) & TD_CTRL_ACTLEN_MASK) /* 1-based */
+#define uhci_expected_length(info) (((info >> 21) + 1) & TD_TOKEN_EXPLEN_MASK) /* 1-based */
 #define uhci_toggle(token)	(((token) >> TD_TOKEN_TOGGLE) & 1)
 #define uhci_endpoint(token)	(((token) >> 15) & 0xf)
 #define uhci_devaddr(token)	(((token) >> 8) & 0x7f)
 #define uhci_devep(token)	(((token) >> 8) & 0x7ff)
-#define uhci_packetid(token)	((token) & 0xff)
+#define uhci_packetid(token)	((token) & TD_TOKEN_PID_MASK)
 #define uhci_packetout(token)	(uhci_packetid(token) != USB_PID_IN)
 #define uhci_packetin(token)	(uhci_packetid(token) == USB_PID_IN)
 
@@ -163,7 +164,7 @@ struct uhci_td {
 	struct list_head list;		/* P: urb->lock */
 
 	int frame;
-	struct list_head fl_list;	/* P: frame_list_lock */
+	struct list_head fl_list;	/* P: uhci->frame_list_lock */
 } __attribute__((aligned(16)));
 
 /*
@@ -306,21 +307,25 @@ struct uhci {
 	struct uhci_qh *skelqh[UHCI_NUM_SKELQH];	/* Skeleton QH's */
 
 	spinlock_t frame_list_lock;
-	struct uhci_frame_list *fl;		/* Frame list */
+	struct uhci_frame_list *fl;		/* P: uhci->frame_list_lock */
 	int fsbr;				/* Full speed bandwidth reclamation */
 	int is_suspended;
 
+	/* Main list of URB's currently controlled by this HC */
+	spinlock_t urb_list_lock;
+	struct list_head urb_list;		/* P: uhci->urb_list_lock */
+
+	/* List of QH's that are done, but waiting to be unlinked (race) */
 	spinlock_t qh_remove_list_lock;
-	struct list_head qh_remove_list;
+	struct list_head qh_remove_list;	/* P: uhci->qh_remove_list_lock */
 
+	/* List of asynchronously unlinked URB's */
 	spinlock_t urb_remove_list_lock;
-	struct list_head urb_remove_list;
-
-	spinlock_t urb_list_lock;
-	struct list_head urb_list;
+	struct list_head urb_remove_list;	/* P: uhci->urb_remove_list_lock */
 
+	/* List of URB's awaiting completion callback */
 	spinlock_t complete_list_lock;
-	struct list_head complete_list;
+	struct list_head complete_list;		/* P: uhci->complete_list_lock */
 
 	struct virt_root_hub rh;	/* private data of the virtual root hub */
 };
@@ -333,7 +338,7 @@ struct urb_priv {
 	dma_addr_t transfer_buffer_dma_handle;
 
 	struct uhci_qh *qh;		/* QH for this URB */
-	struct list_head td_list;
+	struct list_head td_list;	/* P: urb->lock */
 
 	int fsbr : 1;			/* URB turned on FSBR */
 	int fsbr_timeout : 1;		/* URB timed out on FSBR */
@@ -345,12 +350,37 @@ struct urb_priv {
 	int status;			/* Final status */
 
 	unsigned long inserttime;	/* In jiffies */
-	unsigned long fsbrtime;	/* In jiffies */
+	unsigned long fsbrtime;		/* In jiffies */
 
-	struct list_head queue_list;
-	struct list_head complete_list;
+	struct list_head queue_list;	/* P: uhci->frame_list_lock */
+	struct list_head complete_list;	/* P: uhci->complete_list_lock */
 };
 
+/*
+ * Locking in uhci.c
+ *
+ * spinlocks are used extensively to protect the many lists and data
+ * structures we have. It's not that pretty, but it's necessary. We
+ * need to be done with all of the locks (except complete_list_lock) when
+ * we call urb->complete. I've tried to make it simple enough so I don't
+ * have to spend hours racking my brain trying to figure out if the
+ * locking is safe.
+ *
+ * Here's the safe locking order to prevent deadlocks:
+ *
+ * #1 uhci->urb_list_lock
+ * #2 urb->lock
+ * #3 uhci->urb_remove_list_lock, uhci->frame_list_lock, 
+ *   uhci->qh_remove_list_lock
+ * #4 uhci->complete_list_lock
+ *
+ * If you're going to grab 2 or more locks at once, ALWAYS grab the lock
+ * at the lowest level FIRST and NEVER grab locks at the same level at the
+ * same time.
+ * 
+ * So, if you need uhci->urb_list_lock, grab it before you grab urb->lock
+ */
+
 /* -------------------------------------------------------------------------
    Virtual Root HUB
    ------------------------------------------------------------------------- */