From: Greg KH <greg@kroah.com>
To: marcelo@conectiva.com.br
Cc: linux-usb-devel@lists.sourceforge.net
Subject: [PATCH 6 of 6] USB uhci driver update

Hi,

Here's a patch against 2.4.19-pre1 that updates the USB uhci driver.
The patch is from Johannes Erdfelt, and does the following:
	- fixes a toggle bug
	- fixes a compiler warning
	- fixes problem where interrupt URB is unlinked in the driver's
	  completion handler.
	- fix bug when unmapping the PCI DMA mapping and then sync the
	  data afterwards.
	- documented the locking strategy
	- cleaned up the locking strategy

thanks,

greg k-h


diff -Nru a/drivers/usb/uhci.c b/drivers/usb/uhci.c
--- a/drivers/usb/uhci.c	Mon Feb 25 16:54:34 2002
+++ b/drivers/usb/uhci.c	Mon Feb 25 16:54:34 2002
@@ -4,7 +4,7 @@
  * Maintainer: Johannes Erdfelt <johannes@erdfelt.com>
  *
  * (C) Copyright 1999 Linus Torvalds
- * (C) Copyright 1999-2001 Johannes Erdfelt, johannes@erdfelt.com
+ * (C) Copyright 1999-2002 Johannes Erdfelt, johannes@erdfelt.com
  * (C) Copyright 1999 Randy Dunlap
  * (C) Copyright 1999 Georg Acher, acher@in.tum.de
  * (C) Copyright 1999 Deti Fliegl, deti@fliegl.de
@@ -240,10 +240,10 @@
 	unsigned long flags;
 
 	/* If it's not inserted, don't remove it */
+	spin_lock_irqsave(&uhci->frame_list_lock, flags);
 	if (td->frame == -1 && list_empty(&td->fl_list))
-		return;
+		goto out;
 
-	spin_lock_irqsave(&uhci->frame_list_lock, flags);
 	if (td->frame != -1 && uhci->fl->frame_cpu[td->frame] == td) {
 		if (list_empty(&td->fl_list)) {
 			uhci->fl->frame[td->frame] = td->link;
@@ -268,6 +268,7 @@
 	list_del_init(&td->fl_list);
 	td->frame = -1;
 
+out:
 	spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
 }
 
@@ -358,6 +359,9 @@
 	pci_pool_free(uhci->qh_pool, qh, qh->dma_handle);
 }
 
+/*
+ * MUST be called with uhci->frame_list_lock acquired
+ */
 static void _uhci_insert_qh(struct uhci *uhci, struct uhci_qh *skelqh, struct urb *urb)
 {
 	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
@@ -417,11 +421,10 @@
 		return;
 
 	/* Only go through the hoops if it's actually linked in */
+	spin_lock_irqsave(&uhci->frame_list_lock, flags);
 	if (!list_empty(&qh->list)) {
 		qh->urbp = NULL;
 
-		spin_lock_irqsave(&uhci->frame_list_lock, flags);
-
 		pqh = list_entry(qh->list.prev, struct uhci_qh, list);
 
 		if (pqh->urbp) {
@@ -444,9 +447,8 @@
 		qh->element = qh->link = UHCI_PTR_TERM;
 
 		list_del_init(&qh->list);
-
-		spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
 	}
+	spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
 
 	spin_lock_irqsave(&uhci->qh_remove_list_lock, flags);
 
@@ -658,6 +660,9 @@
 	return urbp;
 }
 
+/*
+ * MUST be called with urb->lock acquired
+ */
 static void uhci_add_td_to_urb(struct urb *urb, struct uhci_td *td)
 {
 	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
@@ -667,6 +672,9 @@
 	list_add_tail(&td->list, &urbp->td_list);
 }
 
+/*
+ * MUST be called with urb->lock acquired
+ */
 static void uhci_remove_td_from_urb(struct uhci_td *td)
 {
 	if (list_empty(&td->list))
@@ -677,22 +685,22 @@
 	td->urb = NULL;
 }
 
+/*
+ * MUST be called with urb->lock acquired
+ */
 static void uhci_destroy_urb_priv(struct urb *urb)
 {
 	struct list_head *head, *tmp;
 	struct urb_priv *urbp;
 	struct uhci *uhci;
-	unsigned long flags;
-
-	spin_lock_irqsave(&urb->lock, flags);
 
 	urbp = (struct urb_priv *)urb->hcpriv;
 	if (!urbp)
-		goto out;
+		return;
 
 	if (!urbp->dev || !urbp->dev->bus || !urbp->dev->bus->hcpriv) {
 		warn("uhci_destroy_urb_priv: urb %p belongs to disconnected device or bus?", urb);
-		goto out;
+		return;
 	}
 
 	if (!list_empty(&urb->urb_list))
@@ -715,20 +723,21 @@
 		uhci_free_td(uhci, td);
 	}
 
-	if (urbp->setup_packet_dma_handle)
+	if (urbp->setup_packet_dma_handle) {
 		pci_unmap_single(uhci->dev, urbp->setup_packet_dma_handle,
 			sizeof(devrequest), PCI_DMA_TODEVICE);
+		urbp->setup_packet_dma_handle = 0;
+	}
 
-	if (urbp->transfer_buffer_dma_handle)
+	if (urbp->transfer_buffer_dma_handle) {
 		pci_unmap_single(uhci->dev, urbp->transfer_buffer_dma_handle,
 			urb->transfer_buffer_length, usb_pipein(urb->pipe) ?
 			PCI_DMA_FROMDEVICE : PCI_DMA_TODEVICE);
+		urbp->transfer_buffer_dma_handle = 0;
+	}
 
 	urb->hcpriv = NULL;
 	kmem_cache_free(uhci_up_cachep, urbp);
-
-out:
-	spin_unlock_irqrestore(&urb->lock, flags);
 }
 
 static void uhci_inc_fsbr(struct uhci *uhci, struct urb *urb)
