[PATCH] uhci.c FSBR timeout

There was a discussion a long time ago about how safe the bit operations
were as well as recently.

set_bit/clear_bit are not safe on x86 UP, nor are they safe on other
architectures. It's also unclear from the UHCI spec if the HC's are safe
with respect to atomic updates to the status field.

This patch ditches all of the uses of set_bit and clear_bit and changes
the algorithm that depended on it.

The FSBR timeout algorithm would reenable FSBR for transfers when they
started making progress again. So instead of trying for this best case,
we convert the transfer over to depth first from the standard breadth
first. To make sure the transfer doesn't hog all of the bandwidth, every
5th TD is left in breadth first mode. This will ensure other transfers
get some bandwidth.

It's not perfect, but I think it's a good compromise.

Note: td->info is read only by the HC, so we can touch it whenever we
want.

drivers/usb/host/uhci.c

@@ -101,6 +101,11 @@ static void wakeup_hc(struct uhci *uhci);
 #define IDLE_TIMEOUT	(HZ / 20)	/* 50 ms */
 #define FSBR_DELAY	(HZ / 20)	/* 50 ms */
 
+/* When we timeout an idle transfer for FSBR, we'll switch it over to */
+/* depth first traversal. We'll do it in groups of this number of TD's */
+/* to make sure it doesn't hog all of the bandwidth */
+#define DEPTH_INTERVAL	5
+
 #define MAX_URB_LOOP	2048		/* Maximum number of linked URB's */
 
 /*
@@ -116,12 +121,20 @@ static int uhci_free_dev(struct usb_device *dev)
 	return 0;
 }
 
+/*
+ * Technically, updating td->status here is a race, but it's not really a
+ * problem. The worst that can happen is that we set the IOC bit again
+ * generating a spurios interrupt. We could fix this by creating another
+ * QH and leaving the IOC bit always set, but then we would have to play
+ * games with the FSBR code to make sure we get the correct order in all
+ * the cases. I don't think it's worth the effort
+ */
 static inline void uhci_set_next_interrupt(struct uhci *uhci)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&uhci->frame_list_lock, flags);
-	set_bit(TD_CTRL_IOC_BIT, &uhci->skel_term_td->status);
+	uhci->skel_term_td->status |= TD_CTRL_IOC_BIT;
 	spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
 }
 
@@ -130,7 +143,7 @@ static inline void uhci_clear_next_interrupt(struct uhci *uhci)
 	unsigned long flags;
 
 	spin_lock_irqsave(&uhci->frame_list_lock, flags);
-	clear_bit(TD_CTRL_IOC_BIT, &uhci->skel_term_td->status);
+	uhci->skel_term_td->status &= ~TD_CTRL_IOC_BIT;
 	spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
 }
 
@@ -475,9 +488,9 @@ static int uhci_fixup_toggle(struct urb *urb, unsigned int toggle)
 		tmp = tmp->next;
 
 		if (toggle)
-			set_bit(TD_TOKEN_TOGGLE, &td->info);
+			td->info |= TD_TOKEN_TOGGLE;
 		else
-			clear_bit(TD_TOKEN_TOGGLE, &td->info);
+			td->info &= ~TD_TOKEN_TOGGLE;
 
 		toggle ^= 1;
 	}
@@ -956,14 +969,6 @@ static int uhci_result_control(struct urb *urb)
 
 		tmp = tmp->next;
 
-		if (urbp->fsbr_timeout && (td->status & TD_CTRL_IOC) &&
-		    !(td->status & TD_CTRL_ACTIVE)) {
-			uhci_inc_fsbr(urb->dev->bus->hcpriv, urb);
-			urbp->fsbr_timeout = 0;
-			urbp->fsbrtime = jiffies;
-			clear_bit(TD_CTRL_IOC_BIT, &td->status);
-		}
-
 		status = uhci_status_bits(td->status);
 		if (status & TD_CTRL_ACTIVE)
 			return -EINPROGRESS;
@@ -1132,14 +1137,6 @@ static int uhci_result_interrupt(struct urb *urb)
 
 		tmp = tmp->next;
 
-		if (urbp->fsbr_timeout && (td->status & TD_CTRL_IOC) &&
-		    !(td->status & TD_CTRL_ACTIVE)) {
-			uhci_inc_fsbr(urb->dev->bus->hcpriv, urb);
-			urbp->fsbr_timeout = 0;
-			urbp->fsbrtime = jiffies;
-			clear_bit(TD_CTRL_IOC_BIT, &td->status);
-		}
-
 		status = uhci_status_bits(td->status);
 		if (status & TD_CTRL_ACTIVE)
 			return -EINPROGRESS;
@@ -1839,11 +1836,18 @@ static int uhci_fsbr_timeout(struct uhci *uhci, struct urb *urb)
 {
 	struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
 	struct list_head *head, *tmp;
+	int count = 0;
 
 	uhci_dec_fsbr(uhci, urb);
 
 	urbp->fsbr_timeout = 1;
 
+	/*
+	 * Ideally we would want to fix qh->element as well, but it's
+	 * read/write by the HC, so that can introduce a race. It's not
+	 * really worth the hassle
+	 */
+
 	head = &urbp->td_list;
 	tmp = head->next;
 	while (tmp != head) {
@@ -1851,10 +1855,15 @@ static int uhci_fsbr_timeout(struct uhci *uhci, struct urb *urb)
 
 		tmp = tmp->next;
 
-		if (td->status & TD_CTRL_ACTIVE) {
-			set_bit(TD_CTRL_IOC_BIT, &td->status);
-			break;
-		}
+		/*
+		 * Make sure we don't do the last one (since it'll have the
+		 * TERM bit set) as well as we skip every so many TD's to
+		 * make sure it doesn't hog the bandwidth
+		 */
+		if (tmp != head && (count % DEPTH_INTERVAL) == (DEPTH_INTERVAL - 1))
+			td->link |= UHCI_PTR_DEPTH;
+
+		count++;
 	}
 
 	return 0;