On Mon, Oct 12, 2009 at 10:36:52PM -0400, Alan Stern wrote:
> 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.
Testing for the broken Intel chipset would be a good idea, yes.
However, it means way more bureaucratic headache for me, since I would
have to get committee clearance to do so. Also, the engineering team
wanted this to be a generic fix because they thought it would cause
problems in future designs. They read the EHCI spec as saying that
software should not attempt to schedule within the IST. The spec
clearly doesn't make it a requirement (no shalls in that section), but
it's apparently not something any of the other OSes do, either. These
are somewhat weak arguments, but my preference is to make this generic.
> > > 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.
Ah, ok, I see the flaw there. I was perplexed because the fix worked on
this platform 20 times, and failed 20 times without it. I can see from
my logs that I stopped using cheese before the microframe index wrapped.
> > > 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. :-)
I can turn this into a patch and test it on the box on Friday. At this
point it's really your code, so would you like to make the patch?
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
