On 28.01.21 at 15:14 Mathias Nyman wrote:
If I'm understanding it correctly, you're always creating a bounce
buffer though it is not necessary (at least in my case - my test patch
proofed, that no changes at all are necessary). Why aren't you checking
for URB_NO_TRANSFER_DMA_MAP at the very beginning? Or is it your purpose
to first basically test your new code path? That would be ok.
Bounce buffer is only used when there is no other way of meeting the xHCI
alignment requirements. Worst case is once every 255 TRB, and for maximum 1024bytes.
Each TRB can point to 64K of data.
This driver is from ~2012 - I'm not sure if they already considered USB 3.x at
this time (comments to USB 2 and 1 can be found). *I* could not find any 512 Bytes
related alignment code - but that doesn't say much. I tried to map the xHCI spec
to the Linux urb API - I'm not sure how to map TRBs or TDs. Could a urb be
understood as a TRB?
I can't go against the spec just because it doesn't cause issues in your
two usecases. The overhead of the bounce buffer usage is so small there's no
point in that kind of optimization.
That's why I am a bit unsure at the moment regarding the result of the test. As I
meanwhile know that it's working even w/o doing anything (by chance?), I can't say
for sure that your patch is working as expected. The only thing I can say, is (see
below): it doesn't hurt any more.
URB_NO_TRANSFER_DMA_MAP only indicates the data was DMA mapped before driver
submitted the URB, so usb core does not need to map it.
I was speculating that it could be a cause why data is so oddly aligned
(urbs with data starting at less than 512 bytes from 64k boundary), and
thus not meeting the xHC boundary and aligment requirements even if data
is otherwise contiguous.
xHC controller doesn't care who mapped the data, or if data is in a sg list
or already contiguous in urb->transfer_buffer as long as we follow
the aligment and boudary rules.
Bug was that driver assumed the data that needed to be bounce
buffered was in a sg list. Your cased proved it could be in
urb->transfer_buffer instead.
That's true for sure: the driver fills the data into transfer_buffer (it uses
usb_fill_bulk_urb).
I tested with the notebook (in both directions) - it seems to work - I
didn't get any problems though I used 24 kB bulk packets. Throughput was
unaltered high.
I'm doing the same test tomorrow with the other USB 3.1 controller!
Successfully tested on the other USB 3.1 host.
Thank you, much appreciated.
I think I'll submit this patch as it is (with a proper commit message)
can I add reported-by: Andreas Hartmann <andihartmann@xxxxxxxxxxxxxxx>, and
tested-by: Andreas Hartmann <andihartmann@xxxxxxxxxxxxxxx> tags to the patch?
That's OK for me.
Thanks
Andreas
