On Wed, Nov 16, 2011 at 01:02:44PM +0100, Paolo Bonzini wrote:
While processing a frame, the host controller will write to the queue head's element link field. The following sequence could then happen when two consecutive sends occur to the same pipe.
controller SeaBIOS --------------------------------------------------------------------- td->link = UHCI_PTR_TERM; td->ctrl |= TD_CTRL_ACTIVE; read TD from memory wait_td(td); td->ctrl &= ~TD_CTRL_ACTIVE; write back td->ctrl exit usb_send_bulk restart usb_send_bulk pipe->qh.element = &tds; pipe->qh.element = td->link; ... go on and set up the first td ... write back pipe->qh.element td->ctrl |= TD_CTRL_ACTIVE;Once the host controller has written UHCI_PTR_TERM to the element link, subsequent tds would never be processed. This is surprisingly frequent when the two consecutive sends are in the OUT direction (and just as surprisingly, it seems like it never happens in the IN direction).
Interesting. Good catch! If I understand this correctly, the issue occurs because all TDs being complete doesn't guarentee that the QH is complete, and thus the next transfer races with the controller on the QH update.
To fix this, re-enable TDs whenever the host controller completes the pending ones. Also, at the end of the processing do not wait for each single TD to become inactive, but simply wait for the host controller to invalidate the element link.
[...]
- // Enable tds
- barrier();
- SET_FLATPTR(pipe->qh.element, (u32)MAKE_FLATPTR(GET_SEG(SS), tds));
- int tdpos = 0;
- struct uhci_td *start_td = tds; while (datasize) { struct uhci_td *td = &tds[tdpos++ % STACKTDS]; int ret = wait_td(td);
@@ -479,21 +476,23 @@ uhci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize) barrier(); td->status = (uhci_maxerr(3) | (lowspeed ? TD_CTRL_LS : 0) | TD_CTRL_ACTIVE);
if (GET_FLATPTR(pipe->qh.element) & UHCI_PTR_TERM) {// Enable tds. The previous td might be the first one, since// there is still a race window between reading and writing// pipe->qh.element (we do not use CMPXCHG). start_td ensures// that what we write will always be linked to the first active td.barrier();SET_FLATPTR(pipe->qh.element, (u32)MAKE_FLATPTR(GET_SEG(SS), start_td));}
Can't the code just use wait_pipe() at the end to eliminate the race? Why does the code need to recheck for qh.element==UHCI_PTR_TERM in the loop? The host should never modify qh.element, and the controller can only assign qh.element=td->link. So, the only way qh.element can be UHCI_PTR_TERM is if the transfer is complete (in which case we don't want to start it again) or the transfer is just beginning (which can be setup at the start of the transfer instead of in the loop). Am I missing something?
[...]
- int i;
- for (i=0; i<STACKTDS; i++) {
struct uhci_td *td = &tds[tdpos++ % STACKTDS];int ret = wait_td(td);if (ret)goto fail;- }
- SET_FLATPTR(pipe->toggle, !!toggle);
- return 0;
- return wait_pipe(pipe);
I agree this is better, but the timeouts have changed. Bulk transfers need more time than 500ms to complete.
Thanks for working on this. -Kevin
PS - the patch series looks good to me.