Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Suwan Kim <suwan.kim027@gmail.com>
Cc: shuah@kernel.org, <valentina.manea.m@gmail.com>,
	<gregkh@linuxfoundation.org>, <linux-usb@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] usbip: Implement SG support to vhci
Date: Fri, 21 Jun 2019 16:05:24 -0400 (EDT)
Message-ID: <Pine.LNX.4.44L0.1906211548560.1471-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <20190621174553.28862-3-suwan.kim027@gmail.com>

On Sat, 22 Jun 2019, Suwan Kim wrote:

> There are bugs on vhci with usb 3.0 storage device. Originally, vhci
> doesn't supported SG. So, USB storage driver on vhci divides SG list
> into multiple URBs and it causes buffer overflow on the xhci of the
> server. So we need to add SG support to vhci

It doesn't cause buffer overflow.  The problem was that a transfer got
terminated too early because the transfer length for one of the URBs
was not divisible by the maxpacket size.

> In this patch, vhci basically support SG and it sends each SG list
> entry to the stub driver. Then, the stub driver sees total length of
> the buffer and allocates SG table and pages according to the total
> buffer length calling sgl_alloc(). After the stub driver receives
> completed URB, it again sends each SG list entry to the vhci.
> 
> If HCD of server doesn't support SG, the stub driver allocates
> big buffer using kmalloc() instead of using sgl_alloc() which
> allocates SG list and pages.

You might be better off not using kmalloc.  It's easier for the kernel 
to allocate a bunch of small buffers than a single big one.  Then you 
can create a separate URB for each buffer.

> Alan fixed vhci bug with the USB 3.0 storage device by modifying
> USB storage driver.
> ("usb-storage: Set virt_boundary_mask to avoid SG overflows")
> But the fundamental solution of it is to add SG support to vhci.
> 
> This patch works well with the USB 3.0 storage devices without Alan's
> patch, and we can revert Alan's patch if it causes some troubles.

These last two paragraphs don't need to be in the patch description.

> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---

I'm not sufficiently familiar with the usbip drivers to review most of 
this.  However...

> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index be87c8a63e24..cc93c1a87a3e 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -696,7 +696,8 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  	}
>  	vdev = &vhci_hcd->vdev[portnum-1];
>  
> -	if (!urb->transfer_buffer && urb->transfer_buffer_length) {
> +	if (!urb->transfer_buffer && !urb->num_sgs &&
> +	     urb->transfer_buffer_length) {
>  		dev_dbg(dev, "Null URB transfer buffer\n");
>  		return -EINVAL;
>  	}
> @@ -1142,6 +1143,11 @@ static int vhci_setup(struct usb_hcd *hcd)
>  		hcd->speed = HCD_USB3;
>  		hcd->self.root_hub->speed = USB_SPEED_SUPER;
>  	}
> +
> +	/* support sg */
> +	hcd->self.sg_tablesize = ~0;
> +	hcd->self.no_sg_constraint = 1;

You probably shouldn't do this, for two reasons.  First, sg_tablesize
of the server's HCD may be smaller than ~0.  If the client's value is
larger than the server's, a transfer could be accepted on the client
but then fail on the server because the SG list was too big.

Also, you may want to restrict the size of SG transfers even further,
so that you don't have to allocate a tremendous amount of memory all at
once on the server.  An SG transfer can be quite large.  I don't know 
what a reasonable limit would be -- 16 perhaps?

Alan Stern


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-21 17:45 [PATCH 0/2] " Suwan Kim
2019-06-21 17:45 ` [PATCH 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci Suwan Kim
2019-06-29  0:11   ` shuah
2019-07-01  9:29     ` Suwan Kim
2019-06-21 17:45 ` [PATCH 2/2] usbip: Implement SG support to vhci Suwan Kim
2019-06-21 20:05   ` Alan Stern [this message]
2019-06-24 14:58     ` Suwan Kim
2019-06-24 17:24       ` Alan Stern
2019-07-04 17:24         ` Suwan Kim
2019-07-05  1:41           ` Alan Stern
2019-07-05  9:07             ` Suwan Kim
2019-06-22 10:40   ` kbuild test robot

Reply instructions:

You may reply publically 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=Pine.LNX.4.44L0.1906211548560.1471-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=shuah@kernel.org \
    --cc=suwan.kim027@gmail.com \
    --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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git