On Thu, Aug 29, 2013 at 12:23 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 28 Aug 2013, Ming Lei wrote:
>
>> Below is the approach I proposed(mentioned in another thread), which should be
>> simper than this one, any comments?
>>
>> drivers/usb/host/ehci-sched.c | 53 ++++++++++++++++++++++++++++++++++++++---
>> drivers/usb/host/ehci.h | 1 +
>> 2 files changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
>> index 83be03f..80ef95d 100644
>> --- a/drivers/usb/host/ehci-sched.c
>> +++ b/drivers/usb/host/ehci-sched.c
>> @@ -1370,6 +1370,33 @@ 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 i, end;
>> + u32 mod = ehci->periodic_size;
>> +
>> + if (ehci->last_base == -1)
>> + return 0;
>> +
>> + end = ((stream->next_uframe + span - period) >> 3) & (mod - 1);
>> + for (i = ehci->last_base; i != ehci->last_iso_frame;
>> + i = (i + 1) & (mod - 1)) {
>> + /* don't schedule URB which is behind base totally */
>> + if (end == i) {
>> + for (i = 0; i < urb->number_of_packets; i++) {
>> + urb->iso_frame_desc[i].length = 0;
>> + urb->iso_frame_desc[i].status = 0;
>> + }
>> + return 1;
>> + }
>> + if (((stream->next_uframe >> 3) & (mod - 1)) == i)
>> + ehci->last_iso_frame = i;
>> + }
>
> Why do you use a loop here? This looks like a straightforward thing to
> test: Starting from last_base, which comes first: next_uframe or
> last_iso_frame?
OK, that is simper.
>
> It's not a very good idea to change iso_frame_desc[i].length. HCDs
> aren't suppose to touch that field. Also, why set the frame
> descriptor status to 0? It should remain equal to -EXDEV, because the
> frame never gets sent over the bus.
Right.
>
> 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
No, no other TDs scheduled for the frames since the queue is empty
when iso_stream_rebase is running.
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
