From: Sarah Sharp
> Hi David,
>
> I've been thinking about this some more, and I'd like to propose a much
> simpler solution.
>
> The TD fragment rules didn't go into the xHCI specification until the
> 1.0 revision. The code that follows those rules only seems to trigger
> issues for 0.96 ASMedia hosts, while Intel 1.0 hosts benefit from the
> code when USB ethernet devices are attached. The patch that changed the
> link TRB behavior (commit 6c12db90f19727c76990e7f4801c67a148b30111) was
> submitted in 2010, and the xHCI 1.0 spec didn't come out until later
> that year, so it's unlikely that Synopsys host is a 1.0 host either.
Ah - so I was right in thinking that a LINK TRB mustn't point to
a TRB that is still owned by the driver.
The thing is, that patch has a timing bug, and I think that is what Walt
is hitting. It is there regardless of my NOP change.
It might be hit by doing back to back single sector transfers.
Without the NOP patch it is unlikely that the transfers will start with
the LINK TRB - since they all use a moderate number of TRB.
Even writing the NOP TRB perturbs the timing slightly - although they
get added quite quickly.
The problem is that, although the 6c12 patch stopped inc_enq() changing
the ownership of LINK TRB, it left prepare_ring() doing so.
This means that there is a finite time where the last TRB is a LINK TRB.
The more fragments a transfer has, the longer this is true.
Clearly this is only a problem if the controller is actually active.
So you might think that you need to be setting up a transfer just as an
earlier transfer is completing. However I think the timings are such
that you can be setting up the transfer in response to the earlier
transfer completing.
This is what makes the error machine dependant.
So this fix is actually just a version of the 6c12 patch that
actually works!
It would look a lot less invasive if you compared it to the code
before that patch was applied.
> So why not just limit your code (and the sg table entry limit) to only
> trigger for 1.0 hosts? That fix would be much less complex than this
> one. I would still consider the other cleanup patches on to of it for
> inclusion in 3.15, although it looks like patches 4 and 5 rely on this
> patch.
>
> Walt, can you turn on xHCI debugging and look for whether the NEC host
> that worked with David's patch is a 1.0 host?
In order to hit the bug I've fixed you need two things:
1) An xhci controller that doesn't like 'dangling' LINK TRB.
(These will be the ones that made the 6c12 fix needed.)
and:
2a) A host system where the timings of the PCIe transfers (especially
the latency) are such that the controller manages to read a
LINK TRB that prepare_transfer() has updated while queueing a
new transfer in response to a transfer completion event.
or:
2b) To setup a second transfer just as the last transfer is completing.
I'm not sure this can happen for disks, but it could happen for
network traffic.
So that fact that any given controller works in a specific system
doesn't mean it will work in any system.
I suspect that any that don't like dangling LINKs will fail
under certain workloads on some systems.
I will look at a patch the mitigates the problems for disk requests.
David
--
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
