On Tue, Aug 27, 2019 at 08:43:55AM -0600, shuah wrote: > On 8/27/19 8:38 AM, shuah wrote: > > On 8/26/19 11:23 AM, Suwan Kim wrote: > > > There are bugs on vhci with usb 3.0 storage device. In USB, each SG > > > list entry buffer should be divisible by the bulk max packet size. > > > But with native SG support, this problem doesn't matter because the > > > SG buffer is treated as contiguous buffer. But without native SG > > > support, USB storage driver breaks SG list into several URBs and the > > > error occurs because of a buffer size of URB that cannot be divided > > > by the bulk max packet size. The error situation is as follows. > > > > > > When USB Storage driver requests 31.5 KB data and has SG list which > > > has 3584 bytes buffer followed by 7 4096 bytes buffer for some > > > reason. USB Storage driver splits this SG list into several URBs > > > because VHCI doesn't support SG and sends them separately. So the > > > first URB buffer size is 3584 bytes. When receiving data from device, > > > USB 3.0 device sends data packet of 1024 bytes size because the max > > > packet size of BULK pipe is 1024 bytes. So device sends 4096 bytes. > > > But the first URB buffer has only 3584 bytes buffer size. So host > > > controller terminates the transfer even though there is more data to > > > receive. So, vhci needs to support SG transfer to prevent this error. > > > > > > In this patch, vhci supports SG regardless of whether the server's > > > host controller supports SG or not, because stub driver splits SG > > > list into several URBs if the server's host controller doesn't > > > support SG. > > > > > > To support SG, vhci sets URB_DMA_MAP_SG flag in transfer_flags of > > > usbip header if URB has SG list and this flag will tell stub driver > > > to use SG list. > > > > > > vhci sends each SG list entry to stub driver. Then, stub driver sees > > > the total length of the buffer and allocates SG table and pages > > > according to the total buffer length calling sgl_alloc(). After stub > > > driver receives completed URB, it again sends each SG list entry to > > > vhci. > > > > > > If the server's host controller doesn't support SG, stub driver > > > breaks a single SG request into several URBs and submits them to > > > the server's host controller. When all the split URBs are completed, > > > stub driver reassembles the URBs into a single return command and > > > sends it to vhci. > > > > > > Moreover, in the situation where vhci supports SG, but stub driver > > > does not, or vice versa, usbip works normally. Because there is no > > > protocol modification, there is no problem in communication between > > > server and client even if the one has a kernel without SG support. > > > > > > In the case of vhci supports SG and stub driver doesn't, because > > > vhci sends only the total length of the buffer to stub driver as > > > it did before the patch applied, stub driver only needs to allocate > > > the required length of buffers using only kmalloc() regardless of > > > whether vhci supports SG or not. But stub driver has to allocate > > > buffer with kmalloc() as much as the total length of SG buffer which > > > is quite huge when vhci sends SG request, so it has overhead in > > > buffer allocation in this situation. > > > > > > If stub driver needs to send data buffer to vhci because of IN pipe, > > > stub driver also sends only total length of buffer as metadata and > > > then sends real data as vhci does. Then vhci receive data from stub > > > driver and store it to the corresponding buffer of SG list entry. > > > > > > And for the case of stub driver supports SG and vhci doesn't, since > > > the USB storage driver checks that vhci doesn't support SG and sends > > > the request to stub driver by splitting the SG list into multiple > > > URBs, stub driver allocates a buffer for each URB with kmalloc() as > > > it did before this patch. > > > > > > * Test environment > > > > > > Test uses two difference machines and two different kernel version > > > to make mismatch situation between the client and the server where > > > vhci supports SG, but stub driver does not, or vice versa. All tests > > > are conducted in both full SG support that both vhci and stub support > > > SG and half SG support that is the mismatch situation. Test kernel > > > version is 5.3-rc6 with commit "usb: add a HCD_DMA flag instead of > > > guestimating DMA capabilities" to avoid unnecessary DMA mapping and > > > unmapping. > > > > > > - Test kernel version > > > - 5.3-rc6 with SG support > > > - 5.1.20-200.fc29.x86_64 without SG support > > > > > > * SG support test > > > > > > - Test devices > > > - Super-speed storage device - SanDisk Ultra USB 3.0 > > > - High-speed storage device - SMI corporation USB 2.0 flash drive > > > > > > - Test description > > > > > > Test read and write operation of mass storage device that uses the > > > BULK transfer. In test, the client reads and writes files whose size > > > is over 1G and it works normally. > > > > > > * Regression test > > > > > > - Test devices > > > - Super-speed device - Logitech Brio webcam > > > - High-speed device - Logitech C920 HD Pro webcam > > > - Full-speed device - Logitech bluetooth mouse > > > - Britz BR-Orion speaker > > > - Low-speed device - Logitech wired mouse > > > > > > - Test description > > > > > > Moving and click test for mouse. To test the webcam, use gnome-cheese. > > > To test the speaker, play music and video on the client. All works > > > normally. > > > > > > * VUDC compatibility test > > > > > > VUDC also works well with this patch. Tests are done with two USB > > > gadget created by CONFIGFS USB gadget. Both use the BULK pipe. > > > > > > 1. Serial gadget > > > 2. Mass storage gadget > > > > > > - Serial gadget test > > > > > > Serial gadget on the host sends and receives data using cat command > > > on the /dev/ttyGS<N>. The client uses minicom to communicate with > > > the serial gadget. > > > > > > - Mass storage gadget test > > > > > > After connecting the gadget with vhci, use "dd" to test read and > > > write operation on the client side. > > > > > > Read - dd if=/dev/sd<N> iflag=direct of=/dev/null bs=1G count=1 > > > Write - dd if=<my file path> iflag=direct of=/dev/sd<N> bs=1G count=1 > > > > > > Signed-off-by: Suwan Kim <suwan.kim027@xxxxxxxxx> > > > --- > > > v7 - v8 > > > - Modify the commit log which describes URB_DMA_MAP_SG flag setting. > > > > > > v6 - v7 > > > - Move the flag set in setup_cmd_submit_pdu() of vhci_tx.c and > > > manipulate usbip header flag instead of urb->transfer_flags. > > > > > > - Remove clearing URB_DMA_MAP_SG flag in vhci_rx. > > > > setup_cmd_submit_pdu() is just for pdu and shouldn't be concerned > > about the urb. > > > > Please keep the URB_DMA_MAP_SG setting in urb->transfer_flags. > > That mean you are restoring v6 code change with the commit log > > updates from v8. > > > > I mean v6 with my comments on v6 addressed, moving setting the flag > after kalloc() and other comments. Ok. I completed test with your comment and 5.3-rc6 and will send v9. Regards Suwan Kim
