[SeaBIOS] [PATCH 03/16] usb-uhci: fix race against host controller

Kevin O'Connor kevin at koconnor.net
Thu Nov 17 05:53:02 CET 2011


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.



More information about the SeaBIOS mailing list