On Thu, May 03, 2001, Martin Diehl <[EMAIL PROTECTED]> wrote:
> > Anyway - chances are the observation that uhci_append_queued_urb() makes
> > significant contribution to the problem might be helpful. Probably the
> > eurb which was determined much earlier with status==EINPROGRESS might be
> > racy: all active TD of eurb might meanwhile been transmitted. This might
> > even be true from the very beginning - eurb is determined inside
> > spin_lock_irqsave, so status might be outdated (i.e. all its TD's
> > already inactive at this point). So we end up connecting the new QH's TD
> > to a lltd->link, which is now out-of-reach for the HC?
>
> just proved: exactly this HC/HCD race is killing my test!
>
> After narrowing down the racy window the issue disappeared. Idea is to
> test if lltd->status is still set ACTIVE as late as possible before
> appending the new QH. Fallback to normal insertion if found inactive.
> With this change applied I was unable to break it with all the tests tried
> so far - including the remote floodping saturation which used to kill the
> thing instantaneously.
>
> Two points two verify:
> - idea is based on the assumption, whenever we find *lltd (the last TD
> from the last QH for the same EP) beeing inactive there are absolutely
> no active TD's pending for this EP - do you see any reason this might be
> wrong?
> - There is still a minor window for this race between testing lltd->status
> and lltd->link update becomes visible to the HC. Lower limit for this
> window is given by the uhci_fixup_toggle() call, i.e. depends on the
> number of TD's for the QH to be appended. For example, assuming
> something like 100nsec/TD to update toggle a 8KB=128*64 transfer would
> give about 10usec window. Not so small compared to about 50 usec the TD
> is on the wire. I'm not sure whether we can move the call in front of
> the test - we would have to restore the toggles in case the test fails
> and we fallback. If so, the window would be reduced to some 10 nsec,
> i.e. shorter than USB-1.1 bittime - should be save I think.
> Of course the right thing would be to synchronize with HC, but I don't
> see how to do this.
> Currently I've kept the toggle-call in the window and tried to detect if
> we were overrun - which in turn has a mini-window for false positive...
>
> Updated patch below. The additional more or less cosmetical changes
> (driver name, GFP_DMA) still include the /proc/driver/uhci typo fix
> you've already forwarded to Linus - patch is vs. 2.4.4.
Here's a different version which I think isn't susceptible to aformentioned
races. I'm going to go over it some more, but I've had good results with
it so far.
On Thu, May 03, 2001, Georg Acher <[EMAIL PROTECTED]> wrote:
> On Thu, May 03, 2001 at 05:37:09PM +0200, Martin Diehl wrote:
> <...>
> > > spin_lock_irqsave, so status might be outdated (i.e. all its TD's
> > > already inactive at this point). So we end up connecting the new QH's TD
> > > to a lltd->link, which is now out-of-reach for the HC?
> >
> > just proved: exactly this HC/HCD race is killing my test!
>
> This is exactly the cause, why usb-uhci is always using a "bottom QH" (bqh)
> as the last element in the vertical TD list when queuing bulks. With this
> last QH, UHCI enters a new context and doesn't update the element pointer of
> the old "top" QH. So there's no race when appending new TDs, the old bqh is
> recycled as the top QH of the new TD list. Of course, there's a slight
> memory cost, but absolutely no race window.
>
> BTW: The helper QH just serves as unified "collection" point, where all bqhs
> are horizontally linked to. It eliminates the work of re-linking of all
> bqhs when a following QH is deleted.
Ahh, so that's what the code was doing. I looked at your driver to see
what you were doing but couldn't figure it out. That makes perfect sense
and is definately a better solution. Thanks for the explanation. My patch
makes uhci.c operate like that.
JE
--- linux-2.4.5-pre1.orig/drivers/usb/uhci.c Wed May 2 12:25:47 2001
+++ linux-2.4.5-pre1/drivers/usb/uhci.c Thu May 3 14:32:41 2001
@@ -306,7 +306,7 @@
/*
* Inserts a td into qh list at the top.
*/
-static void uhci_insert_tds_in_qh(struct uhci_qh *qh, struct urb *urb, int breadth)
+static void uhci_insert_tds_in_qh(struct uhci_qh *qh, struct urb *urb, int breadth,
+struct uhci_qh *bqh)
{
struct list_head *tmp, *head;
struct urb_priv *urbp = (struct urb_priv *)urb->hcpriv;
@@ -339,7 +339,10 @@
ptd = td;
}
- ptd->link = UHCI_PTR_TERM;
+ if (bqh)
+ ptd->link = bqh->dma_handle | UHCI_PTR_QH;
+ else
+ ptd->link = UHCI_PTR_TERM;
}
static void uhci_free_td(struct uhci *uhci, struct uhci_td *td)
@@ -390,12 +393,9 @@
pci_pool_free(uhci->qh_pool, qh, qh->dma_handle);
}
-static void uhci_insert_qh(struct uhci *uhci, struct uhci_qh *skelqh, struct uhci_qh
*qh)
+static void _uhci_insert_qh(struct uhci_qh *skelqh, struct uhci_qh *qh)
{
struct uhci_qh *lqh;
- unsigned long flags;
-
- spin_lock_irqsave(&uhci->frame_list_lock, flags);
/* Grab the last QH */
lqh = list_entry(skelqh->list.prev, struct uhci_qh, list);
@@ -405,6 +405,15 @@
lqh->link = qh->dma_handle | UHCI_PTR_QH;
list_add_tail(&qh->list, &skelqh->list);
+}
+
+static void uhci_insert_qh(struct uhci *uhci, struct uhci_qh *skelqh, struct uhci_qh
+*qh)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&uhci->frame_list_lock, flags);
+
+ _uhci_insert_qh(skelqh, qh);
spin_unlock_irqrestore(&uhci->frame_list_lock, flags);
}
@@ -511,7 +520,7 @@
mb(); /* Make sure we flush everything */
/* Only support bulk right now, so no depth */
- lltd->link = ftd->dma_handle;
+ lurbp->bqh->element = ftd->dma_handle;
list_add_tail(&urbp->queue_list, &furbp->queue_list);
@@ -571,28 +580,13 @@
usb_pipeout(urb->pipe), toggle);
if (!urbp->queued) {
- int status;
-
- /* The HC may continue using the current QH if it finished */
- /* all of the TD's in this URB and may have started on the */
- /* next URB's TD's already, so we'll take over ownership */
- /* of this QH and use it instead. Don't forget to delete */
- /* the old QH first */
+ /* Steal the bottom QH from the previous URB */
uhci_free_qh(uhci, nurbp->qh);
+ nurbp->qh = urbp->bqh;
+ nurbp->qh->urbp = urbp;
+ urbp->bqh = NULL;
- nurbp->qh = urbp->qh;
- nurbp->qh->urbp = nurbp;
- urbp->qh = NULL;
-
- /* If the last TD from the first (this) urb didn't */
- /* complete, reset qh->element to the first TD in the */
- /* next urb */
- pltd = list_entry(urbp->td_list.prev, struct uhci_td, list);
- status = uhci_status_bits(pltd->status);
- if ((status & TD_CTRL_ACTIVE) || uhci_actual_length(pltd->status) <
uhci_expected_length(pltd->info)) {
- struct uhci_td *ftd = list_entry(nurbp->td_list.next, struct
uhci_td, list);
- nurbp->qh->element = ftd->dma_handle;
- }
+ _uhci_insert_qh(uhci->skel_bulk_qh, nurbp->qh);
nurbp->queued = 0;
} else {
@@ -899,10 +893,10 @@
/* Low speed or small transfers gets a different queue and treatment */
if (urb->pipe & TD_CTRL_LS) {
- uhci_insert_tds_in_qh(qh, urb, 0);
+ uhci_insert_tds_in_qh(qh, urb, 0, NULL);
uhci_insert_qh(uhci, uhci->skel_ls_control_qh, qh);
} else {
- uhci_insert_tds_in_qh(qh, urb, 1);
+ uhci_insert_tds_in_qh(qh, urb, 1, NULL);
uhci_insert_qh(uhci, uhci->skel_hs_control_qh, qh);
uhci_inc_fsbr(uhci, urb);
}
@@ -1064,7 +1058,7 @@
urbp->qh->urbp = urbp;
/* One TD, who cares about Breadth first? */
- uhci_insert_tds_in_qh(urbp->qh, urb, 0);
+ uhci_insert_tds_in_qh(urbp->qh, urb, 0, NULL);
/* Low speed or small transfers gets a different queue and treatment */
if (urb->pipe & TD_CTRL_LS)
@@ -1277,8 +1271,16 @@
urbp->qh = qh;
qh->urbp = urbp;
+ if (urb->transfer_flags & USB_QUEUE_BULK) {
+ urbp->bqh = uhci_alloc_qh(uhci, urb->dev);
+ if (!urbp->bqh)
+ return -ENOMEM;
+
+ urbp->bqh->urbp = urbp;
+ }
+
/* Always assume breadth first */
- uhci_insert_tds_in_qh(qh, urb, 1);
+ uhci_insert_tds_in_qh(qh, urb, 1, urbp->bqh);
if (urb->transfer_flags & USB_QUEUE_BULK && eurb)
uhci_append_queued_urb(uhci, eurb, urb);
@@ -1674,9 +1676,12 @@
uhci_delete_queued_urb(uhci, urb);
+ /* The interrupt loop will reclaim the QH's */
if (urbp->qh)
- /* The interrupt loop will reclaim the QH's */
uhci_remove_qh(uhci, urbp->qh);
+
+ if (urbp->bqh)
+ uhci_remove_qh(uhci, urbp->bqh);
}
/* FIXME: If we forcefully unlink an urb, we should reset the toggle for */
--- linux-2.4.5-pre1.orig/drivers/usb/uhci.h Fri Apr 27 15:49:56 2001
+++ linux-2.4.5-pre1/drivers/usb/uhci.h Thu May 3 11:31:28 2001
@@ -331,7 +331,8 @@
dma_addr_t setup_packet_dma_handle;
dma_addr_t transfer_buffer_dma_handle;
- struct uhci_qh *qh; /* QH for this URB */
+ struct uhci_qh *qh;
+ struct uhci_qh *bqh; /* For queued URB's */
struct list_head td_list;
int fsbr : 1; /* URB turned on FSBR */
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel
