Re: [PATCH 0/9] Various xhci fixes and improvements

["Michał Pecio" <michal.pecio@gmail.com>]

> We are in the middle of reworking transfer event handling with Niklas
> as well.
> 
> I'd be grateful if you could take a quick look at our solution and
> give your opinion on it as you have expertise in this area.
Beware that I may only have enough of that to be dangerous ;)

But I also have a few of those PCIe cards with various chips and all
three public revisions of the spec, so I'm trying to stay in spec and
verify things on hardware.

BTW, I only really touched isochronous transfers so far, but it looks
like some things are not entirely right in others too. The handling of
short packets for instance - it looks like TRB_ISP is set on various
transfers, but process_xxx_td() functions don't care if the event is
mid-TD and give back the URB immediately.

I though about fixing this (I have a naive hope that some xHC crashes
that I see are "undefined behavior" caused by driver bugs, even though
in reality the hardware is probably simply FUBAR) but I'd need to read
more spec chapters to get there.

> Main idea is simple.
> If a transfer event points to a TRB between dequeue and enqueue we
> give back all queued TDs passed up to this TRB.
> And if the event points to a valid TD then handle that. (normal case)
> 
> Code is much simpler, we get rid of skip flag, and could probably
> merge error_mid_td and urb_length_set flags into one flag.
Yes, lots of things could be better if the code walked the ring from
the last known dequeue to the current one and analyzed what's there.
We could know if the event is for something that we have already freed
(a bug?), or for any known pending TD and which one exactly, or for a
non-transfer TD (like Forced Stop Event).

Many of those thing are currently detected with heuristics which only
work about right if everything goes as planned (no bugs in HW or SW).

And first find the TD, then worry how to handle it. I like that Niklas
managed to shorten this giant loop already, and my patches #2 and #3
shorten it further and move TD selection logic up (#2) and all else
down (#3). This way we can work on selection and handling separately,
with less worrying about how they influence each other.

> The code isn't complete or tested yet. It doesn't handle
> under/overruns, but that could probably be fixed by just turning
> "return 0" for those cases into "break"
Problem is that not all events give valid dequeue pointers. These two,
for example, are not guaranteed to. Then we have no idea where on the
ring the HC currently is. The EP doesn't stall and it may have already
been resumed by a simple doorbell ring.

Regards,
Michal