Re: Possible bug in xhci-ring.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Maciej, that was exactly what I needed! Looks like the problem
in your situation is that last_trb is actually (chronologically)
before hw_dequeue, which the code doesn't expect. You enqueue a
single-TRB-TD at 2f45e080 (so last_trb points there as well), which
encounters a Stall Error. I would guess that the Event TRB for that
error still returns 2f45e080 in its TRB Pointer (which the old code
stored in the stopped_trb variable that I removed), but the hw_dequeue
value we read from the Endpoint Context is 2f45e090.

I don't see this behavior on a DesignWare and an Intel xHC that I have
here, so I guess your Asmedia host controller is simply quirky/broken.
I think it should really store the failed TRB in the TR Dequeue
Pointer of the Endpoint Context when encountering an error, and not
just advance to the next TRB on its own... but I have a hard time
finding a decisive rule for that in the XHCI spec right now (it makes
this clear for the Stop Endpoint Command, but not really for errors).
Sarah, Mathias, do you know if this is specified somewhere or is it
really left up to the implementation?

Nevertheless, we should try to accommodate for this somehow. I think
the only way to catch it is to already look for last_trb on first
search for hw_dequeue, and cycle the bit once more if we encounter it
first (to counteract the incorrect cycling later on). Unfortunately
that will make this whole thing yet more complicated, but it should
work. I'll try to submit a patch tomorrow, but you will need to test
it on that host controller (for both 3.2 and later).

> Some additional thoughts:
>
>> The piece of code you pointed out only affects single-segment
>> transfer rings. I think the kernel generally switched to using
>> multi-segment transfer rings for everything in commit 2fdcd47b69
>> "xHCI: Allocate 2 segments for transfer ring", which explains why
>> the problem doesn't affect kernels after 3.2
>
> Does this mean that in kernels after 3.2 the problematic code line
> (that toggles new_cycle_state) is never executed, i.e. dead code?

Well... yes, it's never executed, but I wouldn't call it dead code.
It's just coincidence that the kernel currently doesn't use
single-segment rings, there's no intrinsic reason for it and it might
change in the future. I think it's still a good idea to keep the case
working.

Also, really the only reason later kernels work for that controller is
because we don't handle the case where a TD wraps all the way around a
whole multi-segment ring back to the same segment. We should do the
extra cycle in that case as well (and a normal transfer would look
like that on your controller), so I think we should fix both of these
bug and make sure the cycle state is correct in every conceivable
setup.
--
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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux