linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Suwan Kim <suwan.kim027@gmail.com>
To: shuah <shuah@kernel.org>
Cc: valentina.manea.m@gmail.com, gregkh@linuxfoundation.org,
	stern@rowland.harvard.edu, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] usbip: Implement SG support to vhci-hcd and stub driver
Date: Wed, 7 Aug 2019 00:48:56 +0900	[thread overview]
Message-ID: <20190806154856.GB3738@localhost.localdomain> (raw)
In-Reply-To: <eeb617a1-e0e0-f27e-f8c7-e0180bb6e911@kernel.org>

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.

Do I add test description about the USB3 super-speed and USB2
high-speed device to the commit log?

Regards
Suwan Kim

  reply	other threads:[~2019-08-06 15:49 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-06 12:31 [PATCH v4 0/2] usbip: Implement SG support Suwan Kim
2019-08-06 12:31 ` [PATCH v4 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci Suwan Kim
2019-08-06 15:11   ` shuah
2019-08-06 15:32     ` Suwan Kim
2019-08-06 15:38       ` shuah
2019-08-06 15:52         ` Suwan Kim
2019-08-06 12:31 ` [PATCH v4 2/2] usbip: Implement SG support to vhci-hcd and stub driver Suwan Kim
2019-08-06 15:13   ` shuah
2019-08-06 15:48     ` Suwan Kim [this message]
2019-08-06 17:27       ` shuah
2019-08-08 14:21   ` Suwan Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190806154856.GB3738@localhost.localdomain \
    --to=suwan.kim027@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=shuah@kernel.org \
    --cc=stern@rowland.harvard.edu \
    --cc=valentina.manea.m@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).