I'm not entirely sure either, that is why I wanted the specs.
Alan sent me the PLX NET2280 specs, but I'm using the USB3380/USB3381 - the
USB3 version of the device.
As it seems from the code, the HW interface is mostly the same, with some minor
changes, so it is the same driver (net2280.c) but with some branches for
USB3380 when needed.
Maybe the problem has something to do with a minor difference between the
NET2280 and USB3380, that was over looked when adding the support for the
USB3380.
So, if any of you have the specs for that one, I would love to get them and see
if there is such a difference in that area.
Anyway, as for the proposed path, this was my logic:
1. The req->td->dmadesc equals to 0 iff:
-- There was a transaction ending with a short packet, and
-- The read() to read it was shorter than the transaction length, and
-- The read() to complete it is longer than the residue.
I believe this is true from the printouts of various cases, but I can't be
positive it is correct.
2. Entering this if, there should be no more data in the endpoint (a short
packet terminated the transaction). If there is, the transaction wasn't really
done and we should exit and wait for it to finish entirely. That is the inner
if.
That inner if should never happen, but it is there to be on the safe side.
That is why it is marked with the comment /* paranoia */.
The size of the data available in the endpoint is ep->dma->dmacount and it
is read to tmp.
This entire clause is based on my own educated guesses.
3. If we passed that inner if without breaking in the original code, than tmp &
DMA_BYTE_MASK_COUNT== 0.
That means we will always pass dma bytes count of 0 to dma_done(), meaning
all the requested bytes were read.
4. dma_done() reports back to the upper layer that the request (read()) was
done and how many bytes were read. In the original code that would always be
the request size, regardless of the actual size of the data.
That did not make sense to me at all.
5. However, the original value of tmp is req->td->dmacount, which is the
dmacount value when the request's dma transaction was finished. And that is a
much more reasonable value to report back to the caller.
As you can see, this is based a lot on educated guesses and debug printouts of
various cases. That is why I would like to get your input on this, to make sure
I'm on the right track.
To recreate the problem., try reading from a bulk out endpoint in a loop, 1024
* n bytes in each iteration. Connect the PLX to a host you can control, and
send to that endpoint 1024 * n +x bytes such that 0 < x < 1024 * n and (x %
1024) != 0
You would expect the first read() to return 1024 * n and the second read to
return x, but you will get the first read to return 1024 *n and the second one
to return 1024 * n.
That is true for every positive integer n.
My patch, solves the problem, and does not break any of the other cases I've
tried.
To conclude:
I would love to get your input on this patch, as it is based on personal
debugging and guesses, without knowing the HW specs.
I would also love to get the USB3380 specs, so I could verify some aspects of
this problem myself, and for future references.
Peace and love and marry Christmas and happy Hanukah,
Raz
-----Original Message-----
From: Alan Stern [mailto:[email protected]]
Sent: Tuesday, December 27, 2016 5:06 PM
To: Linus Torvalds <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; USB list
<[email protected]>; Felipe Balbi <[email protected]>; Raz Manor
<[email protected]>; Tim Harvey <[email protected]>; Heikki Krogerus
<[email protected]>; Jussi Kivilinna
<[email protected]>; Colin Ian King <[email protected]>
Subject: RE: net2280 Driver Bug
On Mon, 26 Dec 2016, Linus Torvalds wrote:
> On Dec 26, 2016 4:04 PM, "Alan Stern" wrote:
>
>
> Note that the usage of tmp in the "if (unlikely(req->td->dmadesc == 0)) {"
> branch really is not conflicting, because that branch breaks out of
> the enclosing "while" loop.
>
>
> No.
>
> Look closer.
>
> It does indeed breast out of the loop, but before it dies that, it
> uses "tmp" again. And it wants the *old* tmp, not the new one..
Are you certain that it doesn't want the new value of tmp? How can you tell?
-- it was not immediately obvious to me. This was what I had in mind when I
wrote that it wasn't clear whether your patch was entirely correct.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
