Messages in this thread | | | Subject | Re: [PATCH v4 2/2] usbip: Implement SG support to vhci-hcd and stub driver | From | shuah <> | Date | Tue, 6 Aug 2019 11:27:36 -0600 |
| |
On 8/6/19 9:48 AM, Suwan Kim wrote: > On Tue, Aug 06, 2019 at 09:13:54AM -0600, shuah wrote: >> On 8/6/19 6:31 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_map_urb_for_dma() sets URB_DMA_MAP_SG flag in >>> urb->transfer_flags 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 regardless of whether vhci supports >>> SG or not. >>> >>> 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. >>> >>> In the case of stub driver that supports SG, buffer is allocated by >>> sgl_alloc(). However, stub driver that does not support SG allocates >>> buffer using only kmalloc(). Therefore, if vhci supports SG and stub >>> driver doesn't, 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 big overhead in buffer allocation. >>> >>> 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 with kmalloc() as it did before >>> this patch. >>> >>> 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 >>> >> >> Thanks for the test results. >> >> Were you able to test with USB lowspeed devices? > > I tested USB3 super-speed device and USB2 high-speed device. > But I don't have full/low speed device that uses BULK transfer. > In USB spec, low-speed device doesn't support BULK transfer. >
I think I mentioned earlier that the reason to test with lowspeed is make sure there are no regressions due this change. You aren't testing BULK transfer, you are looking for any regressions.
> Do I add test description about the USB3 super-speed and USB2 > high-speed device to the commit log? >
Please do.
thanks, -- Shuah
| |