On Wed, 20 Jul 2011 tom.leiming@xxxxxxxxx wrote:
> From: Ming Lei <tom.leiming@xxxxxxxxx>
>
> The current ehci ISO schedule only supports that the max irq delay
> for ehci is 20ms(see 'if (excess >= mod - 2 * SCHEDULE_SLOP)' in
> iso_stream_schedule). And if ehci irq is disabled for longer time
> than 20ms, ehci ISO schedule will behave badly and cause
> failure of "request %p would overflow".
How often does this happen? If it does happen then something is badly
wrong already. In such cases the failure ought to be easily visible --
we don't want to hide it and pretend that everything is all right.
For example, there was a case in the mailing list not long ago where
the IRQ delay was as long as 40 ms. The scheduling failure caused
people to investigate, and they found that their wifi driver was
leaving interrupts disabled much longer than it should have. Without
this obvious failure, they might never have known.
In short, it's not obvious that this problem needs to be fixed.
> This patch uses kernel time to check if iso schedule was handled
> late, and fixes the problem.
>
> Certainly, the problem can be fixed by increasing the allowable
> delay, such as:
>
> if (excess >= mod - N * SCHEDULE_SLOP) /*iso_stream_schedule*/
>
> and take a larger value of N than 2, but the fix is not feasible
> enough, also may decrease the allowable ISO schedule window, especially
> for small periodic size case(such as, 256 and 512).
>
> The method in this patch can fix all the above shortcomings, and the
> only drawback is that it may introduce extra computation on ktime.
The idea is good but the implementation could be improved.
> Signed-off-by: Ming Lei <tom.leiming@xxxxxxxxx>
> ---
> drivers/usb/host/ehci-sched.c | 17 +++++++++++++++--
> drivers/usb/host/ehci.h | 1 +
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index 6c9fbe3..1846cbe 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -1432,16 +1432,17 @@ iso_stream_schedule (
> else
> next = now;
>
> - /* Fell behind (by up to twice the slop amount)?
> + /* Fell behind (by up to any time)
If you're going to erase the main point of the parenthetical remark,
you might as well get rid of the entire remark.
> * We decide based on the time of the last currently-scheduled
> * slot, not the time of the next available slot.
> */
> excess = (stream->next_uframe - period - next) & (mod - 1);
> - if (excess >= mod - 2 * SCHEDULE_SLOP)
> + if (ktime_us_delta(stream->next_ktime, ktime_get()) < 0)
This computation can be wrong, because ktime values are not
synchronized with the EHCI controller's frame counter. Also, it
ignores the value of "next". Thus you really can't remove the
existing test.
However, it is possible to add an additional test. If the
ktime_us_delta value is < 10000 (i.e., 10 ms) then the submission is
definitely too late.
> start = next + excess - mod + period *
> DIV_ROUND_UP(mod - excess, period);
You need to change this computation too, because it's no longer
necessarily true that next + excess lies between mod and 2*mod. Get
rid of the "- mod" and instead compute "& (mod - 1)".
> else
> start = next + excess + period;
> +
> if (start - now >= mod) {
> ehci_dbg(ehci, "request %p would overflow (%d+%d >= %d)\n",
> urb, start - now - period, period,
> @@ -1591,6 +1592,17 @@ itd_link (struct ehci_hcd *ehci, unsigned frame, struct ehci_itd *itd)
> *hw_p = cpu_to_hc32(ehci, itd->itd_dma | Q_TYPE_ITD);
> }
>
> +static void get_next_uframe_ktime(struct ehci_hcd *ehci,
> + struct ehci_iso_stream *stream)
> +{
> + unsigned mod = ehci->periodic_size << 3;
> + u32 now =
> + ehci_readl(ehci, &ehci->regs->frame_index) & (mod - 1);
> +
> + stream->next_ktime = ktime_add_us(ktime_get(),
> + ((stream->next_uframe - now) & (mod - 1)) * 125);
> +}
You don't need two separate "& (mod - 1)" operations; only the second
one is necessary.
> +
> /* fit urb's itds into the selected schedule slot; activate as needed */
> static int
> itd_link_urb (
> @@ -1658,6 +1670,7 @@ itd_link_urb (
> }
> }
> stream->next_uframe = next_uframe;
> + get_next_uframe_ktime(ehci, stream);
Something like this is also needed in sitd_link_urb().
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
