On Thu, 29 Aug 2013, Ming Lei wrote:
> On Wed, 28 Aug 2013 12:23:31 -0400 (EDT)
> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > You must never alter ehci->last_iso_frame like this. It violates the
> > driver's invariants for time to run "backward". After all, there may
> > already be other TDs scheduled for the frames you are about to scan
> > again; they mustn't get scanned until the frame pointer has wrapped
> > around.
> >
> > This last problem in particular means your proposal can't be accepted.
>
> On Thu, Aug 29, 2013 at 8:43 AM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> > No, no other TDs scheduled for the frames since the queue is empty
> > when iso_stream_rebase is running.
>
> The above isn't true, but no rescan on other TDs introduced:
>
> - suppose URB0 is scheduled via stream0 when underrun without ISO_ASAP
>
> - iso_stream_rebase() find that stream0->next_uframe is behind
> ehci->last_iso_frame but URB0 isn't completed before
> ehci->last_iso_frame, then adjust it as stream0->next_uframe >> 3,
> and record its old value as ehci->last_iso_frame'.
>
> - when next ehci irq comes, scan_isoc() scans from stream0->next_uframe
> to 'now', and the extra scan introduced by iso_stream_rebase() is from
> stream0->next_uframe to ehci->last_iso_frame' - 1, during this period,
> only TDs belonged to undrun URBs without ISO_ASAP will scanned, and no
> other TDs scheduled in these frames at all.(ehci->last_iso_frame doesn't
> affect schedule, only decides start point of the next scan)
ehci->last_iso_frame does indeed affect the schedule:
base = ehci->last_iso_frame << 3;
next = (next - base) & (mod - 1);
start = (stream->next_uframe - base) & (mod - 1);
/* Is the schedule already full? */
if (unlikely(start < period)) {
ehci_dbg(ehci, "iso sched full %p (%u-%u < %u mod %u)\n",
urb, stream->next_uframe, base,
period, mod);
status = -ENOSPC;
goto fail;
}
Note that start depends on base, which is derived from
ehci->last_iso_frame. If you decrease ehci->last_iso_frame, earlier
calculations using the larger value will now be incorrect.
> So no sort of run 'backward' things, and URBs are always completed in
> time order, aren't they?
No. Here's an example. I will exaggerate some of the values to help
make the point clear.
An SMI blocks interrupts for 100 ms. When the SMI ends, we
give back all the isochronous URBs on endpoints A and B. The
frame counter is now equal to 600, but the URBs we gave back
were scheduled to end in frame 520. The endpoint queues are
empty.
The completion handler for ep A submits an URB lasting for 1000
frames (11 packets with interval = 100); it is scheduled to
start in frame 620 and its last packet is scheduled for frame
596 after wrapping around.
The completion hanlder for ep B then runs, and it submits an
URB lasting 95 frames; it is scheduled to start in frame 521
and its last packet is scheduled for frame 615. Since this is
larger than the current frame counter, you rebase
ehci->last_iso_frame back to 520 or earlier.
Now the next time an interrupt occurs, the driver will scan
the last TD for ep A in frame 596. But that TD shouldn't be
scanned until 1020 ms have elapsed.
Like I said, this is exaggerated and probably will never happen. But
it is legal according to the API and therefore we have to allow it.
Besides, at this point it's not that much extra work for you to add the
sched->first_packet field. With that in place, you don't need to
rebase last_iso_frame, which removes the problem. It also allows the
driver to be improved by not scheduling any TDs that lie between
last_iso_frame and the current frame counter.
Of course, once you do this, your patch will start to look an awful lot
like mine.
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
