Bug in split transactions on Raspberry Pi

[Doug Anderson]

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.


-Doug