Bug in split transactions on Raspberry Pi

[Doug Anderson]

Alan,

On Wed, Jan 27, 2016 at 11:21 AM, Doug Anderson <dianders at chromium.org> wrote:
> Alan,
>
> On Wed, Jan 27, 2016 at 11:03 AM, Alan Stern <stern at rowland.harvard.edu> wrote:
>> On Tue, 26 Jan 2016, Doug Anderson wrote:
>>
>>> > This probably indicates that the list of patches was incomplete (i.e.,
>>> > some other patches were applied before these).  Also, the patches
>>> > aren't listed in order of the patchwork IDs -- which order is correct?
>>>
>>> The order like 01, 02, 03, etc is correct.  Patchwork numbers things
>>> in a random order (depending on the vagaries of SMTP and which one
>>> arrives to their server first).
>>>
>>> Ah, I had tried linux/master recently, but that has a patch that's not in v4.4:
>>>
>>> fbb9e22b15ad usb: dwc2: host: enable descriptor dma for fs devices
>>>
>>>
>>> I'd bet that you don't have descriptor DMA turned on anyway, so the
>>> descriptor DMA patches won't really matter (they'll just be noops).
>>> ...but you could pick all the patches up to that one to avoid
>>> conflicts.
>>>
>>> fbb9e22b15ad usb: dwc2: host: enable descriptor dma for fs devices
>>> 762d3a1a9cd7 usb: dwc2: host: process all completed urbs
>>> 3f808bdae75e usb: dwc2: host: always increment available host channel
>>> during release
>>> c17b337c1ea4 usb: dwc2: host: program descriptor for next frame
>>> b9392d9920fd usb: dwc2: host: add function to compare frame index
>>> 2b046bc5aaef usb: dwc2: host: spinlock release channel
>>> 26a19ea69906 usb: dwc2: host: fix use of qtd after free in desc dma mode
>>> c503b3815385 usb: dwc2: host: rework isochronous halt path
>>> dde4c1bf5df0 usb: dwc2: host: set active bit in isochronous descriptors
>>> 3ac38d260fa5 usb: dwc2: host: ensure filling of isoc desc is correctly done
>>
>> In the end I just used the contents of the dwc2 directory from Linus's
>> current tree -- I don't think it has changed since 4.5-rc1.  Your
>> patches applied on top of that with no issues.
>>
>> They seem to work.  Is there anything in particular you would like me
>> to test?
>
> I was sorta wondering if it fixed the original problem you were
> seeing?  Unless I missed it it sounded like it was still broken on ToT
> and I was wondering if my series happened to help in any way...
>
> Other than that the series mostly improves compatibility with splits,
> especially when many periodic devices are plugged in (like a Full
> Speed USB audio device plugged in at the same time as several
> keyboards).
>
>
>> One problem I noticed: The controller does hub status polling to an
>> external high-speed hub much too frequently.  The interrupt endpoint's
>> bInterval value is 12, meaning the polling interval should be 256 ms.
>> But when data was available, the polls were issued every 2 microframes
>> instead of every 2048.
>>
>> Could the driver be rescheduling the interrupt endpoint every time an
>> URB completes and a new one is submitted?  That's what it looks like.
>> This might be related to the giving-URBs-back-in-a-tasklet change.
>
> Hrm.  That started with this series?  OK, I'll take a look and see if
> it can reproduce.
>
> Oh, wait, I think I know what this is.  This is probably ("usb: dwc2:
> host: Manage frame nums better in scheduler").  I'd bet that if you go
> before that change then the behavior will be the way it was.
>
> In said patch you can see that dwc2_schedule_periodic() will now
> always pick a new first frame.  As I understand it that will happen if
> there was ever a short period of time when we weren't scheduled and I
> was worried about the next slice being in the past.  It wouldn't hurt
> to change the test to only pick a new first frame if the existing
> first frame is in the past.  That would probably fix your problem.
>
> Question: is the behavior you're seeing illegal / undesirable?  It
> only happens when there's data available, right?  My current
> (rewritten) reservation algorithm in dwc2 is pretty crude  From a high
> speed device perspective it reserves a timeslot for the EP in every
> microframe even if the EP has a long interval.  It won't typically
> schedule the EP more often than every "interval" microframes, but the
> reservation is there.  That means we can't support quite as many
> devices at the same time, but it has the advantage that if we screw up
> (like if we miss an SOF) we know we can actually try to make up for it
> quickly and we won't collide with anyone else.  The whole thing also
> simplifies the scheduler.  In your particular case, we used our
> existing reservation to schedule the device a little sooner than
> requested.

This patch should fix ya.

FIXUP: FROMLIST: usb: dwc2: host: Manage frame nums better in scheduler
https://chromium-review.googlesource.com/324185


The series is starting to get enough comments that I'm tempted to send
up a new version, but I'm always a little reluctant to post 21 patches
again if more feedback is coming.

Unless I hear otherwise, I'll plan to post up a new version with
accumulated fixes / tags by Friday so that a new version is ready for
John on Monday.


Thank you very much for your testing and problem reports.  It is very
much appreciated!

-Doug