@@ -870,7 +879,7 @@
 	 * It's IN if the pipe is an output pipe or we're not expecting
 	 * data back.
 	 */
-	destination &= ~TD_PID;
+	destination &= ~TD_TOKEN_PID_MASK;
 	if (usb_pipeout(urb->pipe) || !urb->transfer_buffer_length)
 		destination |= USB_PID_IN;
 	else
@@ -1308,9 +1317,7 @@
 	struct uhci *uhci = (struct uhci *)urb->dev->bus->hcpriv;
 	struct list_head *tmp, *head;
 	int ret = 0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&uhci->urb_list_lock, flags);
 	head = &uhci->urb_list;
 	tmp = head->next;
 	while (tmp != head) {
@@ -1333,8 +1340,6 @@
 	} else
 		ret = -1;	/* no previous urb found */
 
-	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
-
 	return ret;
 }
 
@@ -1442,34 +1447,30 @@
 	return ret;
 }
 
+/*
+ * MUST be called with uhci->urb_list_lock acquired
+ */
 static struct urb *uhci_find_urb_ep(struct uhci *uhci, struct urb *urb)
 {
 	struct list_head *tmp, *head;
-	unsigned long flags;
-	struct urb *u = NULL;
 
 	/* We don't match Isoc transfers since they are special */
 	if (usb_pipeisoc(urb->pipe))
 		return NULL;
 
-	spin_lock_irqsave(&uhci->urb_list_lock, flags);
 	head = &uhci->urb_list;
 	tmp = head->next;
 	while (tmp != head) {
-		u = list_entry(tmp, struct urb, urb_list);
+		struct urb *u = list_entry(tmp, struct urb, urb_list);
 
 		tmp = tmp->next;
 
 		if (u->dev == urb->dev && u->pipe == urb->pipe &&
 		    u->status == -EINPROGRESS)
-			goto out;
+			return u;
 	}
-	u = NULL;
-
-out:
-	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 
-	return u;
+	return NULL;
 }
 
 static int uhci_submit_urb(struct urb *urb)
@@ -1493,13 +1494,15 @@
 	INIT_LIST_HEAD(&urb->urb_list);
 	usb_inc_dev_use(urb->dev);
 
