Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 8 Aug 2019 23:21:39 +0900
From:   Suwan Kim <suwan.kim027@gmail.com>
To:     shuah@kernel.org, valentina.manea.m@gmail.com,
        gregkh@linuxfoundation.org, stern@rowland.harvard.edu
Cc:     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
Message-ID: <20190808142139.GA4761@localhost.localdomain>
References: <20190806123154.23798-1-suwan.kim027@gmail.com>
 <20190806123154.23798-3-suwan.kim027@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20190806123154.23798-3-suwan.kim027@gmail.com>
User-Agent: Mutt/1.12.0 (2019-05-25)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 06, 2019 at 09:31:54PM +0900, 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
> 
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
> v3 - v4
> 
> - Rewrite the description about the vhci bug with USB 3.0 storage
>   device.
> - Add the description about the test with VUDC.
> - Fix the error message in stub_recv_cmd_unlink().
> 
> v2 - v3
> 
> - Rewrite the commit log to make it more clear.
> - Add the description about the mismatch situation in commit log.
> - Run chechpatch.pl and fix errors with coding style and typos.
> - Fix the error path of usbip_recv_xbuff() to stop receiving data
>   after TCP error occurs.
> - Consolidate the duplicated error path in usbip_recv_xbuff() and
>   vhci_send_cmd_submit().
> - Undo the unnecessary changes
>   * Undo the deletion of empty line in stub_send_ret_submit()
>   * Move memset() lines in stub_send_ret_submit() to the original
>     position.
> 
> v1 - v2
> 
> - Add the logic that splits single SG request into several URBs in
>   stub driver if server’s HC doesn’t support SG.
> ---
>  drivers/usb/usbip/stub.h         |   7 +-
>  drivers/usb/usbip/stub_main.c    |  55 ++++++---
>  drivers/usb/usbip/stub_rx.c      | 204 ++++++++++++++++++++++---------
>  drivers/usb/usbip/stub_tx.c      |  99 +++++++++++----
>  drivers/usb/usbip/usbip_common.c |  59 ++++++---
>  drivers/usb/usbip/vhci_hcd.c     |  15 ++-
>  drivers/usb/usbip/vhci_tx.c      |  63 ++++++++--
>  7 files changed, 376 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/usb/usbip/stub.h b/drivers/usb/usbip/stub.h
> index 35618ceb2791..d11270560c24 100644
> --- a/drivers/usb/usbip/stub.h
> +++ b/drivers/usb/usbip/stub.h
> @@ -52,7 +52,11 @@ struct stub_priv {
>  	unsigned long seqnum;
>  	struct list_head list;
>  	struct stub_device *sdev;
> -	struct urb *urb;
> +	struct urb **urbs;
> +	struct scatterlist *sgl;
> +	int num_urbs;
> +	int completed_urbs;
> +	int urb_status;
>  
>  	int unlinking;
>  };
> @@ -86,6 +90,7 @@ extern struct usb_device_driver stub_driver;
>  struct bus_id_priv *get_busid_priv(const char *busid);
>  void put_busid_priv(struct bus_id_priv *bid);
>  int del_match_busid(char *busid);
> +void stub_free_priv_and_urb(struct stub_priv *priv);
>  void stub_device_cleanup_urbs(struct stub_device *sdev);
>  
>  /* stub_rx.c */
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index 2e4bfccd4bfc..3ee636215e9c 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -6,6 +6,7 @@
>  #include <linux/string.h>
>  #include <linux/module.h>
>  #include <linux/device.h>
> +#include <linux/scatterlist.h>
>  
>  #include "usbip_common.h"
>  #include "stub.h"
> @@ -288,6 +289,42 @@ static struct stub_priv *stub_priv_pop_from_listhead(struct list_head *listhead)
>  	return NULL;
>  }
>  
> +void stub_free_priv_and_urb(struct stub_priv *priv)
> +{
> +	struct urb *urb;
> +	int i;
> +
> +	for (i = 0; i < priv->num_urbs; i++) {
> +		urb = priv->urbs[i];
> +
> +		if (!urb)
> +			return;
> +
> +		kfree(urb->setup_packet);
> +		urb->setup_packet = NULL;
> +
> +
> +		if (urb->transfer_buffer && !priv->sgl) {
> +			kfree(urb->transfer_buffer);
> +			urb->transfer_buffer = NULL;
> +		}
> +
> +		if (urb->num_sgs) {
> +			sgl_free(urb->sg);
> +			urb->sg = NULL;
> +			urb->num_sgs = 0;
> +		}
> +
> +		usb_free_urb(urb);
> +	}
> +
> +	list_del(&priv->list);

I find list_del() error in my implementation. When client closes
connection with server, stub driver calls stub_device_cleanup_urbs()
to free all the privs and urbs. stub_device_cleanup_urbs() takes
stub_priv from the lists calling stub_priv_pop() and calls
stub_free_priv_and_urb() to free priv and urbs. But the error occurs
when stub_free_priv_and_urb() tries to call list_del() to delete
stub priv from the list because it was already deleted from the
list in stub_priv_pop().

> +	if (priv->sgl)
> +		sgl_free(priv->sgl);
> +	kfree(priv->urbs);
> +	kmem_cache_free(stub_priv_cache, priv);
> +}
> +
>  static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
>  {
>  	unsigned long flags;
> @@ -314,25 +351,15 @@ static struct stub_priv *stub_priv_pop(struct stub_device *sdev)
>  void stub_device_cleanup_urbs(struct stub_device *sdev)
>  {
>  	struct stub_priv *priv;
> -	struct urb *urb;
> +	int i;
>  
>  	dev_dbg(&sdev->udev->dev, "Stub device cleaning up urbs\n");
>  
>  	while ((priv = stub_priv_pop(sdev))) {
> -		urb = priv->urb;
> -		dev_dbg(&sdev->udev->dev, "free urb seqnum %lu\n",
> -			priv->seqnum);
> -		usb_kill_urb(urb);
> +		for (i = 0; i < priv->num_urbs; i++)
> +			usb_kill_urb(priv->urbs[i]);
>  
> -		kmem_cache_free(stub_priv_cache, priv);
> -
> -		kfree(urb->transfer_buffer);
> -		urb->transfer_buffer = NULL;
> -
> -		kfree(urb->setup_packet);
> -		urb->setup_packet = NULL;
> -
> -		usb_free_urb(urb);
> +		stub_free_priv_and_urb(priv);
>  	}
>  }

I modified stub_free_priv_and_urb() and stub_device_cleanup_urbs()
to support SG. In stub_device_cleanup_urbs(), the code is the same
as stub_free_priv_and_urb() except that it does not call list_del()
when freeing stub priv and urbs. So I removed the duplicated code
and make stub_device_cleanup_urbs() call stub_free_priv_and_urb()
to free stub priv and urbs. But when calling stub_free_priv_and_urb()
in stub_device_cleanup_urbs(), it should avoid calling list_del()
but it calls. I will fix it up and send v5.

Regards
Suwan Kim