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)
So no sort of run 'backward' things, and URBs are always completed in
time order, aren't they?
Below patch is modified according to your comment.
drivers/usb/host/ehci-sched.c | 55 ++++++++++++++++++++++++++++++++++++++---
drivers/usb/host/ehci.h | 1 +
2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 83be03f..80da5fd 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1370,6 +1370,35 @@ sitd_slot_ok (
return 1;
}
+/* rebase in case of underun without ISO_ASAP together */
+static int iso_stream_rebase(struct ehci_hcd *ehci, struct urb *urb,
+ struct ehci_iso_stream *stream, u32 span, u32 period)
+{
+ u32 start, size;
+ u32 mod = ehci->periodic_size;
+
+ if (ehci->last_base == -1)
+ return 0;
+
+ start = stream->next_uframe >> 3;
+ size = (ehci->last_iso_frame - ehci->last_base) & (mod - 1);
+
+ /* if the URB should have been started */
+ if (size > ((start - ehci->last_base) & (mod - 1))) {
+ /* if the URB should have been completed now */
+ size = (ehci->last_iso_frame - start) & (mod - 1);
+ if (size > ((span - period) & (mod - 1))) {
+ int i;
+ for (i = 0; i < urb->number_of_packets; i++)
+ urb->iso_frame_desc[i].status = -EXDEV;
+ return 1;
+ }
+ ehci->last_iso_frame = start & (mod - 1);
+ }
+
+ return 0;
+}
+
/*
* This scheduler plans almost as far into the future as it has actual
* periodic schedule slots. (Affected by TUNE_FLS, which defaults to
@@ -1409,7 +1438,9 @@ iso_stream_schedule (
* (irq delays etc). If there are, the behavior depends on
* whether URB_ISO_ASAP is set.
*/
- if (likely (!list_empty (&stream->td_list))) {
+ if (likely (!list_empty (&stream->td_list) ||
+ hcd_complete_in_progress(
+ ehci_to_hcd(ehci), urb))) {
/* Take the isochronous scheduling threshold into account */
if (ehci->i_thresh)
@@ -1417,6 +1448,14 @@ iso_stream_schedule (
else
next = (now + 2 + 7) & ~0x07; /* full frame cache */
+ if (list_empty(&stream->td_list) &&
+ !(urb->transfer_flags & URB_ISO_ASAP))
+ if (iso_stream_rebase(ehci, urb, stream, span,
+ period)) {
+ status = 1;
+ goto fail;
+ }
+
/*
* Use ehci->last_iso_frame as the base. There can't be any
* TDs scheduled for earlier than that.
@@ -1517,6 +1556,7 @@ iso_stream_schedule (
/* Make sure scan_isoc() sees these */
if (ehci->isoc_count == 0)
ehci->last_iso_frame = now >> 3;
+ ehci->last_base = -1;
return 0;
fail:
@@ -1838,7 +1878,10 @@ static int itd_submit (struct ehci_hcd *ehci, struct urb *urb,
status = iso_stream_schedule(ehci, urb, stream);
if (likely (status == 0))
itd_link_urb (ehci, urb, ehci->periodic_size << 3, stream);
- else
+ else if (status > 0) {
+ ehci_urb_done(ehci, urb, 0);
+ status = 0;
+ } else
usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
done_not_linked:
spin_unlock_irqrestore (&ehci->lock, flags);
@@ -2224,7 +2267,10 @@ static int sitd_submit (struct ehci_hcd *ehci, struct urb *urb,
status = iso_stream_schedule(ehci, urb, stream);
if (status == 0)
sitd_link_urb (ehci, urb, ehci->periodic_size << 3, stream);
- else
+ else if (status > 0) {
+ ehci_urb_done(ehci, urb, 0);
+ status = 0;
+ } else
usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
done_not_linked:
spin_unlock_irqrestore (&ehci->lock, flags);
@@ -2255,6 +2301,9 @@ static void scan_isoc(struct ehci_hcd *ehci)
}
ehci->now_frame = now_frame;
+ if (ehci->last_base == -1)
+ ehci->last_base = ehci->last_iso_frame;
+
frame = ehci->last_iso_frame;
for (;;) {
union ehci_shadow q, *q_p;
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 2822e79..279e182 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -149,6 +149,7 @@ struct ehci_hcd { /* one per controller */
unsigned intr_unlink_wait_cycle;
unsigned intr_unlink_cycle;
unsigned now_frame; /* frame from HC hardware */
+ unsigned last_base; /* previous 'last_iso_frame' */
unsigned last_iso_frame; /* last frame scanned for iso */
unsigned intr_count; /* intr activity count */
unsigned isoc_count; /* isoc activity count */
Thanks,
--
Ming Lei
--
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
