On Sun, Oct 11, 2009 at 10:31:36AM -0400, Alan Stern wrote:
> Can you describe which hardware this applies to? And does the bug
> affect high-speed isochronous endpoints?
I don't know if I'm supposed to say which hardware this applies to, so
I'll have to err on the side of caution. The chipset engineers said
this only applies to split isoc transactions, but I didn't tweak a high
speed webcam driver to confirm.
> On Fri, 9 Oct 2009, Sarah Sharp wrote:
>
> > @@ -1354,6 +1354,7 @@ iso_stream_schedule (
> > period <<= 3;
> >
> > now = ehci_readl(ehci, &ehci->regs->frame_index) % mod;
> > + next = now + ehci->i_thresh;
> >
> > /* when's the last uframe this urb could start? */
> > max = now + mod;
> > @@ -1365,13 +1366,15 @@ iso_stream_schedule (
> > */
> > if (likely (!list_empty (&stream->td_list))) {
> > start = stream->next_uframe;
> > - if (start < now)
> > + if (start < next)
> > start += mod;
> >
> > /* Fell behind (by up to twice the slop amount)? */
> > if (start >= max - 2 * 8 * SCHEDULE_SLOP)
> > start += period * DIV_ROUND_UP(
> > - max - start, period) - mod;
> > + max - start + ehci->i_thresh,
> > + period)
> > + - mod;
> >
> > /* Tried to schedule too far into the future? */
> > if (unlikely((start + sched->span) >= max)) {
> > status = -EFBIG;
> > goto fail;
> > }
> > stream->next_uframe = start;
> > goto ready;
>
> Sorry, what I wrote before wasn't quite right.
Can you give an example of where my patch would fail to schedule
correctly? start does need to be moded by the periodic schedule size
before we jump to ready, but was there a different mistake in this
patch?
> This computation is
> tricky. To check whether start is too small, you have to fit it into
> the interval from next to (next + mod - 1). But then to check whether
> it is too big, you have to fit it into the interval from now to (now +
> mod - 1). And then in the end, you need to assign stream->next_uframe
> a value between 0 and (mod - 1).
>
> Since mod is always a power of two, the easiest approach may be
> something like this:
>
> next = now + ehci->i_thread;
> ...
> start = stream->next_uframe;
>
> /* Fell behind (by up to twice the slop amount)? */
> if ((start - next) & (mod - 1) >=
> mod - 2 * 8 * SCHEDULE_SLOP)
> start += period * DIV_ROUND_UP(
> (next - start) & (mod - 1),
> period);
This looks fine, except in the case where the driver has scheduled
transfers really far in the future (where stream->next_uframe > next +
mod - 2 * 8 * SCHEDULE_SLOP). The old code and my patch have a similar
problem, of course.
> /* Tried to schedule too far into the future? */
> if (unlikely(((start - now) & (mod - 1)) + sched->span
> >= max)) { ...
> }
> stream->next_uframe = start % mod;
> goto ready;
I agree with this math.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
