[PATCH] USB: UHCI: Get rid of excessive spinlocks

This patch introduces a major simplification into the UHCI driver by
replacing its multiple spinlocks with a single one.  The protected area of
code is slightly larger and there's more possibilities for contention on
an SMP system, but I think that shouldn't be a problem.  Stephen Hemminger
has been kind enough to test this on his SMP computer and he hasn't
encountered any difficulties.

drivers/usb/host/uhci-debug.c

@@ -27,7 +27,7 @@ static inline void lprintk(char *buf)
 		p = strchr(buf, '\n');
 		if (p)
 			*p = 0;
-		printk("%s\n", buf);
+		printk(KERN_DEBUG "%s\n", buf);
 		buf = p;
 		if (buf)
 			buf++;
@@ -328,21 +328,17 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct urb_priv *urbp, char *bu
 	//out += sprintf(out, "Inserttime=%lx ",urbp->inserttime);
 	//out += sprintf(out, "FSBRtime=%lx ",urbp->fsbrtime);
 
-	spin_lock(&urbp->urb->lock);
 	count = 0;
 	list_for_each(tmp, &urbp->td_list)
 		count++;
-	spin_unlock(&urbp->urb->lock);
 	out += sprintf(out, "TDs=%d ",count);
 
 	if (urbp->queued)
 		out += sprintf(out, "queued\n");
 	else {
-		spin_lock(&uhci->frame_list_lock);
 		count = 0;
 		list_for_each(tmp, &urbp->queue_list)
 			count++;
-		spin_unlock(&uhci->frame_list_lock);
 		out += sprintf(out, "queued URBs=%d\n", count);
 	}
 
@@ -352,12 +348,10 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct urb_priv *urbp, char *bu
 static int uhci_show_lists(struct uhci_hcd *uhci, char *buf, int len)
 {
 	char *out = buf;
-	unsigned long flags;
 	struct list_head *head, *tmp;
 	int count;
 
 	out += sprintf(out, "Main list URBs:");
-	spin_lock_irqsave(&uhci->urb_list_lock, flags);
 	if (list_empty(&uhci->urb_list))
 		out += sprintf(out, " Empty\n");
 	else {
@@ -373,10 +367,8 @@ static int uhci_show_lists(struct uhci_hcd *uhci, char *buf, int len)
 			tmp = tmp->next;
 		}
 	}
-	spin_unlock_irqrestore(&uhci->urb_list_lock, flags);
 
 	out += sprintf(out, "Remove list URBs:");
-	spin_lock_irqsave(&uhci->urb_remove_list_lock, flags);
 	if (list_empty(&uhci->urb_remove_list))
 		out += sprintf(out, " Empty\n");
 	else {
@@ -392,10 +384,8 @@ static int uhci_show_lists(struct uhci_hcd *uhci, char *buf, int len)
 			tmp = tmp->next;
 		}
 	}
-	spin_unlock_irqrestore(&uhci->urb_remove_list_lock, flags);
 
 	out += sprintf(out, "Complete list URBs:");
-	spin_lock_irqsave(&uhci->complete_list_lock, flags);
 	if (list_empty(&uhci->complete_list))
 		out += sprintf(out, " Empty\n");
 	else {
@@ -411,7 +401,6 @@ static int uhci_show_lists(struct uhci_hcd *uhci, char *buf, int len)
 			tmp = tmp->next;
 		}
 	}
-	spin_unlock_irqrestore(&uhci->complete_list_lock, flags);
 
 	return out - buf;
 }
@@ -425,7 +414,7 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci, char *buf, int len)
 	struct uhci_td *td;
 	struct list_head *tmp, *head;
 
-	spin_lock_irqsave(&uhci->frame_list_lock, flags);
+	spin_lock_irqsave(&uhci->schedule_lock, flags);
 
 	out += sprintf(out, "HC status\n");
 	out += uhci_show_status(uhci, out, len - (out - buf));
@@ -508,11 +497,11 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci, char *buf, int len)
 		}
 	}
 
-	spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
-
 	if (debug > 2)
 		out += uhci_show_lists(uhci, out, len - (out - buf));
 
+	spin_unlock_irqrestore(&uhci->schedule_lock, flags);
+
 	return out - buf;
 }
 
@@ -623,4 +612,3 @@ static struct file_operations uhci_proc_operations = {
 	.release =	uhci_proc_release,
 };
 #endif
-

drivers/usb/host/uhci-hcd.h

@@ -342,8 +342,8 @@ struct uhci_hcd {
 	struct uhci_td *term_td;	/* Terminating TD, see UHCI bug */
 	struct uhci_qh *skelqh[UHCI_NUM_SKELQH];	/* Skeleton QH's */
 
-	spinlock_t frame_list_lock;
-	struct uhci_frame_list *fl;		/* P: uhci->frame_list_lock */
+	spinlock_t schedule_lock;
+	struct uhci_frame_list *fl;		/* P: uhci->schedule_lock */
 	int fsbr;				/* Full-speed bandwidth reclamation */
 	unsigned long fsbrtimeout;		/* FSBR delay */
 
@@ -353,24 +353,19 @@ struct uhci_hcd {
 	unsigned int saved_framenumber;		/* Save during PM suspend */
 
 	/* 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 */
+	struct list_head urb_list;		/* P: uhci->schedule_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;	/* P: uhci->qh_remove_list_lock */
+	struct list_head qh_remove_list;	/* P: uhci->schedule_lock */
 
 	/* List of TD's that are done, but waiting to be freed (race) */
-	spinlock_t td_remove_list_lock;
-	struct list_head td_remove_list;	/* P: uhci->td_remove_list_lock */
+	struct list_head td_remove_list;	/* P: uhci->schedule_lock */
 
 	/* List of asynchronously unlinked URB's */
-	spinlock_t urb_remove_list_lock;
-	struct list_head urb_remove_list;	/* P: uhci->urb_remove_list_lock */
+	struct list_head urb_remove_list;	/* P: uhci->schedule_lock */
 
 	/* List of URB's awaiting completion callback */
-	spinlock_t complete_list_lock;
-	struct list_head complete_list;		/* P: uhci->complete_list_lock */
+	struct list_head complete_list;		/* P: uhci->schedule_lock */
 
 	int rh_numports;
 
@@ -401,26 +396,15 @@ struct urb_priv {
 /*
  * 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.
+ * Almost everything relating to the hardware schedule and processing
+ * of URBs is protected by uhci->schedule_lock.  urb->status is protected
+ * by urb->lock; that's the one exception.
  *
- * Here's the safe locking order to prevent deadlocks:
+ * To prevent deadlocks, never lock uhci->schedule_lock while holding
+ * urb->lock.  The safe order of locking is:
  *
- * #1 uhci->urb_list_lock
+ * #1 uhci->schedule_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
  */
 
 #endif