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.
This patch is relative to 2.4.19-pre8, however I think it can be
applied, and should be applied, to 2.5 as well.
JE
--- linux-2.4.19-pre8.orig/drivers/usb/uhci.c Fri May 10 19:40:39 2002
+++ linux-2.4.19-pre8/drivers/usb/uhci.c Fri May 10 20:58:09 2002
@@ -100,6 +100,11 @@
#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 */
/*
@@ -115,12 +120,20 @@
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);
}
@@ -129,7 +142,7 @@
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);
}
@@ -474,9 +487,9 @@
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;
}
@@ -953,14 +966,6 @@
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;
@@ -1127,14 +1132,6 @@
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;
@@ -1830,11 +1827,18 @@
{
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) {
@@ -1842,10 +1846,15 @@
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;
_______________________________________________________________
Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: [EMAIL PROTECTED]
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
