On Thu, 3 May 2001, Martin Diehl wrote: > Ok, seems I should check that. My reading of UHCI-1.1 docs, p32 was, the > HC would never advance to the next QH on NAK (or other error)... Done - I was wrong. The advance criteria is only applied to the vertical processing. paperbag for blind men's approach. > 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. Martin ----------- --- linux-2.4.4/drivers/usb.orig/uhci.c Wed May 2 19:41:41 2001 +++ linux-2.4.4/drivers/usb/uhci.c Thu May 3 17:19:58 2001 @@ -181,7 +181,7 @@ dma_addr_t dma_handle; struct uhci_td *td; - td = pci_pool_alloc(uhci->td_pool, GFP_DMA | GFP_ATOMIC, &dma_handle); + td = pci_pool_alloc(uhci->td_pool, SLAB_ATOMIC, &dma_handle); if (!td) return NULL; @@ -358,7 +358,7 @@ dma_addr_t dma_handle; struct uhci_qh *qh; - qh = pci_pool_alloc(uhci->qh_pool, GFP_DMA | GFP_ATOMIC, &dma_handle); + qh = pci_pool_alloc(uhci->qh_pool, SLAB_ATOMIC, &dma_handle); if (!qh) return NULL; @@ -507,15 +507,36 @@ lltd = list_entry(lurbp->td_list.prev, struct uhci_td, list); ftd = list_entry(urbp->td_list.next, struct uhci_td, list); - uhci_fixup_toggle(urb, uhci_toggle(lltd->info) ^ 1); + if (lltd->status & TD_CTRL_ACTIVE) { + + /* minor window starts here */ + uhci_fixup_toggle(urb, uhci_toggle(lltd->info) ^ 1); - mb(); /* Make sure we flush everything */ - /* Only support bulk right now, so no depth */ - lltd->link = ftd->dma_handle; + mb(); /* Make sure we flush everything */ + /* Only support bulk right now, so no depth */ + lltd->link = ftd->dma_handle; + + /* end of minor window: we are lost if the HC finished + * processing *lltd before it sees the updated lltd->link + */ + mb(); + if (!(lltd->status&TD_CTRL_ACTIVE)) + err("%s: HC/HCD race? (might be false positive)", + __FUNCTION__); - list_add_tail(&urbp->queue_list, &furbp->queue_list); + list_add_tail(&urbp->queue_list, &furbp->queue_list); - urbp->queued = 1; + urbp->queued = 1; + } + else { /* we've lost the race: HC has already finished + * all TD's for this endpoint + * so we fallback to normal insertion mode + */ + + spin_unlock_irqrestore(&uhci->frame_list_lock, flags); + uhci_insert_qh(uhci, uhci->skel_bulk_qh, urbp->qh); + return; + } spin_unlock_irqrestore(&uhci->frame_list_lock, flags); } @@ -2548,14 +2569,14 @@ uhci->fl->dma_handle = dma_handle; uhci->td_pool = pci_pool_create("uhci_td", uhci->dev, - sizeof(struct uhci_td), 16, 0, GFP_DMA | GFP_ATOMIC); + sizeof(struct uhci_td), 16, 0, SLAB_ATOMIC); if (!uhci->td_pool) { printk(KERN_ERR "Unable to create td pci_pool\n"); goto free_fl; } uhci->qh_pool = pci_pool_create("uhci_qh", uhci->dev, - sizeof(struct uhci_qh), 16, 0, GFP_DMA | GFP_ATOMIC); + sizeof(struct uhci_qh), 16, 0, SLAB_ATOMIC); if (!uhci->qh_pool) { printk(KERN_ERR "Unable to create qh pci_pool\n"); goto free_td_pool; @@ -2819,7 +2840,7 @@ uhci->proc_entry = ent; #endif - request_region(uhci->io_addr, io_size, "usb-uhci"); + request_region(uhci->io_addr, io_size, "uhci"); reset_hc(uhci); @@ -2827,7 +2848,7 @@ start_hc(uhci); retval = -EBUSY; - if (request_irq(irq, uhci_interrupt, SA_SHIRQ, "usb-uhci", uhci) == 0) { + if (request_irq(irq, uhci_interrupt, SA_SHIRQ, "uhci", uhci) == 0) { uhci->irq = irq; pci_write_config_word(dev, USBLEGSUP, USBLEGSUP_DEFAULT); @@ -2986,7 +3007,7 @@ up_failed: #ifdef CONFIG_PROC_FS - remove_proc_entry("uhci", 0); + remove_proc_entry("driver/uhci", 0); proc_failed: #endif @@ -3006,7 +3027,7 @@ printk(KERN_INFO "uhci: not all urb_priv's were freed\n"); #ifdef CONFIG_PROC_FS - remove_proc_entry("uhci", 0); + remove_proc_entry("driver/uhci", 0); #endif if (errbuf) _______________________________________________ [EMAIL PROTECTED] To unsubscribe, use the last form field at: http://lists.sourceforge.net/lists/listinfo/linux-usb-devel