On Fri, 2 Oct 2009, Sarah Sharp wrote:
> On Thu, Oct 01, 2009 at 03:32:59PM -0400, Alan Stern wrote:
> > On Thu, 1 Oct 2009, Sarah Sharp wrote:
> >
> > > Does the EHCI driver respect the isochronous scheduling threshold that the
> > > hardware reports?
> >
> > Depends what you mean. As I recall, the scheduling threshold is
> > defined mainly for use in unlinking -- and ehci-hcd doesn't really
> > support unlinking isochronous transfers at all.
>
> There were a couple engineers at Intel working on a new integrated EHCI
> host controller who were concerned about the Linux EHCI driver
> "respecting the IST". They had seen some device disconnects when
> another OS tried to unlink an active iTD within the zone between the
> current microframe and the isoc scheduling threshold. Since the EHCI
> driver just waits until the iTD completes before it unlinks it, this
> shouldn't be a problem, however...
Unlinks shouldn't, no. At some point we may add support for genuine
isochronous unlinks. But if/when we do, the code will use the
shceduling threshold properly.
> > > I see several places in iso_stream_schedule() in ehci-sched.c that use
> > > SCHEDULE_SLOP. I don't really understand the case where the stream is already
> > > being used,
> >
> > If the stream is already in use then new iTDs can be scheduled for any
> > frame right down to the currently active frame. It's not a big deal if
> > they turn out to be scheduled too soon; since they're isochronous we
> > expect that now and then some of them won't get transferred properly.
>
> ..the engineers also thought that the IST should be honored whenever the
> isoc schedule changes, including when new iTDs are added to an active
> endpoint. They're concerned about hardware executing the stale cached
> linked list and causing "undefined behavior" when the HW and SW go out
> of sync.
I don't see how. The stale cached linked list is still valid; it's
only drawback is that it lacks the new entries.
> It looks like you made a commit to the isoc scheduling code back in May
> 2008 (commit b40e43fcc532fa44a375a37d592e32cd0d50fe7a) that may have
> made the driver more resilient to this race condition?
It improved the scheduling code, but it didn't really affect the race
you are concerned about.
> I'm a bit confused, because that code uses a local variable "start" and
> then doesn't seem to do anything with it. It looks like you're trying
> to ensure the starting microframe for the new transfer isn't in the
> past, and isn't too far in the future to wrap past our current place in
> the frame list:
Right.
> now = ehci_readl(ehci, &ehci->regs->frame_index) % mod;
>
> /* when's the last uframe this urb could start? */
> max = now + mod;
>
> /* Typical case: reuse current schedule, stream is still active.
> * Hopefully there are no gaps from the host falling behind
> * (irq delays etc), but if there are we'll take the next
> * slot in the schedule, implicitly assuming URB_ISO_ASAP.
> */
> if (likely (!list_empty (&stream->td_list))) {
> start = stream->next_uframe;
> if (start < now)
> start += mod;
>
> /* Fell behind (by up to twice the slop amount)? */
> if (start >= max - 2 * 8 * SCHEDULE_SLOP)
> start += stream->interval * DIV_ROUND_UP(
> max - start, stream->interval) - mod;
>
> /* Tried to schedule too far into the future? */
> if (unlikely((start + sched->span) >= max)) {
> status = -EFBIG;
> goto fail;
> }
> goto ready;
> }
>
> However, the ready label does nothing with the start variable:
>
> ready:
> /* report high speed start in uframes; full speed, in frames */
> urb->start_frame = stream->next_uframe;
> if (!stream->highspeed)
> urb->start_frame >>= 3;
> return 0;
>
> Shouldn't the code set urb->start_frame to the start variable? Or at
> least do something with it?
You're right; stream->next_uframe should be set equal to start before
the goto. This is a bug. Would you care to fix it?
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
