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...
> > I see that the driver sets ehci->i_thresh to the number of microframes that the
> > host controller may cache (plus 2 if the host controller reports a number of
> > microframes instead of just one frame), but the driver doesn't seem to do
> > anything with i_thresh.
>
> Right.
>
> > 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.
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?
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:
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?
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
