> 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