-	spin_lock_irqsave(&urb->lock, flags);
+	spin_lock_irqsave(&uhci->urb_list_lock, flags);
+	spin_lock(&urb->lock);
 
 	if (urb->status == -EINPROGRESS || urb->status == -ECONNRESET ||
 	    urb->status == -ECONNABORTED) {
 		dbg("uhci_submit_urb: urb not available to submit (status = %d)", urb->status);
 		/* Since we can have problems on the out path */
-		spin_unlock_irqrestore(&urb->lock, flags);
+		spin_unlock(&urb->lock);
+		spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 		usb_dec_dev_use(urb->dev);
 
 		return ret;
@@ -1568,18 +1571,21 @@
 out:
 	urb->status = ret;
 
-	spin_unlock_irqrestore(&urb->lock, flags);
-
 	if (ret == -EINPROGRESS) {
-		spin_lock_irqsave(&uhci->urb_list_lock, flags);
 		/* We use _tail to make find_urb_ep more efficient */
 		list_add_tail(&urb->urb_list, &uhci->urb_list);
+
+		spin_unlock(&urb->lock);
 		spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 
 		return 0;
 	}
 
 	uhci_unlink_generic(uhci, urb);
+
+	spin_unlock(&urb->lock);
+	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
+
 	uhci_call_completion(urb);
 
 	return ret;
@@ -1588,7 +1594,7 @@
 /*
  * Return the result of a transfer
  *
- * Must be called with urb_list_lock acquired
+ * MUST be called with urb_list_lock acquired
  */
 static void uhci_transfer_result(struct uhci *uhci, struct urb *urb)
 {
@@ -1627,10 +1633,10 @@
 
 	urbp->status = ret;
 
-	spin_unlock_irqrestore(&urb->lock, flags);
-
-	if (ret == -EINPROGRESS)
+	if (ret == -EINPROGRESS) {
+		spin_unlock_irqrestore(&urb->lock, flags);
 		return;
+	}
 
 	switch (usb_pipetype(urb->pipe)) {
 	case PIPE_CONTROL:
@@ -1660,15 +1666,22 @@
 			usb_pipetype(urb->pipe), urb);
 	}
 
+	/* Remove it from uhci->urb_list */
 	list_del_init(&urb->urb_list);
 
 	uhci_add_complete(urb);
+
+	spin_unlock_irqrestore(&urb->lock, flags);
 }
 
+/*
+ * MUST be called with urb->lock acquired
+ */
 static void uhci_unlink_generic(struct uhci *uhci, struct urb *urb)
 {
 	struct list_head *head, *tmp;
 	struct urb_priv *urbp = urb->hcpriv;
+	int prevactive = 1;
 
 	/* We can get called when urbp allocation fails, so check */
 	if (!urbp)
@@ -1676,6 +1689,19 @@
 
 	uhci_dec_fsbr(uhci, urb);	/* Safe since it checks */
 
+	/*
+	 * Now we need to find out what the last successful toggle was
+	 * so we can update the local data toggle for the next transfer
+	 *
+	 * There's 3 way's the last successful completed TD is found:
+	 *
+	 * 1) The TD is NOT active and the actual length < expected length
+	 * 2) The TD is NOT active and it's the last TD in the chain
+	 * 3) The TD is active and the previous TD is NOT active
+	 *
+	 * Control and Isochronous ignore the toggle, so this is safe
+	 * for all types
+	 */
 	head = &urbp->td_list;
 	tmp = head->next;
 	while (tmp != head) {
@@ -1683,15 +1709,18 @@
 
 		tmp = tmp->next;
 
-		/* Control and Isochronous ignore the toggle, so this */
-		/* is safe for all types */
-		if ((!(td->status & TD_CTRL_ACTIVE) &&
-		    (uhci_actual_length(td->status) < uhci_expected_length(td->info)) ||
-		    tmp == head)) {
+		if (!(td->status & TD_CTRL_ACTIVE) &&
+		    (uhci_actual_length(td->status) < uhci_expected_length(td->info) ||
+		    tmp == head))
 			usb_settoggle(urb->dev, uhci_endpoint(td->info),
 				uhci_packetout(td->info),
 				uhci_toggle(td->info) ^ 1);
-		}
+		else if ((td->status & TD_CTRL_ACTIVE) && !prevactive)
+			usb_settoggle(urb->dev, uhci_endpoint(td->info),
+				uhci_packetout(td->info),
+				uhci_toggle(td->info));
+
+		prevactive = td->status & TD_CTRL_ACTIVE;
 	}
 
 	uhci_delete_queued_urb(uhci, urb);
@@ -1714,6 +1743,9 @@
 
 	uhci = (struct uhci *)urb->dev->bus->hcpriv;
 
+	spin_lock_irqsave(&uhci->urb_list_lock, flags);
+	spin_lock(&urb->lock);
+
 	/* Release bandwidth for Interrupt or Isoc. transfers */
 	/* Spinlock needed ? */
 	if (urb->bandwidth) {
@@ -1729,35 +1761,41 @@
 		}
 	}
 
