Messages in this thread | | | Subject | Re: [PATCH v8] usbip: Implement SG support to vhci-hcd and stub driver | From | shuah <> | Date | Tue, 27 Aug 2019 08:43:55 -0600 |
| |
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@gmail.com> >> --- >> 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.
thanks, -- Shuah
| |