Re: [PATCH v8] usbip: Implement SG support to vhci-hcd and stub driver

[Suwan Kim]

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