-	if (urb->status != -EINPROGRESS)
+	if (urb->status != -EINPROGRESS) {
+		spin_unlock(&urb->lock);
+		spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 		return 0;
+	}
 
-	spin_lock_irqsave(&uhci->urb_list_lock, flags);
 	list_del_init(&urb->urb_list);
-	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 
 	uhci_unlink_generic(uhci, urb);
 
 	/* Short circuit the virtual root hub */
 	if (urb->dev == uhci->rh.dev) {
 		rh_unlink_urb(urb);
+
+		spin_unlock(&urb->lock);
+		spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
+
 		uhci_call_completion(urb);
 	} else {
 		if (urb->transfer_flags & USB_ASYNC_UNLINK) {
-			/* urb_list is available now since we called */
-			/*  uhci_unlink_generic already */
-
 			urbp->status = urb->status = -ECONNABORTED;
 
-			spin_lock_irqsave(&uhci->urb_remove_list_lock, flags);
+			spin_lock(&uhci->urb_remove_list_lock);
 
-			/* Check to see if the remove list is empty */
+			/* If we're the first, set the next interrupt bit */
 			if (list_empty(&uhci->urb_remove_list))
 				uhci_set_next_interrupt(uhci);
 			
 			list_add(&urb->urb_list, &uhci->urb_remove_list);
 
-			spin_unlock_irqrestore(&uhci->urb_remove_list_lock, flags);
+			spin_unlock(&uhci->urb_remove_list_lock);
+
+			spin_unlock(&urb->lock);
+			spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
+
 		} else {
 			urb->status = -ENOENT;
 
@@ -1770,6 +1808,9 @@
 			} else
 				schedule_timeout(1+1*HZ/1000); 
 
+			spin_unlock(&urb->lock);
+			spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
+
 			uhci_call_completion(urb);
 		}
 	}
