On Tue, 8 May 2001, David Brownell wrote:
> > How about a simpler fix? ...
>
> Like the attached patch. This is against 2.4.4 and includes:
>
> - that simpler hashchain patch (no new spinlocks!)
> - what ac5 had (handle more flakey hardware)
> - lots of readl() to flush PCI writes
>
> That readl() stuff may, the theory goes, also handle some
> flakey hardware cases ... maybe fewer could be used, but
> in this case I thought "better safe than sorry".
Hi David,
I've just re-run the test which triggered the BUG() for me with your
ohci-0508 patch applied. Works fine for me, no BUG() or other problems
encountered. However, IMHO it is not the whole story yet. My test does not
cover the error paths so far. So it's not too much surprise some remaining
chances to BUG() due to error-out handling were not hit. Let me explain:
The BUG() appeared on UP, so it must be a race with writers in
interrupt. Hence all readers and writers to the hashchains have to be
serialized. Candidates are all hash_add_*(), hash_free_*() and dma_to_*()
functions from usb-ohci.h. As already stated in a former post I also
dislike adding a new spinlock since the hashchains are part of the ed's
and thus should be protected by the enclosing usb_ed_lock.
However, td_alloc() is not the only path, where we have the candidates
in. td_free() is another example. Most places where it is called are
already protected. One path however is urb_free_priv() -> td_free() which
is unprotected in some out-on-error cases in sohci_submit_urb(). I've
solved this by simply not releasing the usb_ed_lock between the td_alloc()
loop and linking the ep/submitting the urb. This covers the two occasions
where urb_free_priv() is called unprotected. Furthermore the intermediate
code (iso/int bandwidth allocation) is not only pretty cheap and doesn't
sleep, it is also racy by itself: window between bandwidth check and claim
for all HCD's! Not dropping the lock would solve this too - although I do
believe there should be some bus->lock for the bandwidth stuff.
Finally, the several direct calls to hash_add/free and dma_to_*() in
usb-ohci.c are well protected AFAICS.
Below, I've included the patch (to be applied after your ohci-0508!).
Tested as before, i.e. not covering the error path (would require an
iso/int overallocation situation).
Since you've asked, a few words about performance wrt. the PCI post-write
fixes: I don't see any difference in USB-throughput - always 1.15MB/sec
for bulk transfer - with or without the additional readl(). Even when
running in parallel to fullscreen bttv-application we loose only a few
percent - it's still about 1.05 MB/sec.
Martin
---------------------------
--- linux-2.4.4/drivers/usb/usb-ohci.c.ohci-0508 Thu May 10 23:16:36 2001
+++ linux-2.4.4/drivers/usb/usb-ohci.c Fri May 11 09:18:32 2001
@@ -608,10 +608,10 @@
return -ENOMEM;
}
}
- spin_unlock_irqrestore (&usb_ed_lock, flags);
if (ed->state == ED_NEW || (ed->state & ED_DEL)) {
urb_free_priv (ohci, urb_priv);
+ spin_unlock_irqrestore (&usb_ed_lock, flags);
usb_dec_dev_use (urb->dev);
return -EINVAL;
}
@@ -633,6 +633,7 @@
}
if (bustime < 0) {
urb_free_priv (ohci, urb_priv);
+ spin_unlock_irqrestore (&usb_ed_lock, flags);
usb_dec_dev_use (urb->dev);
return bustime;
}
@@ -642,7 +643,6 @@
#endif
}
- spin_lock_irqsave (&usb_ed_lock, flags);
urb->actual_length = 0;
urb->hcpriv = urb_priv;
urb->status = USB_ST_URB_PENDING;
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel
