I opened a pull request for this patch https://github.com/torvalds/linux/pull/388 Hope it could restart the discussion Peace and love, Raz -----Original Message----- From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] Sent: Thursday, January 5, 2017 9:08 PM To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> Cc: Raz Manor <Raz.Manor@xxxxxxxxxx>; Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>; USB list <linux-usb@xxxxxxxxxxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Tim Harvey <tharvey@xxxxxxxxxxxxx>; Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>; Jussi Kivilinna <jussi.kivilinna@xxxxxxxxxxx>; Colin Ian King <colin.king@xxxxxxxxxxxxx> Subject: Re: net2280 Driver Bug On Thu, 5 Jan 2017, Greg Kroah-Hartman wrote: > On Tue, Dec 27, 2016 at 03:56:11PM +0000, Raz Manor wrote: > > 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. > > What ever happened to this? > > Alan, any thoughts about this change? Sorry, I haven't had time to look at this, and I probably won't have time in the near future. The existing code looks obviously wrong, but the exact change required to fix it isn't entirely clear. Besides, I'm not acquainted with the USB3380 card, only the older NET2280 (which supports USB-2 but not USB-3). I suggested to Raz that he should CC: the people who added USB3380 support to the driver. They are more likely to have the specs than anyone else. I can test a patch with my card, if Raz wants to post one. Alan Stern > thanks, > > greg k-h -- 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