@@ -1903,12 +1944,14 @@
 /* prepare Interrupt pipe transaction data; HUB INTERRUPT ENDPOINT */
 static int rh_send_irq(struct urb *urb)
 {
-	int i, len = 1;
 	struct uhci *uhci = (struct uhci *)urb->dev->bus->hcpriv;
+	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
 	unsigned int io_addr = uhci->io_addr;
+	unsigned long flags;
+	int i, len = 1;
 	__u16 data = 0;
-	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
 
+	spin_lock_irqsave(&urb->lock, flags);
 	for (i = 0; i < uhci->rh.numports; i++) {
 		data |= ((inw(io_addr + USBPORTSC1 + i * 2) & 0xa) > 0 ? (1 << (i + 1)) : 0);
 		len = (i + 1) / 8 + 1;
@@ -1918,6 +1961,8 @@
 	urb->actual_length = len;
 	urbp->status = 0;
 
+	spin_unlock_irqrestore(&urb->lock, flags);
+
 	if ((data > 0) && (uhci->rh.send != 0)) {
 		dbg("root-hub INT complete: port1: %x port2: %x data: %x",
 			inw(io_addr + USBPORTSC1), inw(io_addr + USBPORTSC2), data);
@@ -1944,7 +1989,6 @@
 
 	spin_lock_irqsave(&uhci->urb_list_lock, flags);
 	head = &uhci->urb_list;
-
 	tmp = head->next;
 	while (tmp != head) {
 		struct urb *u = list_entry(tmp, struct urb, urb_list);
@@ -1952,6 +1996,8 @@
 
 		tmp = tmp->next;
 
+		spin_lock(&urb->lock);
+
 		/* Check if the FSBR timed out */
 		if (urbp->fsbr && !urbp->fsbr_timeout && time_after_eq(jiffies, urbp->fsbrtime + IDLE_TIMEOUT))
 			uhci_fsbr_timeout(uhci, u);
@@ -1961,6 +2007,8 @@
 			list_del(&u->urb_list);
 			list_add_tail(&u->urb_list, &list);
 		}
+
+		spin_unlock(&urb->lock);
 	}
 	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 
@@ -2194,6 +2242,9 @@
 	return stat;
 }
 
+/*
+ * MUST be called with urb->lock acquired
+ */
 static int rh_unlink_urb(struct urb *urb)
 {
 	struct uhci *uhci = (struct uhci *)urb->dev->bus->hcpriv;
@@ -2229,16 +2280,25 @@
 
 static void uhci_call_completion(struct urb *urb)
 {
-	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
+	struct urb_priv *urbp;
 	struct usb_device *dev = urb->dev;
 	struct uhci *uhci = (struct uhci *)dev->bus->hcpriv;
 	int is_ring = 0, killed, resubmit_interrupt, status;
 	struct urb *nurb;
+	unsigned long flags;
+
+	spin_lock_irqsave(&urb->lock, flags);
+
+	urbp = (struct urb_priv *)urb->hcpriv;
+	if (!urbp || !urb->dev) {
+		spin_unlock_irqrestore(&urb->lock, flags);
+		return;
+	}
 
 	killed = (urb->status == -ENOENT || urb->status == -ECONNABORTED ||
 			urb->status == -ECONNRESET);
 	resubmit_interrupt = (usb_pipetype(urb->pipe) == PIPE_INTERRUPT &&
-			urb->interval && !killed);
+			urb->interval);
 
 	nurb = urb->next;
 	if (nurb && !killed) {
@@ -2263,14 +2323,6 @@
 		is_ring = (nurb == urb);
 	}
 
-	status = urbp->status;
-	if (!resubmit_interrupt)
-		/* We don't need urb_priv anymore */
-		uhci_destroy_urb_priv(urb);
-
-	if (!killed)
-		urb->status = status;
-
 	if (urbp->transfer_buffer_dma_handle)
 		pci_dma_sync_single(uhci->dev, urbp->transfer_buffer_dma_handle,
 			urb->transfer_buffer_length, usb_pipein(urb->pipe) ?
@@ -2280,11 +2332,28 @@
 		pci_dma_sync_single(uhci->dev, urbp->setup_packet_dma_handle,
 			sizeof(devrequest), PCI_DMA_TODEVICE);
 
+	status = urbp->status;
+	if (!resubmit_interrupt || killed)
+		/* We don't need urb_priv anymore */
+		uhci_destroy_urb_priv(urb);
+
+	if (!killed)
+		urb->status = status;
+
 	urb->dev = NULL;
-	if (urb->complete)
+	spin_unlock_irqrestore(&urb->lock, flags);
+
+	if (urb->complete) {
 		urb->complete(urb);
 
-	if (resubmit_interrupt) {
+		/* Recheck the status. The completion handler may have */
+		/*  unlinked the resubmitting interrupt URB */
+		killed = (urb->status == -ENOENT ||
+			  urb->status == -ECONNABORTED ||
+			  urb->status == -ECONNRESET);
+	}
+
+	if (resubmit_interrupt && !killed) {
 		urb->dev = dev;
 		uhci_reset_interrupt(urb);
 	} else {
@@ -2311,11 +2380,14 @@
 		struct urb_priv *urbp = list_entry(tmp, struct urb_priv, complete_list);
 		struct urb *urb = urbp->urb;
 
-		tmp = tmp->next;
-
 		list_del_init(&urbp->complete_list);
+		spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
 
 		uhci_call_completion(urb);
+
+		spin_lock_irqsave(&uhci->complete_list_lock, flags);
+		head = &uhci->complete_list;
+		tmp = head->next;
 	}
 	spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
 }
@@ -2337,7 +2409,8 @@
 		list_del_init(&urb->urb_list);
 
 		urbp->status = urb->status = -ECONNRESET;
-		uhci_call_completion(urb);
+
+		uhci_add_complete(urb);
 	}
 	spin_unlock_irqrestore(&uhci->urb_remove_list_lock, flags);
 }
@@ -3070,3 +3143,4 @@
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
+
diff -Nru a/drivers/usb/uhci.h b/drivers/usb/uhci.h
--- a/drivers/usb/uhci.h	Mon Feb 25 16:54:33 2002
+++ b/drivers/usb/uhci.h	Mon Feb 25 16:54:33 2002
@@ -121,15 +121,16 @@
  * 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 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_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 @@
 	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,11 +350,36 @@
 	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