On Mon, 12 Oct 2009, Sarah Sharp wrote:
> 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.
You might want to make this change as specific as possible, so that
existing controllers will see the same behavior as before. That means
testing for the bad controller type and testing for full-speed
transfers.
> > 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?
Let's see. For the sake of argument, suppose mod is 1024 and
ehci->i_thresh is 10 (frame caching). Consider the case where now is
1020, max is 2044, next is 1030, and stream->next_uframe is 3. Since
the (start < next) test succeeds, you'll change start to 1027. Then
the (start >= max - 2 * 8 * SCHEDULE_SLOP) test will fail, as will
the (start + sched->span >= max) test. So you'll set
stream->next_uframe to (start % mod) == 3 and end up scheduling in the
middle of the threshold period, which wraps around from 1020 to 6.
> > 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.
You can fix that by changing the test below; replace max by (max - 2 *
8 * SCHEDULE_SLOP) or something of the sort.
> > /* 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.
I agree with your agreement. :-)
Alan Stern
--
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
