[PATCH v2 0/2] usbip: Implement SG support

[PATCH v2 0/2] usbip: Implement SG support

[Suwan Kim]

There are bugs on vhci with usb 3.0 storage device. Originally, vhci
doesn't supported SG, so USB storage driver on vhci breaks SG list
into multiple URBs and it causes error that a transfer got terminated
too early because the transfer length for one of the URBs was not
divisible by the maxpacket size.

To support SG, vhci doesn't map and unmap URB for DMA to use native
SG list (urb->num_sgs). In DMA mapping function of vhci, it sets
URB_DMA_MAP_SG flag in urb->transfer_flags if URB has SG list and
this flag will tell the stub driver to use SG list.

In this patch, vhci basically support SG and it sends each SG list
entry to the stub driver. Then, the 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 the stub driver receives
completed URB, it again sends each SG list entry to vhci.

If HCD of the server doesn't support SG, the stub driver breaks a
single SG reqeust into several URBs and submit them to the server's
HCD. When all the split URBs are completed, the stub driver
reassembles the URBs into a single return command and sends it to
vhci.

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.

Suwan Kim (2):
  usbip: Skip DMA mapping and unmapping for urb at vhci
  usbip: Implement SG support to vhci

 drivers/usb/usbip/stub.h         |   7 +-
 drivers/usb/usbip/stub_main.c    |  52 +++++---
 drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
 drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
 drivers/usb/usbip/usbip_common.c |  60 +++++++--
 drivers/usb/usbip/vhci_hcd.c     |  29 ++++-
 drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
 7 files changed, 391 insertions(+), 121 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci

[Suwan Kim]

vhci doesn’t do DMA for remote device. Actually, the real DMA
operation is done by network card driver. vhci just passes virtual
address of the buffer to the network stack, so vhci doesn’t use and
need dma address of the buffer of the URB.

When it comes to supporting SG for vhci, it is useful to use native
SG list (urb->num_sgs) instead of mapped SG list because DMA mapping
fnuction can adjust the number of SG list (urb->num_mapped_sgs).

But HCD provides DMA mapping and unmapping function by default.
Moreover, it causes unnecessary DMA mapping and unmapping which
will be done again at the NIC driver and it wastes CPU cycles.
So, implement map_urb_for_dma and unmap_urb_for_dma function for
vhci in order to skip the DMA mapping and unmapping procedure.

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 the
stub driver to use SG list.

Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
 drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 000ab7225717..14fc6d9f4e6a 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
 	return 0;
 }
 
+static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+			gfp_t mem_flags)
+{
+	dev_dbg(hcd->self.controller, "vhci does not map urb for dma\n");
+
+	if (urb->num_sgs)
+		urb->transfer_flags |= URB_DMA_MAP_SG;
+
+       return 0;
+}
+
+static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
+{
+       dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");
+}
+
 static const struct hc_driver vhci_hc_driver = {
 	.description	= driver_name,
 	.product_desc	= driver_desc,
@@ -1304,6 +1320,9 @@ static const struct hc_driver vhci_hc_driver = {
 
 	.get_frame_number = vhci_get_frame_number,
 
+	.map_urb_for_dma = vhci_map_urb_for_dma,
+	.unmap_urb_for_dma = vhci_unmap_urb_for_dma,
+
 	.hub_status_data = vhci_hub_status,
 	.hub_control    = vhci_hub_control,
 	.bus_suspend	= vhci_bus_suspend,
-- 
2.20.1

Re: [PATCH v2 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci

[shuah]

Hi Suwan,

On 7/5/19 10:43 AM, Suwan Kim wrote:
> vhci doesn’t do DMA for remote device. Actually, the real DMA
> operation is done by network card driver. vhci just passes virtual
> address of the buffer to the network stack, so vhci doesn’t use and
> need dma address of the buffer of the URB.
> 
> When it comes to supporting SG for vhci, it is useful to use native
> SG list (urb->num_sgs) instead of mapped SG list because DMA mapping
> fnuction can adjust the number of SG list (urb->num_mapped_sgs).
> 
> But HCD provides DMA mapping and unmapping function by default.
> Moreover, it causes unnecessary DMA mapping and unmapping which
> will be done again at the NIC driver and it wastes CPU cycles.
> So, implement map_urb_for_dma and unmap_urb_for_dma function for
> vhci in order to skip the DMA mapping and unmapping procedure.
> 
> 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 the
> stub driver to use SG list.
> 
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
>   drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 000ab7225717..14fc6d9f4e6a 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
>   	return 0;
>   }
>   
> +static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +			gfp_t mem_flags)
> +{
> +	dev_dbg(hcd->self.controller, "vhci does not map urb for dma\n");
> +
> +	if (urb->num_sgs)
> +		urb->transfer_flags |= URB_DMA_MAP_SG;
> +

Shouldn't this be part of patch 2. The debug message saying "no map"
and setting flag doesn't make sense.

> +       return 0;

This should be a tab and no spaces btw. chekpatch isn't happy.

> +}
> +
> +static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> +{
> +       dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");

This should be a tab and no spaces btw. chekpatch isn't happy.


WARNING: please, no spaces at the start of a line
#144: FILE: drivers/usb/usbip/vhci_hcd.c:1299:
+       return 0;$

WARNING: please, no spaces at the start of a line
#149: FILE: drivers/usb/usbip/vhci_hcd.c:1304:
+       dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");$

total: 0 errors, 2 warnings, 31 lines checked


> +}
> +
>   static const struct hc_driver vhci_hc_driver = {
>   	.description	= driver_name,
>   	.product_desc	= driver_desc,
> @@ -1304,6 +1320,9 @@ static const struct hc_driver vhci_hc_driver = {
>   
>   	.get_frame_number = vhci_get_frame_number,
>   
> +	.map_urb_for_dma = vhci_map_urb_for_dma,
> +	.unmap_urb_for_dma = vhci_unmap_urb_for_dma,
> +
>   	.hub_status_data = vhci_hub_status,
>   	.hub_control    = vhci_hub_control,
>   	.bus_suspend	= vhci_bus_suspend,
> 

thanks,
-- Shuah

Re: [PATCH v2 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci

[Suwan Kim]

On Mon, Jul 22, 2019 at 02:26:42PM -0600, shuah wrote:
> Hi Suwan,
> 
> On 7/5/19 10:43 AM, Suwan Kim wrote:
> > vhci doesn’t do DMA for remote device. Actually, the real DMA
> > operation is done by network card driver. vhci just passes virtual
> > address of the buffer to the network stack, so vhci doesn’t use and
> > need dma address of the buffer of the URB.
> > 
> > When it comes to supporting SG for vhci, it is useful to use native
> > SG list (urb->num_sgs) instead of mapped SG list because DMA mapping
> > fnuction can adjust the number of SG list (urb->num_mapped_sgs).
> > 
> > But HCD provides DMA mapping and unmapping function by default.
> > Moreover, it causes unnecessary DMA mapping and unmapping which
> > will be done again at the NIC driver and it wastes CPU cycles.
> > So, implement map_urb_for_dma and unmap_urb_for_dma function for
> > vhci in order to skip the DMA mapping and unmapping procedure.
> > 
> > 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 the
> > stub driver to use SG list.
> > 
> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > ---
> >   drivers/usb/usbip/vhci_hcd.c | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> > index 000ab7225717..14fc6d9f4e6a 100644
> > --- a/drivers/usb/usbip/vhci_hcd.c
> > +++ b/drivers/usb/usbip/vhci_hcd.c
> > @@ -1288,6 +1288,22 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
> >   	return 0;
> >   }
> > +static int vhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > +			gfp_t mem_flags)
> > +{
> > +	dev_dbg(hcd->self.controller, "vhci does not map urb for dma\n");
> > +
> > +	if (urb->num_sgs)
> > +		urb->transfer_flags |= URB_DMA_MAP_SG;
> > +
> 
> Shouldn't this be part of patch 2. The debug message saying "no map"
> and setting flag doesn't make sense.

I think you are right. Setting flag should be in patch 2. Thank you
for pointing out :)
I will remove unnecessary debug messages and move setting flag to
patch 2.

> > +       return 0;
> 
> This should be a tab and no spaces btw. chekpatch isn't happy.
> 
> > +}
> > +
> > +static void vhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> > +{
> > +       dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");
> 
> This should be a tab and no spaces btw. chekpatch isn't happy.
> 
> 
> WARNING: please, no spaces at the start of a line
> #144: FILE: drivers/usb/usbip/vhci_hcd.c:1299:
> +       return 0;$
> 
> WARNING: please, no spaces at the start of a line
> #149: FILE: drivers/usb/usbip/vhci_hcd.c:1304:
> +       dev_dbg(hcd->self.controller, "vhci does not unmap urb for dma\n");$
> 
> total: 0 errors, 2 warnings, 31 lines checked

I'm sorry for my fault. I will check it.

Regards,
Suwan Kim

[PATCH v2 2/2] usbip: Implement SG support to vhci

[Suwan Kim]

There are bugs on vhci with usb 3.0 storage device. Originally, vhci
doesn't supported SG, so USB storage driver on vhci breaks SG list
into multiple URBs and it causes error 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 the 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 vhci.

If HCD of the server doesn't support SG, the stub driver breaks a
single SG reqeust into several URBs and submit them to the server's
HCD. When all the split URBs are completed, the stub driver
reassembles the URBs into a single return command and sends it to
vhci.

Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
 drivers/usb/usbip/stub.h         |   7 +-
 drivers/usb/usbip/stub_main.c    |  52 +++++---
 drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
 drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
 drivers/usb/usbip/usbip_common.c |  60 +++++++--
 drivers/usb/usbip/vhci_hcd.c     |  10 +-
 drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
 7 files changed, 372 insertions(+), 121 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..dec5af9f7179 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,39 @@ 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->setup_packet) {
+			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);
+	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 +348,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);
-
-		kmem_cache_free(stub_priv_cache, priv);
-
-		kfree(urb->transfer_buffer);
-		urb->transfer_buffer = NULL;
+		for (i = 0; i < priv->num_urbs; i++)
+			usb_kill_urb(priv->urbs[i]);
 
-		kfree(urb->setup_packet);
-		urb->setup_packet = NULL;
-
-		usb_free_urb(urb);
+		stub_free_priv_and_urb(priv);
 	}
 }
 
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index b0a855acafa3..8e32697acabb 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -7,6 +7,7 @@
 #include <linux/kthread.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/scatterlist.h>
 
 #include "usbip_common.h"
 #include "stub.h"
@@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
 static int stub_recv_cmd_unlink(struct stub_device *sdev,
 				struct usbip_header *pdu)
 {
-	int ret;
+	int ret, i;
 	unsigned long flags;
 	struct stub_priv *priv;
 
@@ -246,12 +247,13 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
 		 * so a driver in a client host will know the failure
 		 * of the unlink request ?
 		 */
-		ret = usb_unlink_urb(priv->urb);
-		if (ret != -EINPROGRESS)
-			dev_err(&priv->urb->dev->dev,
-				"failed to unlink a urb # %lu, ret %d\n",
-				priv->seqnum, ret);
-
+		for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
+			ret = usb_unlink_urb(priv->urbs[i]);
+			if (ret != -EINPROGRESS)
+				dev_err(&priv->urbs[i]->dev->dev,
+					"failed to unlink a urb # %lu, ret %d\n",
+					priv->seqnum, ret);
+		}
 		return 0;
 	}
 
@@ -433,14 +435,36 @@ static void masking_bogus_flags(struct urb *urb)
 	urb->transfer_flags &= allowed;
 }
 
+static int stub_recv_xbuff(struct usbip_device *ud, struct stub_priv *priv)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < priv->num_urbs; i++) {
+		ret = usbip_recv_xbuff(ud, priv->urbs[i]);
+		if (ret < 0)
+			break;
+	}
+
+	return ret;
+}
+
 static void stub_recv_cmd_submit(struct stub_device *sdev,
 				 struct usbip_header *pdu)
 {
-	int ret;
 	struct stub_priv *priv;
 	struct usbip_device *ud = &sdev->ud;
 	struct usb_device *udev = sdev->udev;
+	struct scatterlist *sgl = NULL, *sg;
+	void *buffer = NULL;
+	unsigned long long buf_len;
+	int nents;
+	int num_urbs = 1;
 	int pipe = get_pipe(sdev, pdu);
+	int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG;
+	int support_sg = 1;
+	int np = 0;
+	int ret, i;
 
 	if (pipe == -1)
 		return;
@@ -449,76 +473,143 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 	if (!priv)
 		return;
 
-	/* setup a urb */
-	if (usb_pipeisoc(pipe))
-		priv->urb = usb_alloc_urb(pdu->u.cmd_submit.number_of_packets,
-					  GFP_KERNEL);
-	else
-		priv->urb = usb_alloc_urb(0, GFP_KERNEL);
+	buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
 
-	if (!priv->urb) {
-		usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
-		return;
+	/* allocate urb transfer buffer, if needed */
+	if (buf_len) {
+		if (use_sg) {
+			sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
+			if (!sgl)
+				goto err_malloc;
+		} else {
+			buffer = kzalloc(buf_len, GFP_KERNEL);
+			if (!buffer)
+				goto err_malloc;
+		}
 	}
 
-	/* allocate urb transfer buffer, if needed */
-	if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
-		priv->urb->transfer_buffer =
-			kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
-				GFP_KERNEL);
-		if (!priv->urb->transfer_buffer) {
+	/* Check if the server's HCD supports SG */
+	if (use_sg && !udev->bus->sg_tablesize) {
+		/* If the server's HCD doesn't support SG, break a single SG
+		 * reqeust into several URBs and map the each SG list entry to
+		 * the each URB buffer. The previously allocated SG list is
+		 * stored in priv->sgl (If the server's HCD support SG, SG list
+		 * is stored only in urb->sg) and it is used as an indicator
+		 * that the server split single SG request into several URBs.
+		 * Later, priv->sgl is used by stub_complete() and
+		 * stub_send_ret_submit() to reassemble the divied URBs.
+		 */
+		support_sg = 0;
+		num_urbs = nents;
+		priv->completed_urbs = 0;
+		pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
+	}
+
+	/* allocate urb array */
+	priv->num_urbs = num_urbs;
+	priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
+	if (!priv->urbs)
+		goto err_urbs;
+
+	/* setup a urb */
+	if (support_sg) {
+		if (usb_pipeisoc(pipe))
+			np = pdu->u.cmd_submit.number_of_packets;
+
+		priv->urbs[0] = usb_alloc_urb(np, GFP_KERNEL);
+		if (!priv->urbs[0])
+			goto err_urb;
+
+		if (buf_len) {
+			if (use_sg) {
+				priv->urbs[0]->sg = sgl;
+				priv->urbs[0]->num_sgs = nents;
+				priv->urbs[0]->transfer_buffer = NULL;
+			}
+			else {
+				priv->urbs[0]->transfer_buffer = buffer;
+			}
+		}
+
+		/* copy urb setup packet */
+		priv->urbs[0]->setup_packet = kmemdup(&pdu->u.cmd_submit.setup,
+					8, GFP_KERNEL);
+		if (!priv->urbs[0]->setup_packet) {
+			dev_err(&udev->dev, "allocate setup_packet\n");
 			usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
 			return;
 		}
-	}
 
-	/* copy urb setup packet */
-	priv->urb->setup_packet = kmemdup(&pdu->u.cmd_submit.setup, 8,
-					  GFP_KERNEL);
-	if (!priv->urb->setup_packet) {
-		dev_err(&udev->dev, "allocate setup_packet\n");
-		usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
-		return;
+		usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
+	} else {
+		for_each_sg(sgl, sg, nents, i) {
+			priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
+			/* The URBs which is previously allocated will be freed
+			 * in stub_device_cleanup_urbs() if error occurs.
+			 */
+			if (!priv->urbs[i])
+				goto err_urb;
+
+			usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
+			priv->urbs[i]->transfer_buffer = sg_virt(sg);
+			priv->urbs[i]->transfer_buffer_length = sg->length;
+		}
+		priv->sgl = sgl;
 	}
 
-	/* set other members from the base header of pdu */
-	priv->urb->context                = (void *) priv;
-	priv->urb->dev                    = udev;
-	priv->urb->pipe                   = pipe;
-	priv->urb->complete               = stub_complete;
+	for (i = 0; i < num_urbs; i++) {
+		/* set other members from the base header of pdu */
+		priv->urbs[i]->context = (void *) priv;
+		priv->urbs[i]->dev = udev;
+		priv->urbs[i]->pipe = pipe;
+		priv->urbs[i]->complete = stub_complete;
 
-	usbip_pack_pdu(pdu, priv->urb, USBIP_CMD_SUBMIT, 0);
+		/* no need to submit an intercepted request, but harmless? */
+		tweak_special_requests(priv->urbs[i]);
 
+		masking_bogus_flags(priv->urbs[i]);
+	}
 
-	if (usbip_recv_xbuff(ud, priv->urb) < 0)
+	if (stub_recv_xbuff(ud, priv) < 0)
 		return;
 
-	if (usbip_recv_iso(ud, priv->urb) < 0)
+	if (usbip_recv_iso(ud, priv->urbs[0]) < 0)
 		return;
 
-	/* no need to submit an intercepted request, but harmless? */
-	tweak_special_requests(priv->urb);
-
-	masking_bogus_flags(priv->urb);
 	/* urb is now ready to submit */
-	ret = usb_submit_urb(priv->urb, GFP_KERNEL);
-
-	if (ret == 0)
-		usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
-				  pdu->base.seqnum);
-	else {
-		dev_err(&udev->dev, "submit_urb error, %d\n", ret);
-		usbip_dump_header(pdu);
-		usbip_dump_urb(priv->urb);
-
-		/*
-		 * Pessimistic.
-		 * This connection will be discarded.
-		 */
-		usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
+	for (i = 0; i < priv->num_urbs; i++) {
+		ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
+
+		if (ret == 0)
+			usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
+					pdu->base.seqnum);
+		else {
+			dev_err(&udev->dev, "submit_urb error, %d\n", ret);
+			usbip_dump_header(pdu);
+			usbip_dump_urb(priv->urbs[i]);
+
+			/*
+			* Pessimistic.
+			* This connection will be discarded.
+			*/
+			usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
+			break;
+		}
 	}
 
 	usbip_dbg_stub_rx("Leave\n");
+	return;
+
+err_urb:
+	kfree(priv->urbs);
+err_urbs:
+	if (buffer)
+		kfree(buffer);
+	if (sgl)
+		sgl_free(sgl);
+err_malloc:
+	usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
+	return;
 }
 
 /* recv a pdu */
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index f0ec41a50cbc..4561147ec1a1 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -5,25 +5,11 @@
 
 #include <linux/kthread.h>
 #include <linux/socket.h>
+#include <linux/scatterlist.h>
 
 #include "usbip_common.h"
 #include "stub.h"
 
-static void stub_free_priv_and_urb(struct stub_priv *priv)
-{
-	struct urb *urb = priv->urb;
-
-	kfree(urb->setup_packet);
-	urb->setup_packet = NULL;
-
-	kfree(urb->transfer_buffer);
-	urb->transfer_buffer = NULL;
-
-	list_del(&priv->list);
-	kmem_cache_free(stub_priv_cache, priv);
-	usb_free_urb(urb);
-}
-
 /* be in spin_lock_irqsave(&sdev->priv_lock, flags) */
 void stub_enqueue_ret_unlink(struct stub_device *sdev, __u32 seqnum,
 			     __u32 status)
@@ -85,6 +71,21 @@ void stub_complete(struct urb *urb)
 		break;
 	}
 
+	/* If the server break single SG reqeust into the several URBs, the
+	 * URBs must be reassembled before sending completed URB to the vhci.
+	 * Don't wake up the tx thread until all the URBs are completed.
+	 */
+	if (priv->sgl) {
+		priv->completed_urbs++;
+
+		/* Only save the first error status */
+		if (urb->status && !priv->urb_status)
+			priv->urb_status = urb->status;
+
+		if (priv->completed_urbs != priv->num_urbs)
+			return;
+	}
+
 	/* link a urb to the queue of tx. */
 	spin_lock_irqsave(&sdev->priv_lock, flags);
 	if (sdev->ud.tcp_socket == NULL) {
@@ -149,33 +150,40 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 {
 	unsigned long flags;
 	struct stub_priv *priv, *tmp;
-
 	struct msghdr msg;
 	size_t txsize;
-
 	size_t total_size = 0;
 
 	while ((priv = dequeue_from_priv_tx(sdev)) != NULL) {
-		int ret;
-		struct urb *urb = priv->urb;
+		struct urb *urb = priv->urbs[0];
 		struct usbip_header pdu_header;
 		struct usbip_iso_packet_descriptor *iso_buffer = NULL;
 		struct kvec *iov = NULL;
+		struct scatterlist *sg;
+		u32 actual_length = 0;
 		int iovnum = 0;
+		int ret;
+		int i;
 
-		txsize = 0;
-		memset(&pdu_header, 0, sizeof(pdu_header));
-		memset(&msg, 0, sizeof(msg));
-
-		if (urb->actual_length > 0 && !urb->transfer_buffer) {
+		if (urb->actual_length > 0 && !urb->transfer_buffer &&
+		   !urb->num_sgs) {
 			dev_err(&sdev->udev->dev,
 				"urb: actual_length %d transfer_buffer null\n",
 				urb->actual_length);
 			return -1;
 		}
 
+		txsize = 0;
+		memset(&pdu_header, 0, sizeof(pdu_header));
+		memset(&msg, 0, sizeof(msg));
+
 		if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
 			iovnum = 2 + urb->number_of_packets;
+		else if (usb_pipein(urb->pipe) && urb->actual_length > 0
+				&& urb->num_sgs)
+			iovnum = 1 + urb->num_sgs;
+		else if (usb_pipein(urb->pipe) && priv->sgl)
+			iovnum = 1 + priv->num_urbs;
 		else
 			iovnum = 2;
 
@@ -192,6 +200,15 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 		setup_ret_submit_pdu(&pdu_header, urb);
 		usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
 				  pdu_header.base.seqnum);
+
+		if (priv->sgl) {
+			for (i = 0; i < priv->num_urbs; i++)
+				actual_length += priv->urbs[i]->actual_length;
+
+			pdu_header.u.ret_submit.status = priv->urb_status;
+			pdu_header.u.ret_submit.actual_length = actual_length;
+		}
+
 		usbip_header_correct_endian(&pdu_header, 1);
 
 		iov[iovnum].iov_base = &pdu_header;
@@ -200,12 +217,47 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 		txsize += sizeof(pdu_header);
 
 		/* 2. setup transfer buffer */
-		if (usb_pipein(urb->pipe) &&
+		if (usb_pipein(urb->pipe) && priv->sgl) {
+			/* If the server split a single SG request into several
+			 * URBs because the server's HCD doesn't support SG,
+			 * reassemble the split URB buffers into a single
+			 * return command.
+			 */
+			for (i = 0; i < priv->num_urbs; i++) {
+				iov[iovnum].iov_base =
+					priv->urbs[i]->transfer_buffer;
+				iov[iovnum].iov_len =
+					priv->urbs[i]->actual_length;
+				iovnum++;
+			}
+			txsize += actual_length;
+		} else if (usb_pipein(urb->pipe) &&
 		    usb_pipetype(urb->pipe) != PIPE_ISOCHRONOUS &&
 		    urb->actual_length > 0) {
-			iov[iovnum].iov_base = urb->transfer_buffer;
-			iov[iovnum].iov_len  = urb->actual_length;
-			iovnum++;
+			if (urb->num_sgs) {
+				unsigned int copy = urb->actual_length;
+				int size;
+
+				for_each_sg(urb->sg, sg, urb->num_sgs ,i) {
+					if (copy == 0)
+						break;
+
+					if (copy < sg->length)
+						size = copy;
+					else
+						size = sg->length;
+
+					iov[iovnum].iov_base = sg_virt(sg);
+					iov[iovnum].iov_len = size;
+
+					iovnum++;
+					copy -= size;
+				}
+			} else {
+				iov[iovnum].iov_base = urb->transfer_buffer;
+				iov[iovnum].iov_len  = urb->actual_length;
+				iovnum++;
+			}
 			txsize += urb->actual_length;
 		} else if (usb_pipein(urb->pipe) &&
 			   usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 45da3e01c7b0..6affbfcfc927 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -680,8 +680,12 @@ EXPORT_SYMBOL_GPL(usbip_pad_iso);
 /* some members of urb must be substituted before. */
 int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
 {
-	int ret;
+	struct scatterlist *sg;
+	int ret = 0;
+	int recv;
 	int size;
+	int copy;
+	int i;
 
 	if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
 		/* the direction of urb must be OUT. */
@@ -712,14 +716,52 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
 		}
 	}
 
-	ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
-	if (ret != size) {
-		dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
-		if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
-			usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
-		} else {
-			usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
-			return -EPIPE;
+	if (urb->num_sgs) {
+		copy = size;
+		for_each_sg(urb->sg, sg, urb->num_sgs, i) {
+			int recv_size;
+
+			if (copy < sg->length)
+				recv_size = copy;
+			else
+				recv_size = sg->length;
+
+			recv = usbip_recv(ud->tcp_socket, sg_virt(sg),
+						recv_size);
+
+			if (recv != recv_size) {
+				dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
+				if (ud->side == USBIP_STUB ||
+				    ud->side == USBIP_VUDC) {
+					usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
+				} else {
+					usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
+					return -EPIPE;
+				}
+			}
+			copy -= recv;
+			ret += recv;
+		}
+
+		if (ret != size) {
+			dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
+			if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
+				usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
+			} else {
+				usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
+				return -EPIPE;
+			}
+		}
+	} else {
+		ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
+		if (ret != size) {
+			dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
+			if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
+				usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
+			} else {
+				usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
+				return -EPIPE;
+			}
 		}
 	}
 
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 14fc6d9f4e6a..b5fe85adb42b 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -697,7 +697,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;
 	}
@@ -1143,6 +1144,13 @@ static int vhci_setup(struct usb_hcd *hcd)
 		hcd->speed = HCD_USB3;
 		hcd->self.root_hub->speed = USB_SPEED_SUPER;
 	}
+
+	/* support SG.
+	 * sg_tablesize is an arbitrary vaule to alleviate memory pressure
+	 * on the host. */
+	hcd->self.sg_tablesize = 32;
+	hcd->self.no_sg_constraint = 1;
+
 	return 0;
 }
 
diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c
index 2fa26d0578d7..3472180f5af8 100644
--- a/drivers/usb/usbip/vhci_tx.c
+++ b/drivers/usb/usbip/vhci_tx.c
@@ -5,6 +5,7 @@
 
 #include <linux/kthread.h>
 #include <linux/slab.h>
+#include <linux/scatterlist.h>
 
 #include "usbip_common.h"
 #include "vhci.h"
@@ -51,12 +52,13 @@ static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
 static int vhci_send_cmd_submit(struct vhci_device *vdev)
 {
 	struct vhci_priv *priv = NULL;
-
+	struct scatterlist *sg;
 	struct msghdr msg;
-	struct kvec iov[3];
+	struct kvec *iov;
 	size_t txsize;
-
 	size_t total_size = 0;
+	int iovnum;
+	int i;
 
 	while ((priv = dequeue_from_priv_tx(vdev)) != NULL) {
 		int ret;
@@ -72,18 +74,41 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
 		usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n",
 				  priv->seqnum);
 
+		if (urb->num_sgs && usb_pipeout(urb->pipe))
+			iovnum = 2 + urb->num_sgs;
+		else
+			iovnum = 3;
+
+		iov = kzalloc(iovnum * sizeof(*iov), GFP_KERNEL);
+		if (!iov) {
+			usbip_event_add(&vdev->ud,
+						SDEV_EVENT_ERROR_MALLOC);
+			return -ENOMEM;
+		}
+
 		/* 1. setup usbip_header */
 		setup_cmd_submit_pdu(&pdu_header, urb);
 		usbip_header_correct_endian(&pdu_header, 1);
+		iovnum = 0;
 
-		iov[0].iov_base = &pdu_header;
-		iov[0].iov_len  = sizeof(pdu_header);
+		iov[iovnum].iov_base = &pdu_header;
+		iov[iovnum].iov_len  = sizeof(pdu_header);
 		txsize += sizeof(pdu_header);
+		iovnum++;
 
 		/* 2. setup transfer buffer */
 		if (!usb_pipein(urb->pipe) && urb->transfer_buffer_length > 0) {
-			iov[1].iov_base = urb->transfer_buffer;
-			iov[1].iov_len  = urb->transfer_buffer_length;
+			if (urb->num_sgs && !usb_endpoint_xfer_isoc(&urb->ep->desc)) {
+				for_each_sg(urb->sg, sg, urb->num_sgs ,i) {
+					iov[iovnum].iov_base = sg_virt(sg);
+					iov[iovnum].iov_len = sg->length;
+					iovnum++;
+				}
+			} else {
+				iov[iovnum].iov_base = urb->transfer_buffer;
+				iov[iovnum].iov_len  = urb->transfer_buffer_length;
+				iovnum++;
+			}
 			txsize += urb->transfer_buffer_length;
 		}
 
@@ -93,25 +118,29 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
 
 			iso_buffer = usbip_alloc_iso_desc_pdu(urb, &len);
 			if (!iso_buffer) {
+				kfree(iov);
 				usbip_event_add(&vdev->ud,
 						SDEV_EVENT_ERROR_MALLOC);
 				return -1;
 			}
 
-			iov[2].iov_base = iso_buffer;
-			iov[2].iov_len  = len;
+			iov[iovnum].iov_base = iso_buffer;
+			iov[iovnum].iov_len  = len;
+			iovnum++;
 			txsize += len;
 		}
 
-		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 3, txsize);
+		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, iovnum, txsize);
 		if (ret != txsize) {
 			pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
 			       txsize);
+			kfree(iov);
 			kfree(iso_buffer);
 			usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_TCP);
 			return -1;
 		}
 
+		kfree(iov);
 		kfree(iso_buffer);
 		usbip_dbg_vhci_tx("send txdata\n");
 
-- 
2.20.1

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[shuah]

On 7/5/19 10:43 AM, 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 breaks SG list

grammar - Currently vhci doesn't support?

> into multiple URBs and it causes error 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

What does basically support mean here? Do you mean basic support?

> entry to the stub driver. Then, the 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 the stub driver receives
> completed URB, it again sends each SG list entry to vhci.
> 
> If HCD of the server doesn't support SG, the stub driver breaks a
> single SG reqeust into several URBs and submit them to the server's
> HCD. When all the split URBs are completed, the stub driver
> reassembles the URBs into a single return command and sends it to
> vhci.
> 
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
>   drivers/usb/usbip/stub.h         |   7 +-
>   drivers/usb/usbip/stub_main.c    |  52 +++++---
>   drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
>   drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
>   drivers/usb/usbip/usbip_common.c |  60 +++++++--
>   drivers/usb/usbip/vhci_hcd.c     |  10 +-
>   drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
>   7 files changed, 372 insertions(+), 121 deletions(-)
> 

btw checkpatch isn't very happy with this patch. A few coding style
issues. Please run checkpatch before sending patches.

> 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..dec5af9f7179 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,39 @@ 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->setup_packet) {
> +			kfree(urb->setup_packet);
> +			urb->setup_packet = NULL;
> +		}
> +

You don't need urb->setup_packet null check. kfree() is safe
to call with null pointer. btw checkpatch tells you this.

> +		if (urb->transfer_buffer && !priv->sgl) {

Is this conditional necessary? Why are you checking priv->sgl?
Are there cases where you have memory leak? Is there a case
when urb->transfer_buffer valid when priv->sgl isn't null?

> +			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);
> +	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 +348,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);
> -
> -		kmem_cache_free(stub_priv_cache, priv);
> -
> -		kfree(urb->transfer_buffer);
> -		urb->transfer_buffer = NULL;
> +		for (i = 0; i < priv->num_urbs; i++)
> +			usb_kill_urb(priv->urbs[i]);
>   
> -		kfree(urb->setup_packet);
> -		urb->setup_packet = NULL;
> -
> -		usb_free_urb(urb);
> +		stub_free_priv_and_urb(priv);
>   	}
>   }
>   
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index b0a855acafa3..8e32697acabb 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -7,6 +7,7 @@
>   #include <linux/kthread.h>
>   #include <linux/usb.h>
>   #include <linux/usb/hcd.h>
> +#include <linux/scatterlist.h>
>   
>   #include "usbip_common.h"
>   #include "stub.h"
> @@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
>   static int stub_recv_cmd_unlink(struct stub_device *sdev,
>   				struct usbip_header *pdu)
>   {
> -	int ret;
> +	int ret, i;
>   	unsigned long flags;
>   	struct stub_priv *priv;
>   
> @@ -246,12 +247,13 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
>   		 * so a driver in a client host will know the failure
>   		 * of the unlink request ?
>   		 */
> -		ret = usb_unlink_urb(priv->urb);
> -		if (ret != -EINPROGRESS)
> -			dev_err(&priv->urb->dev->dev,
> -				"failed to unlink a urb # %lu, ret %d\n",
> -				priv->seqnum, ret);
> -
> +		for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
> +			ret = usb_unlink_urb(priv->urbs[i]);
> +			if (ret != -EINPROGRESS)
> +				dev_err(&priv->urbs[i]->dev->dev,
> +					"failed to unlink a urb # %lu, ret %d\n",
> +					priv->seqnum, ret);

This could result in several error messages. This code path is much
longer now compared to previous.

This is how far I have gotten. I am going to take a look at the rest
tomorrow.

thanks,
-- Shuah

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[Suwan Kim]

On Mon, Jul 22, 2019 at 09:51:30PM -0600, shuah wrote:
> On 7/5/19 10:43 AM, 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 breaks SG list
> 
> grammar - Currently vhci doesn't support?

Yes, that is what I intended. Please excuse my poor english.

> > into multiple URBs and it causes error 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
> 
> What does basically support mean here? Do you mean basic support?

What I intended was that 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. I will rewrite the description to make it more
clear.

> > entry to the stub driver. Then, the 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 the stub driver receives
> > completed URB, it again sends each SG list entry to vhci.
> > 
> > If HCD of the server doesn't support SG, the stub driver breaks a
> > single SG reqeust into several URBs and submit them to the server's
> > HCD. When all the split URBs are completed, the stub driver
> > reassembles the URBs into a single return command and sends it to
> > vhci.
> > 
> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > ---
> >   drivers/usb/usbip/stub.h         |   7 +-
> >   drivers/usb/usbip/stub_main.c    |  52 +++++---
> >   drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
> >   drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
> >   drivers/usb/usbip/usbip_common.c |  60 +++++++--
> >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
> >   drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
> >   7 files changed, 372 insertions(+), 121 deletions(-)
> > 
> 
> btw checkpatch isn't very happy with this patch. A few coding style
> issues. Please run checkpatch before sending patches.

I'm sorry for sending it without running checkpatch.
I will fix all problems with checkpatch and resend v3.

> > 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..dec5af9f7179 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,39 @@ 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->setup_packet) {
> > +			kfree(urb->setup_packet);
> > +			urb->setup_packet = NULL;
> > +		}
> > +
> 
> You don't need urb->setup_packet null check. kfree() is safe
> to call with null pointer. btw checkpatch tells you this.

Ok. I will remove null check.

> > +		if (urb->transfer_buffer && !priv->sgl) {
> 
> Is this conditional necessary? Why are you checking priv->sgl?
> Are there cases where you have memory leak? Is there a case
> when urb->transfer_buffer valid when priv->sgl isn't null?

In my implementation, if URB has an SG list, whether SG is supported
by the server's host controller or not, stub driver allocates SG list
and pages calling sg_alloc().

At this time, if the server's host controller does not support SG,
stub driver stores SG list in stub_priv->sgl (not in urb->sg) and
allocates URBs according to each entry of SG list. The page of each
SG list entry is mapped to each URB's transfer buffer.
(urb->transfer_buffer = sg_virt(sg))

So, when deallocating these URBs, stub driver doesn't deallocate the
URB's buffer with kfree(), but use sgl_free() to deallocate the SG
list stored in stub_priv (priv->sgl) and all the pages mapped to each
URB's buffer at once.

> > +			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);
> > +	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 +348,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);
> > -
> > -		kmem_cache_free(stub_priv_cache, priv);
> > -
> > -		kfree(urb->transfer_buffer);
> > -		urb->transfer_buffer = NULL;
> > +		for (i = 0; i < priv->num_urbs; i++)
> > +			usb_kill_urb(priv->urbs[i]);
> > -		kfree(urb->setup_packet);
> > -		urb->setup_packet = NULL;
> > -
> > -		usb_free_urb(urb);
> > +		stub_free_priv_and_urb(priv);
> >   	}
> >   }
> > diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> > index b0a855acafa3..8e32697acabb 100644
> > --- a/drivers/usb/usbip/stub_rx.c
> > +++ b/drivers/usb/usbip/stub_rx.c
> > @@ -7,6 +7,7 @@
> >   #include <linux/kthread.h>
> >   #include <linux/usb.h>
> >   #include <linux/usb/hcd.h>
> > +#include <linux/scatterlist.h>
> >   #include "usbip_common.h"
> >   #include "stub.h"
> > @@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
> >   static int stub_recv_cmd_unlink(struct stub_device *sdev,
> >   				struct usbip_header *pdu)
> >   {
> > -	int ret;
> > +	int ret, i;
> >   	unsigned long flags;
> >   	struct stub_priv *priv;
> > @@ -246,12 +247,13 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
> >   		 * so a driver in a client host will know the failure
> >   		 * of the unlink request ?
> >   		 */
> > -		ret = usb_unlink_urb(priv->urb);
> > -		if (ret != -EINPROGRESS)
> > -			dev_err(&priv->urb->dev->dev,
> > -				"failed to unlink a urb # %lu, ret %d\n",
> > -				priv->seqnum, ret);
> > -
> > +		for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
> > +			ret = usb_unlink_urb(priv->urbs[i]);
> > +			if (ret != -EINPROGRESS)
> > +				dev_err(&priv->urbs[i]->dev->dev,
> > +					"failed to unlink a urb # %lu, ret %d\n",
> > +					priv->seqnum, ret);
> 
> This could result in several error messages. This code path is much
> longer now compared to previous.

I don't know what function or part you point to. stub_recv_cmd_unlink()?

Regards,
Suwan Kim

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[shuah]

Hi Suwan,

On 7/5/19 10:43 AM, 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 breaks SG list
> into multiple URBs and it causes error 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 the 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 vhci.
> 
> If HCD of the server doesn't support SG, the stub driver breaks a
> single SG reqeust into several URBs and submit them to the server's
> HCD. When all the split URBs are completed, the stub driver
> reassembles the URBs into a single return command and sends it to
> vhci.
> 
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> ---
>   drivers/usb/usbip/stub.h         |   7 +-
>   drivers/usb/usbip/stub_main.c    |  52 +++++---
>   drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
>   drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
>   drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
>   drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
>   7 files changed, 372 insertions(+), 121 deletions(-)

While you are working on v3 to fix chekpatch and other issues
I pointed out, I have more for you.

What happens when you have mismatched server and client side?
i.e stub does and vhci doesn't and vice versa.

Make sure to run checkpatch. Also check spelling errors in
comments and your commit log.

I am not sure if your eeror paths are correct. Run usbip_test.sh

tools/testing/selftests/drivers/usb/usbip

> 
> 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..dec5af9f7179 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,39 @@ 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->setup_packet) {
> +			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);
> +	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 +348,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);
> -
> -		kmem_cache_free(stub_priv_cache, priv);
> -
> -		kfree(urb->transfer_buffer);
> -		urb->transfer_buffer = NULL;
> +		for (i = 0; i < priv->num_urbs; i++)
> +			usb_kill_urb(priv->urbs[i]);
>   
> -		kfree(urb->setup_packet);
> -		urb->setup_packet = NULL;
> -
> -		usb_free_urb(urb);
> +		stub_free_priv_and_urb(priv);
>   	}
>   }
>   
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index b0a855acafa3..8e32697acabb 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -7,6 +7,7 @@
>   #include <linux/kthread.h>
>   #include <linux/usb.h>
>   #include <linux/usb/hcd.h>
> +#include <linux/scatterlist.h>
>   
>   #include "usbip_common.h"
>   #include "stub.h"
> @@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
>   static int stub_recv_cmd_unlink(struct stub_device *sdev,
>   				struct usbip_header *pdu)
>   {
> -	int ret;
> +	int ret, i;
>   	unsigned long flags;
>   	struct stub_priv *priv;
>   
> @@ -246,12 +247,13 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
>   		 * so a driver in a client host will know the failure
>   		 * of the unlink request ?
>   		 */
> -		ret = usb_unlink_urb(priv->urb);
> -		if (ret != -EINPROGRESS)
> -			dev_err(&priv->urb->dev->dev,
> -				"failed to unlink a urb # %lu, ret %d\n",
> -				priv->seqnum, ret);
> -
> +		for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
> +			ret = usb_unlink_urb(priv->urbs[i]);
> +			if (ret != -EINPROGRESS)
> +				dev_err(&priv->urbs[i]->dev->dev,
> +					"failed to unlink a urb # %lu, ret %d\n",
> +					priv->seqnum, ret);
> +		}
>   		return 0;
>   	}
>   
> @@ -433,14 +435,36 @@ static void masking_bogus_flags(struct urb *urb)
>   	urb->transfer_flags &= allowed;
>   }
>   
> +static int stub_recv_xbuff(struct usbip_device *ud, struct stub_priv *priv)
> +{
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < priv->num_urbs; i++) {
> +		ret = usbip_recv_xbuff(ud, priv->urbs[i]);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
>   static void stub_recv_cmd_submit(struct stub_device *sdev,
>   				 struct usbip_header *pdu)
>   {
> -	int ret;
>   	struct stub_priv *priv;
>   	struct usbip_device *ud = &sdev->ud;
>   	struct usb_device *udev = sdev->udev;
> +	struct scatterlist *sgl = NULL, *sg;
> +	void *buffer = NULL;
> +	unsigned long long buf_len;
> +	int nents;
> +	int num_urbs = 1;
>   	int pipe = get_pipe(sdev, pdu);
> +	int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG;
> +	int support_sg = 1;
> +	int np = 0;
> +	int ret, i;
>   
>   	if (pipe == -1)
>   		return;
> @@ -449,76 +473,143 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
>   	if (!priv)
>   		return;
>   
> -	/* setup a urb */
> -	if (usb_pipeisoc(pipe))
> -		priv->urb = usb_alloc_urb(pdu->u.cmd_submit.number_of_packets,
> -					  GFP_KERNEL);
> -	else
> -		priv->urb = usb_alloc_urb(0, GFP_KERNEL);
> +	buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
>   
> -	if (!priv->urb) {
> -		usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> -		return;
> +	/* allocate urb transfer buffer, if needed */
> +	if (buf_len) {
> +		if (use_sg) {
> +			sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
> +			if (!sgl)
> +				goto err_malloc;
> +		} else {
> +			buffer = kzalloc(buf_len, GFP_KERNEL);
> +			if (!buffer)
> +				goto err_malloc;
> +		}
>   	}
>   
> -	/* allocate urb transfer buffer, if needed */
> -	if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
> -		priv->urb->transfer_buffer =
> -			kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
> -				GFP_KERNEL);
> -		if (!priv->urb->transfer_buffer) {
> +	/* Check if the server's HCD supports SG */
> +	if (use_sg && !udev->bus->sg_tablesize) {
> +		/* If the server's HCD doesn't support SG, break a single SG
> +		 * reqeust into several URBs and map the each SG list entry to
request - spelling?

map each - not map the each

> +		 * the each URB buffer. The previously allocated SG list is

drop "the each" reword to "corresponding URB"

> +		 * stored in priv->sgl (If the server's HCD support SG, SG list
> +		 * is stored only in urb->sg) and it is used as an indicator
> +		 * that the server split single SG request into several URBs.
> +		 * Later, priv->sgl is used by stub_complete() and
> +		 * stub_send_ret_submit() to reassemble the divied URBs.
> +		 */
> +		support_sg = 0;
> +		num_urbs = nents;
> +		priv->completed_urbs = 0;
> +		pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
> +	}
> +
> +	/* allocate urb array */
> +	priv->num_urbs = num_urbs;
> +	priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
> +	if (!priv->urbs)
> +		goto err_urbs;
> +
> +	/* setup a urb */
> +	if (support_sg) {
> +		if (usb_pipeisoc(pipe))
> +			np = pdu->u.cmd_submit.number_of_packets;
> +
> +		priv->urbs[0] = usb_alloc_urb(np, GFP_KERNEL);
> +		if (!priv->urbs[0])
> +			goto err_urb;
> +
> +		if (buf_len) {
> +			if (use_sg) {
> +				priv->urbs[0]->sg = sgl;
> +				priv->urbs[0]->num_sgs = nents;
> +				priv->urbs[0]->transfer_buffer = NULL;
> +			}
> +			else {
> +				priv->urbs[0]->transfer_buffer = buffer;
> +			}
> +		}
> +
> +		/* copy urb setup packet */
> +		priv->urbs[0]->setup_packet = kmemdup(&pdu->u.cmd_submit.setup,
> +					8, GFP_KERNEL);
> +		if (!priv->urbs[0]->setup_packet) {
> +			dev_err(&udev->dev, "allocate setup_packet\n");
>   			usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
>   			return;
>   		}
> -	}
>   
> -	/* copy urb setup packet */
> -	priv->urb->setup_packet = kmemdup(&pdu->u.cmd_submit.setup, 8,
> -					  GFP_KERNEL);
> -	if (!priv->urb->setup_packet) {
> -		dev_err(&udev->dev, "allocate setup_packet\n");
> -		usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> -		return;
> +		usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
> +	} else {
> +		for_each_sg(sgl, sg, nents, i) {
> +			priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
> +			/* The URBs which is previously allocated will be freed
> +			 * in stub_device_cleanup_urbs() if error occurs.
> +			 */
> +			if (!priv->urbs[i])
> +				goto err_urb;
> +
> +			usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
> +			priv->urbs[i]->transfer_buffer = sg_virt(sg);
> +			priv->urbs[i]->transfer_buffer_length = sg->length;
> +		}
> +		priv->sgl = sgl;
>   	}
>   
> -	/* set other members from the base header of pdu */
> -	priv->urb->context                = (void *) priv;
> -	priv->urb->dev                    = udev;
> -	priv->urb->pipe                   = pipe;
> -	priv->urb->complete               = stub_complete;
> +	for (i = 0; i < num_urbs; i++) {
> +		/* set other members from the base header of pdu */
> +		priv->urbs[i]->context = (void *) priv;
> +		priv->urbs[i]->dev = udev;
> +		priv->urbs[i]->pipe = pipe;
> +		priv->urbs[i]->complete = stub_complete;
>   
> -	usbip_pack_pdu(pdu, priv->urb, USBIP_CMD_SUBMIT, 0);
> +		/* no need to submit an intercepted request, but harmless? */
> +		tweak_special_requests(priv->urbs[i]);
>   
> +		masking_bogus_flags(priv->urbs[i]);
> +	}
>   
> -	if (usbip_recv_xbuff(ud, priv->urb) < 0)
> +	if (stub_recv_xbuff(ud, priv) < 0)
>   		return;
>   
> -	if (usbip_recv_iso(ud, priv->urb) < 0)
> +	if (usbip_recv_iso(ud, priv->urbs[0]) < 0)
>   		return;
>   
> -	/* no need to submit an intercepted request, but harmless? */
> -	tweak_special_requests(priv->urb);
> -
> -	masking_bogus_flags(priv->urb);
>   	/* urb is now ready to submit */
> -	ret = usb_submit_urb(priv->urb, GFP_KERNEL);
> -
> -	if (ret == 0)
> -		usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> -				  pdu->base.seqnum);
> -	else {
> -		dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> -		usbip_dump_header(pdu);
> -		usbip_dump_urb(priv->urb);
> -
> -		/*
> -		 * Pessimistic.
> -		 * This connection will be discarded.
> -		 */
> -		usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> +	for (i = 0; i < priv->num_urbs; i++) {
> +		ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
> +
> +		if (ret == 0)
> +			usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> +					pdu->base.seqnum);
> +		else {
> +			dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> +			usbip_dump_header(pdu);
> +			usbip_dump_urb(priv->urbs[i]);
> +
> +			/*
> +			* Pessimistic.
> +			* This connection will be discarded.
> +			*/
> +			usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> +			break;
> +		}
>   	}
>   
>   	usbip_dbg_stub_rx("Leave\n");
> +	return;
> +
> +err_urb:
> +	kfree(priv->urbs);
> +err_urbs:
> +	if (buffer)
> +		kfree(buffer);

No need to check buffer

> +	if (sgl)
> +		sgl_free(sgl);

No need to check sgl before calling sgl_free()

> +err_malloc:
> +	usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> +	return;
>   }
>   
>   /* recv a pdu */
> diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> index f0ec41a50cbc..4561147ec1a1 100644
> --- a/drivers/usb/usbip/stub_tx.c
> +++ b/drivers/usb/usbip/stub_tx.c
> @@ -5,25 +5,11 @@
>   
>   #include <linux/kthread.h>
>   #include <linux/socket.h>
> +#include <linux/scatterlist.h>
>   
>   #include "usbip_common.h"
>   #include "stub.h"
>   
> -static void stub_free_priv_and_urb(struct stub_priv *priv)
> -{
> -	struct urb *urb = priv->urb;
> -
> -	kfree(urb->setup_packet);
> -	urb->setup_packet = NULL;
> -
> -	kfree(urb->transfer_buffer);
> -	urb->transfer_buffer = NULL;
> -
> -	list_del(&priv->list);
> -	kmem_cache_free(stub_priv_cache, priv);
> -	usb_free_urb(urb);
> -}
> -
>   /* be in spin_lock_irqsave(&sdev->priv_lock, flags) */
>   void stub_enqueue_ret_unlink(struct stub_device *sdev, __u32 seqnum,
>   			     __u32 status)
> @@ -85,6 +71,21 @@ void stub_complete(struct urb *urb)
>   		break;
>   	}
>   
> +	/* If the server break single SG reqeust into the several URBs, the

breaks instead of break
request mispelled

> +	 * URBs must be reassembled before sending completed URB to the vhci.
> +	 * Don't wake up the tx thread until all the URBs are completed.
> +	 */
> +	if (priv->sgl) {
> +		priv->completed_urbs++;
> +
> +		/* Only save the first error status */
> +		if (urb->status && !priv->urb_status)
> +			priv->urb_status = urb->status;

I don't understand the saving first error status.

You could overwrite an error. urb->status could be 0?

> +
> +		if (priv->completed_urbs != priv->num_urbs)
> +			return;

What happens when some urbs don't make it? What happens if
priv->completed_urbs > priv->num_urbs

> +	}
> +
>   	/* link a urb to the queue of tx. */
>   	spin_lock_irqsave(&sdev->priv_lock, flags);
>   	if (sdev->ud.tcp_socket == NULL) {
> @@ -149,33 +150,40 @@ static int stub_send_ret_submit(struct stub_device *sdev)
>   {
>   	unsigned long flags;
>   	struct stub_priv *priv, *tmp;
> -

Don't delete the empty line used for groupong the variables.

>   	struct msghdr msg;
>   	size_t txsize;
> -

Don't delete the empty line used for groupong the variables.
Not worth combining with this patch.

>   	size_t total_size = 0;
>   
>   	while ((priv = dequeue_from_priv_tx(sdev)) != NULL) {
> -		int ret;
> -		struct urb *urb = priv->urb;
> +		struct urb *urb = priv->urbs[0];
>   		struct usbip_header pdu_header;
>   		struct usbip_iso_packet_descriptor *iso_buffer = NULL;
>   		struct kvec *iov = NULL;
> +		struct scatterlist *sg;
> +		u32 actual_length = 0;
>   		int iovnum = 0;
> +		int ret;
> +		int i;
>   
> -		txsize = 0;
> -		memset(&pdu_header, 0, sizeof(pdu_header));
> -		memset(&msg, 0, sizeof(msg));
> -
> -		if (urb->actual_length > 0 && !urb->transfer_buffer) {
> +		if (urb->actual_length > 0 && !urb->transfer_buffer &&
> +		   !urb->num_sgs) {

Here you check for !urb->num_sgs and the code below usb_pipetype()
seems to indicate num_sgs can be 0.

>   			dev_err(&sdev->udev->dev,
>   				"urb: actual_length %d transfer_buffer null\n",
>   				urb->actual_length);
>   			return -1;
>   		}
>   
> +		txsize = 0;
> +		memset(&pdu_header, 0, sizeof(pdu_header));
> +		memset(&msg, 0, sizeof(msg));
> +

Why did you move these lines? It makes it hard to review the patch.
This change isn't necessary for what you are doing. I just want to
see the changes for sg support.

>   		if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
>   			iovnum = 2 + urb->number_of_packets;
> +		else if (usb_pipein(urb->pipe) && urb->actual_length > 0
> +				&& urb->num_sgs)
> +			iovnum = 1 + urb->num_sgs;
> +		else if (usb_pipein(urb->pipe) && priv->sgl)
> +			iovnum = 1 + priv->num_urbs;
>   		else
>   			iovnum = 2;

Can num_sgs be 0 here. See comment above where you check valid urb case.
There is a diconnect here?

>   
> @@ -192,6 +200,15 @@ static int stub_send_ret_submit(struct stub_device *sdev)
>   		setup_ret_submit_pdu(&pdu_header, urb);
>   		usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
>   				  pdu_header.base.seqnum);
> +
> +		if (priv->sgl) {
> +			for (i = 0; i < priv->num_urbs; i++)
> +				actual_length += priv->urbs[i]->actual_length;
> +
> +			pdu_header.u.ret_submit.status = priv->urb_status;
> +			pdu_header.u.ret_submit.actual_length = actual_length;
> +		}
> +
>   		usbip_header_correct_endian(&pdu_header, 1);
>   
>   		iov[iovnum].iov_base = &pdu_header;
> @@ -200,12 +217,47 @@ static int stub_send_ret_submit(struct stub_device *sdev)
>   		txsize += sizeof(pdu_header);
>   
>   		/* 2. setup transfer buffer */
> -		if (usb_pipein(urb->pipe) &&
> +		if (usb_pipein(urb->pipe) && priv->sgl) {
> +			/* If the server split a single SG request into several
> +			 * URBs because the server's HCD doesn't support SG,
> +			 * reassemble the split URB buffers into a single
> +			 * return command.
> +			 */
> +			for (i = 0; i < priv->num_urbs; i++) {
> +				iov[iovnum].iov_base =
> +					priv->urbs[i]->transfer_buffer;
> +				iov[iovnum].iov_len =
> +					priv->urbs[i]->actual_length;
> +				iovnum++;
> +			}
> +			txsize += actual_length;
> +		} else if (usb_pipein(urb->pipe) &&
>   		    usb_pipetype(urb->pipe) != PIPE_ISOCHRONOUS &&
>   		    urb->actual_length > 0) {
> -			iov[iovnum].iov_base = urb->transfer_buffer;
> -			iov[iovnum].iov_len  = urb->actual_length;
> -			iovnum++;
> +			if (urb->num_sgs) {
> +				unsigned int copy = urb->actual_length;
> +				int size;
> +
> +				for_each_sg(urb->sg, sg, urb->num_sgs ,i) {
> +					if (copy == 0)
> +						break;
> +
> +					if (copy < sg->length)
> +						size = copy;
> +					else
> +						size = sg->length;
> +
> +					iov[iovnum].iov_base = sg_virt(sg);
> +					iov[iovnum].iov_len = size;
> +
> +					iovnum++;
> +					copy -= size;
> +				}
> +			} else {
> +				iov[iovnum].iov_base = urb->transfer_buffer;
> +				iov[iovnum].iov_len  = urb->actual_length;
> +				iovnum++;
> +			}
>   			txsize += urb->actual_length;
>   		} else if (usb_pipein(urb->pipe) &&
>   			   usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
> diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> index 45da3e01c7b0..6affbfcfc927 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -680,8 +680,12 @@ EXPORT_SYMBOL_GPL(usbip_pad_iso);
>   /* some members of urb must be substituted before. */
>   int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
>   {
> -	int ret;
> +	struct scatterlist *sg;
> +	int ret = 0;
> +	int recv;
>   	int size;
> +	int copy;
> +	int i;
>   
>   	if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
>   		/* the direction of urb must be OUT. */
> @@ -712,14 +716,52 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
>   		}
>   	}
>   
> -	ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
> -	if (ret != size) {
> -		dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> -		if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> -			usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> -		} else {
> -			usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> -			return -EPIPE;
> +	if (urb->num_sgs) {
> +		copy = size;
> +		for_each_sg(urb->sg, sg, urb->num_sgs, i) {
> +			int recv_size;
> +
> +			if (copy < sg->length)
> +				recv_size = copy;
> +			else
> +				recv_size = sg->length;
> +
> +			recv = usbip_recv(ud->tcp_socket, sg_virt(sg),
> +						recv_size);
> +
> +			if (recv != recv_size) {
> +				dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> +				if (ud->side == USBIP_STUB ||
> +				    ud->side == USBIP_VUDC) {
> +					usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);

This doesn't look right. You continue to send after adding
SDEV_EVENT_ERROR_TCP ?? Shouldn't you bail out instead?

> +				} else {
> +					usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> +					return -EPIPE;
> +				}
> +			}
> +			copy -= recv;
> +			ret += recv;
> +		}
> +
> +		if (ret != size) {
> +			dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> +			if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> +				usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> +			} else {
> +				usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> +				return -EPIPE;
> +			}
> +		}

Get rid of the duplicate code block for usbip_recv() error paths.

> +	} else {
> +		ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
> +		if (ret != size) {
> +			dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> +			if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> +				usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> +			} else {
> +				usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> +				return -EPIPE;
> +			}
>   		}

Get rid of the duplicate code block for usbip_recv() error paths.
There are 3 now and the above in the loop doesn't look right.

>   	}
>   
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 14fc6d9f4e6a..b5fe85adb42b 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -697,7 +697,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) {

Is there a case where transfer_buffer is valid without any num_sgs?

>   		dev_dbg(dev, "Null URB transfer buffer\n");
>   		return -EINVAL;
>   	}
> @@ -1143,6 +1144,13 @@ static int vhci_setup(struct usb_hcd *hcd)
>   		hcd->speed = HCD_USB3;
>   		hcd->self.root_hub->speed = USB_SPEED_SUPER;
>   	}
> +
> +	/* support SG.
> +	 * sg_tablesize is an arbitrary vaule to alleviate memory pressure
> +	 * on the host. */
> +	hcd->self.sg_tablesize = 32;
> +	hcd->self.no_sg_constraint = 1;
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c
> index 2fa26d0578d7..3472180f5af8 100644
> --- a/drivers/usb/usbip/vhci_tx.c
> +++ b/drivers/usb/usbip/vhci_tx.c
> @@ -5,6 +5,7 @@
>   
>   #include <linux/kthread.h>
>   #include <linux/slab.h>
> +#include <linux/scatterlist.h>
>   
>   #include "usbip_common.h"
>   #include "vhci.h"
> @@ -51,12 +52,13 @@ static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
>   static int vhci_send_cmd_submit(struct vhci_device *vdev)
>   {
>   	struct vhci_priv *priv = NULL;
> -
> +	struct scatterlist *sg;
>   	struct msghdr msg;
> -	struct kvec iov[3];
> +	struct kvec *iov;
>   	size_t txsize;
> -
>   	size_t total_size = 0;
> +	int iovnum;
> +	int i;
>   
>   	while ((priv = dequeue_from_priv_tx(vdev)) != NULL) {
>   		int ret;
> @@ -72,18 +74,41 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
>   		usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n",
>   				  priv->seqnum);
>   
> +		if (urb->num_sgs && usb_pipeout(urb->pipe))
> +			iovnum = 2 + urb->num_sgs;

Don't you want to do some kind of range check on num_sgs?

> +		else
> +			iovnum = 3;
> +
> +		iov = kzalloc(iovnum * sizeof(*iov), GFP_KERNEL);
> +		if (!iov) {
> +			usbip_event_add(&vdev->ud,
> +						SDEV_EVENT_ERROR_MALLOC);
> +			return -ENOMEM;
> +		}
> +
>   		/* 1. setup usbip_header */
>   		setup_cmd_submit_pdu(&pdu_header, urb);
>   		usbip_header_correct_endian(&pdu_header, 1);
> +		iovnum = 0;
>   
> -		iov[0].iov_base = &pdu_header;
> -		iov[0].iov_len  = sizeof(pdu_header);
> +		iov[iovnum].iov_base = &pdu_header;
> +		iov[iovnum].iov_len  = sizeof(pdu_header);
>   		txsize += sizeof(pdu_header);
> +		iovnum++;
>   
>   		/* 2. setup transfer buffer */
>   		if (!usb_pipein(urb->pipe) && urb->transfer_buffer_length > 0) {
> -			iov[1].iov_base = urb->transfer_buffer;
> -			iov[1].iov_len  = urb->transfer_buffer_length;
> +			if (urb->num_sgs && !usb_endpoint_xfer_isoc(&urb->ep->desc)) {
> +				for_each_sg(urb->sg, sg, urb->num_sgs ,i) {
> +					iov[iovnum].iov_base = sg_virt(sg);
> +					iov[iovnum].iov_len = sg->length;
> +					iovnum++;
> +				}
> +			} else {
> +				iov[iovnum].iov_base = urb->transfer_buffer;
> +				iov[iovnum].iov_len  = urb->transfer_buffer_length;
> +				iovnum++;

You would have allocated 2 + urb->num_sgs for the isochronous
case as well and not use them?

> +			}
>   			txsize += urb->transfer_buffer_length;
>   		}
>   
> @@ -93,25 +118,29 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
>   
>   			iso_buffer = usbip_alloc_iso_desc_pdu(urb, &len);
>   			if (!iso_buffer) {
> +				kfree(iov);

Consolidate error handling for kfree(iov)

>   				usbip_event_add(&vdev->ud,
>   						SDEV_EVENT_ERROR_MALLOC);
>   				return -1;
>   			}
>   
> -			iov[2].iov_base = iso_buffer;
> -			iov[2].iov_len  = len;
> +			iov[iovnum].iov_base = iso_buffer;
> +			iov[iovnum].iov_len  = len;
> +			iovnum++;
>   			txsize += len;
>   		}
>   
> -		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 3, txsize);
> +		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, iovnum, txsize);
>   		if (ret != txsize) {
>   			pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
>   			       txsize);
> +			kfree(iov);
>   			kfree(iso_buffer);

Consolidate error handling for kfree(iov) and kfree(iso_buffer)

>   			usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_TCP);
>   			return -1;
>   		}
>   
> +		kfree(iov);
>   		kfree(iso_buffer);
>   		usbip_dbg_vhci_tx("send txdata\n");
>   
> 

thanks,
-- Shuah

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[Suwan Kim]

Hi Shuah,

On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
> Hi Suwan,
> 
> On 7/5/19 10:43 AM, 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 breaks SG list
> > into multiple URBs and it causes error 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 the 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 vhci.
> > 
> > If HCD of the server doesn't support SG, the stub driver breaks a
> > single SG reqeust into several URBs and submit them to the server's
> > HCD. When all the split URBs are completed, the stub driver
> > reassembles the URBs into a single return command and sends it to
> > vhci.
> > 
> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > ---
> >   drivers/usb/usbip/stub.h         |   7 +-
> >   drivers/usb/usbip/stub_main.c    |  52 +++++---
> >   drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
> >   drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
> >   drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
> >   drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
> >   7 files changed, 372 insertions(+), 121 deletions(-)
> 
> While you are working on v3 to fix chekpatch and other issues
> I pointed out, I have more for you.
> 
> What happens when you have mismatched server and client side?
> i.e stub does and vhci doesn't and vice versa.
> 
> Make sure to run checkpatch. Also check spelling errors in
> comments and your commit log.
> 
> I am not sure if your eeror paths are correct. Run usbip_test.sh
> 
> tools/testing/selftests/drivers/usb/usbip

I don't know what mismatch you mentioned. Are you saying
"match busid table" at the end of usbip_test.sh?
How does it relate to SG support of this patch?
Could you tell me more about the problem situation?

> > 
> > 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..dec5af9f7179 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,39 @@ 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->setup_packet) {
> > +			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);
> > +	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 +348,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);
> > -
> > -		kmem_cache_free(stub_priv_cache, priv);
> > -
> > -		kfree(urb->transfer_buffer);
> > -		urb->transfer_buffer = NULL;
> > +		for (i = 0; i < priv->num_urbs; i++)
> > +			usb_kill_urb(priv->urbs[i]);
> > -		kfree(urb->setup_packet);
> > -		urb->setup_packet = NULL;
> > -
> > -		usb_free_urb(urb);
> > +		stub_free_priv_and_urb(priv);
> >   	}
> >   }
> > diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> > index b0a855acafa3..8e32697acabb 100644
> > --- a/drivers/usb/usbip/stub_rx.c
> > +++ b/drivers/usb/usbip/stub_rx.c
> > @@ -7,6 +7,7 @@
> >   #include <linux/kthread.h>
> >   #include <linux/usb.h>
> >   #include <linux/usb/hcd.h>
> > +#include <linux/scatterlist.h>
> >   #include "usbip_common.h"
> >   #include "stub.h"
> > @@ -201,7 +202,7 @@ static void tweak_special_requests(struct urb *urb)
> >   static int stub_recv_cmd_unlink(struct stub_device *sdev,
> >   				struct usbip_header *pdu)
> >   {
> > -	int ret;
> > +	int ret, i;
> >   	unsigned long flags;
> >   	struct stub_priv *priv;
> > @@ -246,12 +247,13 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
> >   		 * so a driver in a client host will know the failure
> >   		 * of the unlink request ?
> >   		 */
> > -		ret = usb_unlink_urb(priv->urb);
> > -		if (ret != -EINPROGRESS)
> > -			dev_err(&priv->urb->dev->dev,
> > -				"failed to unlink a urb # %lu, ret %d\n",
> > -				priv->seqnum, ret);
> > -
> > +		for (i = priv->completed_urbs; i < priv->num_urbs; i++) {
> > +			ret = usb_unlink_urb(priv->urbs[i]);
> > +			if (ret != -EINPROGRESS)
> > +				dev_err(&priv->urbs[i]->dev->dev,
> > +					"failed to unlink a urb # %lu, ret %d\n",
> > +					priv->seqnum, ret);
> > +		}
> >   		return 0;
> >   	}
> > @@ -433,14 +435,36 @@ static void masking_bogus_flags(struct urb *urb)
> >   	urb->transfer_flags &= allowed;
> >   }
> > +static int stub_recv_xbuff(struct usbip_device *ud, struct stub_priv *priv)
> > +{
> > +	int ret;
> > +	int i;
> > +
> > +	for (i = 0; i < priv->num_urbs; i++) {
> > +		ret = usbip_recv_xbuff(ud, priv->urbs[i]);
> > +		if (ret < 0)
> > +			break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >   static void stub_recv_cmd_submit(struct stub_device *sdev,
> >   				 struct usbip_header *pdu)
> >   {
> > -	int ret;
> >   	struct stub_priv *priv;
> >   	struct usbip_device *ud = &sdev->ud;
> >   	struct usb_device *udev = sdev->udev;
> > +	struct scatterlist *sgl = NULL, *sg;
> > +	void *buffer = NULL;
> > +	unsigned long long buf_len;
> > +	int nents;
> > +	int num_urbs = 1;
> >   	int pipe = get_pipe(sdev, pdu);
> > +	int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG;
> > +	int support_sg = 1;
> > +	int np = 0;
> > +	int ret, i;
> >   	if (pipe == -1)
> >   		return;
> > @@ -449,76 +473,143 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
> >   	if (!priv)
> >   		return;
> > -	/* setup a urb */
> > -	if (usb_pipeisoc(pipe))
> > -		priv->urb = usb_alloc_urb(pdu->u.cmd_submit.number_of_packets,
> > -					  GFP_KERNEL);
> > -	else
> > -		priv->urb = usb_alloc_urb(0, GFP_KERNEL);
> > +	buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
> > -	if (!priv->urb) {
> > -		usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> > -		return;
> > +	/* allocate urb transfer buffer, if needed */
> > +	if (buf_len) {
> > +		if (use_sg) {
> > +			sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
> > +			if (!sgl)
> > +				goto err_malloc;
> > +		} else {
> > +			buffer = kzalloc(buf_len, GFP_KERNEL);
> > +			if (!buffer)
> > +				goto err_malloc;
> > +		}
> >   	}
> > -	/* allocate urb transfer buffer, if needed */
> > -	if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
> > -		priv->urb->transfer_buffer =
> > -			kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
> > -				GFP_KERNEL);
> > -		if (!priv->urb->transfer_buffer) {
> > +	/* Check if the server's HCD supports SG */
> > +	if (use_sg && !udev->bus->sg_tablesize) {
> > +		/* If the server's HCD doesn't support SG, break a single SG
> > +		 * reqeust into several URBs and map the each SG list entry to
> request - spelling?
> 
> map each - not map the each
> 
> > +		 * the each URB buffer. The previously allocated SG list is
> 
> drop "the each" reword to "corresponding URB"

Thank you for pointing out. I will fix it.

> > +		 * stored in priv->sgl (If the server's HCD support SG, SG list
> > +		 * is stored only in urb->sg) and it is used as an indicator
> > +		 * that the server split single SG request into several URBs.
> > +		 * Later, priv->sgl is used by stub_complete() and
> > +		 * stub_send_ret_submit() to reassemble the divied URBs.
> > +		 */
> > +		support_sg = 0;
> > +		num_urbs = nents;
> > +		priv->completed_urbs = 0;
> > +		pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
> > +	}
> > +
> > +	/* allocate urb array */
> > +	priv->num_urbs = num_urbs;
> > +	priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
> > +	if (!priv->urbs)
> > +		goto err_urbs;
> > +
> > +	/* setup a urb */
> > +	if (support_sg) {
> > +		if (usb_pipeisoc(pipe))
> > +			np = pdu->u.cmd_submit.number_of_packets;
> > +
> > +		priv->urbs[0] = usb_alloc_urb(np, GFP_KERNEL);
> > +		if (!priv->urbs[0])
> > +			goto err_urb;
> > +
> > +		if (buf_len) {
> > +			if (use_sg) {
> > +				priv->urbs[0]->sg = sgl;
> > +				priv->urbs[0]->num_sgs = nents;
> > +				priv->urbs[0]->transfer_buffer = NULL;
> > +			}
> > +			else {
> > +				priv->urbs[0]->transfer_buffer = buffer;
> > +			}
> > +		}
> > +
> > +		/* copy urb setup packet */
> > +		priv->urbs[0]->setup_packet = kmemdup(&pdu->u.cmd_submit.setup,
> > +					8, GFP_KERNEL);
> > +		if (!priv->urbs[0]->setup_packet) {
> > +			dev_err(&udev->dev, "allocate setup_packet\n");
> >   			usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> >   			return;
> >   		}
> > -	}
> > -	/* copy urb setup packet */
> > -	priv->urb->setup_packet = kmemdup(&pdu->u.cmd_submit.setup, 8,
> > -					  GFP_KERNEL);
> > -	if (!priv->urb->setup_packet) {
> > -		dev_err(&udev->dev, "allocate setup_packet\n");
> > -		usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> > -		return;
> > +		usbip_pack_pdu(pdu, priv->urbs[0], USBIP_CMD_SUBMIT, 0);
> > +	} else {
> > +		for_each_sg(sgl, sg, nents, i) {
> > +			priv->urbs[i] = usb_alloc_urb(0, GFP_KERNEL);
> > +			/* The URBs which is previously allocated will be freed
> > +			 * in stub_device_cleanup_urbs() if error occurs.
> > +			 */
> > +			if (!priv->urbs[i])
> > +				goto err_urb;
> > +
> > +			usbip_pack_pdu(pdu, priv->urbs[i], USBIP_CMD_SUBMIT, 0);
> > +			priv->urbs[i]->transfer_buffer = sg_virt(sg);
> > +			priv->urbs[i]->transfer_buffer_length = sg->length;
> > +		}
> > +		priv->sgl = sgl;
> >   	}
> > -	/* set other members from the base header of pdu */
> > -	priv->urb->context                = (void *) priv;
> > -	priv->urb->dev                    = udev;
> > -	priv->urb->pipe                   = pipe;
> > -	priv->urb->complete               = stub_complete;
> > +	for (i = 0; i < num_urbs; i++) {
> > +		/* set other members from the base header of pdu */
> > +		priv->urbs[i]->context = (void *) priv;
> > +		priv->urbs[i]->dev = udev;
> > +		priv->urbs[i]->pipe = pipe;
> > +		priv->urbs[i]->complete = stub_complete;
> > -	usbip_pack_pdu(pdu, priv->urb, USBIP_CMD_SUBMIT, 0);
> > +		/* no need to submit an intercepted request, but harmless? */
> > +		tweak_special_requests(priv->urbs[i]);
> > +		masking_bogus_flags(priv->urbs[i]);
> > +	}
> > -	if (usbip_recv_xbuff(ud, priv->urb) < 0)
> > +	if (stub_recv_xbuff(ud, priv) < 0)
> >   		return;
> > -	if (usbip_recv_iso(ud, priv->urb) < 0)
> > +	if (usbip_recv_iso(ud, priv->urbs[0]) < 0)
> >   		return;
> > -	/* no need to submit an intercepted request, but harmless? */
> > -	tweak_special_requests(priv->urb);
> > -
> > -	masking_bogus_flags(priv->urb);
> >   	/* urb is now ready to submit */
> > -	ret = usb_submit_urb(priv->urb, GFP_KERNEL);
> > -
> > -	if (ret == 0)
> > -		usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> > -				  pdu->base.seqnum);
> > -	else {
> > -		dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> > -		usbip_dump_header(pdu);
> > -		usbip_dump_urb(priv->urb);
> > -
> > -		/*
> > -		 * Pessimistic.
> > -		 * This connection will be discarded.
> > -		 */
> > -		usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> > +	for (i = 0; i < priv->num_urbs; i++) {
> > +		ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
> > +
> > +		if (ret == 0)
> > +			usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
> > +					pdu->base.seqnum);
> > +		else {
> > +			dev_err(&udev->dev, "submit_urb error, %d\n", ret);
> > +			usbip_dump_header(pdu);
> > +			usbip_dump_urb(priv->urbs[i]);
> > +
> > +			/*
> > +			* Pessimistic.
> > +			* This connection will be discarded.
> > +			*/
> > +			usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
> > +			break;
> > +		}
> >   	}
> >   	usbip_dbg_stub_rx("Leave\n");
> > +	return;
> > +
> > +err_urb:
> > +	kfree(priv->urbs);
> > +err_urbs:
> > +	if (buffer)
> > +		kfree(buffer);
> 
> No need to check buffer
> 
> > +	if (sgl)
> > +		sgl_free(sgl);
> 
> No need to check sgl before calling sgl_free()

Ok.

> > +err_malloc:
> > +	usbip_event_add(ud, SDEV_EVENT_ERROR_MALLOC);
> > +	return;
> >   }
> >   /* recv a pdu */
> > diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> > index f0ec41a50cbc..4561147ec1a1 100644
> > --- a/drivers/usb/usbip/stub_tx.c
> > +++ b/drivers/usb/usbip/stub_tx.c
> > @@ -5,25 +5,11 @@
> >   #include <linux/kthread.h>
> >   #include <linux/socket.h>
> > +#include <linux/scatterlist.h>
> >   #include "usbip_common.h"
> >   #include "stub.h"
> > -static void stub_free_priv_and_urb(struct stub_priv *priv)
> > -{
> > -	struct urb *urb = priv->urb;
> > -
> > -	kfree(urb->setup_packet);
> > -	urb->setup_packet = NULL;
> > -
> > -	kfree(urb->transfer_buffer);
> > -	urb->transfer_buffer = NULL;
> > -
> > -	list_del(&priv->list);
> > -	kmem_cache_free(stub_priv_cache, priv);
> > -	usb_free_urb(urb);
> > -}
> > -
> >   /* be in spin_lock_irqsave(&sdev->priv_lock, flags) */
> >   void stub_enqueue_ret_unlink(struct stub_device *sdev, __u32 seqnum,
> >   			     __u32 status)
> > @@ -85,6 +71,21 @@ void stub_complete(struct urb *urb)
> >   		break;
> >   	}
> > +	/* If the server break single SG reqeust into the several URBs, the
> 
> breaks instead of break
> request mispelled

Ok.

> > +	 * URBs must be reassembled before sending completed URB to the vhci.
> > +	 * Don't wake up the tx thread until all the URBs are completed.
> > +	 */
> > +	if (priv->sgl) {
> > +		priv->completed_urbs++;
> > +
> > +		/* Only save the first error status */
> > +		if (urb->status && !priv->urb_status)
> > +			priv->urb_status = urb->status;
> 
> I don't understand the saving first error status.
> 
> You could overwrite an error. urb->status could be 0?

I think that urb->status can be overwritten with different values in
error situations. For example, in a situation where a single SG
request is split into multiple URBs in stub driver, the endpoint can
be stalled with an error in any intermediate URB.

The URB with the error has specific error status which tells us the
exact error situation, but the other URBs can have different error
state which indicates just endpoint stall.

Stub driver should reassemble these URBs and send exact error status
to vhci. So I save the first error status. My thoughts may be wrong.
Please let me know if something goes wrong.

> > +
> > +		if (priv->completed_urbs != priv->num_urbs)
> > +			return;
> 
> What happens when some urbs don't make it? What happens if
> priv->completed_urbs > priv->num_urbs

Complete function of URB is called even if URB fails. So I think
priv->completed_urbs counts normally, whether urb succeeds or fails.
I am going to modify "priv->completed_urbs != priv->num_urbs" to
"priv-> completed_urbs! = Priv-> num_urbs.

> > +	}
> > +
> >   	/* link a urb to the queue of tx. */
> >   	spin_lock_irqsave(&sdev->priv_lock, flags);
> >   	if (sdev->ud.tcp_socket == NULL) {
> > @@ -149,33 +150,40 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> >   {
> >   	unsigned long flags;
> >   	struct stub_priv *priv, *tmp;
> > -
> 
> Don't delete the empty line used for groupong the variables.
> 
> >   	struct msghdr msg;
> >   	size_t txsize;
> > -
> 
> Don't delete the empty line used for groupong the variables.
> Not worth combining with this patch.

Ok. I'm sorry for that.

> >   	size_t total_size = 0;
> >   	while ((priv = dequeue_from_priv_tx(sdev)) != NULL) {
> > -		int ret;
> > -		struct urb *urb = priv->urb;
> > +		struct urb *urb = priv->urbs[0];
> >   		struct usbip_header pdu_header;
> >   		struct usbip_iso_packet_descriptor *iso_buffer = NULL;
> >   		struct kvec *iov = NULL;
> > +		struct scatterlist *sg;
> > +		u32 actual_length = 0;
> >   		int iovnum = 0;
> > +		int ret;
> > +		int i;
> > -		txsize = 0;
> > -		memset(&pdu_header, 0, sizeof(pdu_header));
> > -		memset(&msg, 0, sizeof(msg));
> > -
> > -		if (urb->actual_length > 0 && !urb->transfer_buffer) {
> > +		if (urb->actual_length > 0 && !urb->transfer_buffer &&
> > +		   !urb->num_sgs) {
> 
> Here you check for !urb->num_sgs and the code below usb_pipetype()
> seems to indicate num_sgs can be 0.
> 
> >   			dev_err(&sdev->udev->dev,
> >   				"urb: actual_length %d transfer_buffer null\n",
> >   				urb->actual_length);
> >   			return -1;
> >   		}
> > +		txsize = 0;
> > +		memset(&pdu_header, 0, sizeof(pdu_header));
> > +		memset(&msg, 0, sizeof(msg));
> > +
> 
> Why did you move these lines? It makes it hard to review the patch.
> This change isn't necessary for what you are doing. I just want to
> see the changes for sg support.

I'm sorry. I will undo what I changed.

> >   		if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
> >   			iovnum = 2 + urb->number_of_packets;
> > +		else if (usb_pipein(urb->pipe) && urb->actual_length > 0
> > +				&& urb->num_sgs)
> > +			iovnum = 1 + urb->num_sgs;
> > +		else if (usb_pipein(urb->pipe) && priv->sgl)
> > +			iovnum = 1 + priv->num_urbs;
> >   		else
> >   			iovnum = 2;
> 
> Can num_sgs be 0 here. See comment above where you check valid urb case.
> There is a diconnect here?

urb-> transfer_buffer and urb-> num_sgs are different in their usage.
If a driver uses a normal buffer instead of SG, it uses transfer
buffer and num_sgs is set to 0. However, if the driver uses SG buffer,
urb->num_sgs and urb->sg are used and urb->transfer_buffer is set to
NULL.

Other usb codes use urb->num_sgs or urb->num_mapped_urb as the SG
usage indicator. So the above code that checks URB buffer validation
decides that if transfer_buffer and num_sgs is all 0, there is no
buffer. Considering that, I think this code is not a problem.

> > @@ -192,6 +200,15 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> >   		setup_ret_submit_pdu(&pdu_header, urb);
> >   		usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
> >   				  pdu_header.base.seqnum);
> > +
> > +		if (priv->sgl) {
> > +			for (i = 0; i < priv->num_urbs; i++)
> > +				actual_length += priv->urbs[i]->actual_length;
> > +
> > +			pdu_header.u.ret_submit.status = priv->urb_status;
> > +			pdu_header.u.ret_submit.actual_length = actual_length;
> > +		}
> > +
> >   		usbip_header_correct_endian(&pdu_header, 1);
> >   		iov[iovnum].iov_base = &pdu_header;
> > @@ -200,12 +217,47 @@ static int stub_send_ret_submit(struct stub_device *sdev)
> >   		txsize += sizeof(pdu_header);
> >   		/* 2. setup transfer buffer */
> > -		if (usb_pipein(urb->pipe) &&
> > +		if (usb_pipein(urb->pipe) && priv->sgl) {
> > +			/* If the server split a single SG request into several
> > +			 * URBs because the server's HCD doesn't support SG,
> > +			 * reassemble the split URB buffers into a single
> > +			 * return command.
> > +			 */
> > +			for (i = 0; i < priv->num_urbs; i++) {
> > +				iov[iovnum].iov_base =
> > +					priv->urbs[i]->transfer_buffer;
> > +				iov[iovnum].iov_len =
> > +					priv->urbs[i]->actual_length;
> > +				iovnum++;
> > +			}
> > +			txsize += actual_length;
> > +		} else if (usb_pipein(urb->pipe) &&
> >   		    usb_pipetype(urb->pipe) != PIPE_ISOCHRONOUS &&
> >   		    urb->actual_length > 0) {
> > -			iov[iovnum].iov_base = urb->transfer_buffer;
> > -			iov[iovnum].iov_len  = urb->actual_length;
> > -			iovnum++;
> > +			if (urb->num_sgs) {
> > +				unsigned int copy = urb->actual_length;
> > +				int size;
> > +
> > +				for_each_sg(urb->sg, sg, urb->num_sgs ,i) {
> > +					if (copy == 0)
> > +						break;
> > +
> > +					if (copy < sg->length)
> > +						size = copy;
> > +					else
> > +						size = sg->length;
> > +
> > +					iov[iovnum].iov_base = sg_virt(sg);
> > +					iov[iovnum].iov_len = size;
> > +
> > +					iovnum++;
> > +					copy -= size;
> > +				}
> > +			} else {
> > +				iov[iovnum].iov_base = urb->transfer_buffer;
> > +				iov[iovnum].iov_len  = urb->actual_length;
> > +				iovnum++;
> > +			}
> >   			txsize += urb->actual_length;
> >   		} else if (usb_pipein(urb->pipe) &&
> >   			   usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
> > diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
> > index 45da3e01c7b0..6affbfcfc927 100644
> > --- a/drivers/usb/usbip/usbip_common.c
> > +++ b/drivers/usb/usbip/usbip_common.c
> > @@ -680,8 +680,12 @@ EXPORT_SYMBOL_GPL(usbip_pad_iso);
> >   /* some members of urb must be substituted before. */
> >   int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
> >   {
> > -	int ret;
> > +	struct scatterlist *sg;
> > +	int ret = 0;
> > +	int recv;
> >   	int size;
> > +	int copy;
> > +	int i;
> >   	if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> >   		/* the direction of urb must be OUT. */
> > @@ -712,14 +716,52 @@ int usbip_recv_xbuff(struct usbip_device *ud, struct urb *urb)
> >   		}
> >   	}
> > -	ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
> > -	if (ret != size) {
> > -		dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> > -		if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> > -			usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> > -		} else {
> > -			usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> > -			return -EPIPE;
> > +	if (urb->num_sgs) {
> > +		copy = size;
> > +		for_each_sg(urb->sg, sg, urb->num_sgs, i) {
> > +			int recv_size;
> > +
> > +			if (copy < sg->length)
> > +				recv_size = copy;
> > +			else
> > +				recv_size = sg->length;
> > +
> > +			recv = usbip_recv(ud->tcp_socket, sg_virt(sg),
> > +						recv_size);
> > +
> > +			if (recv != recv_size) {
> > +				dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> > +				if (ud->side == USBIP_STUB ||
> > +				    ud->side == USBIP_VUDC) {
> > +					usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> 
> This doesn't look right. You continue to send after adding
> SDEV_EVENT_ERROR_TCP ?? Shouldn't you bail out instead?

This is my fault. I will fix it.

> > +				} else {
> > +					usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> > +					return -EPIPE;
> > +				}
> > +			}
> > +			copy -= recv;
> > +			ret += recv;
> > +		}
> > +
> > +		if (ret != size) {
> > +			dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> > +			if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> > +				usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> > +			} else {
> > +				usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> > +				return -EPIPE;
> > +			}
> > +		}
> 
> Get rid of the duplicate code block for usbip_recv() error paths.
> 
> > +	} else {
> > +		ret = usbip_recv(ud->tcp_socket, urb->transfer_buffer, size);
> > +		if (ret != size) {
> > +			dev_err(&urb->dev->dev, "recv xbuf, %d\n", ret);
> > +			if (ud->side == USBIP_STUB || ud->side == USBIP_VUDC) {
> > +				usbip_event_add(ud, SDEV_EVENT_ERROR_TCP);
> > +			} else {
> > +				usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
> > +				return -EPIPE;
> > +			}
> >   		}
> 
> Get rid of the duplicate code block for usbip_recv() error paths.
> There are 3 now and the above in the loop doesn't look right.

Ok, I will get rid of the duplication.

> >   	}
> > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> > index 14fc6d9f4e6a..b5fe85adb42b 100644
> > --- a/drivers/usb/usbip/vhci_hcd.c
> > +++ b/drivers/usb/usbip/vhci_hcd.c
> > @@ -697,7 +697,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) {
> 
> Is there a case where transfer_buffer is valid without any num_sgs?

num_sgs must always be 0 for transfer_buffer to be valid.

> >   		dev_dbg(dev, "Null URB transfer buffer\n");
> >   		return -EINVAL;
> >   	}
> > @@ -1143,6 +1144,13 @@ static int vhci_setup(struct usb_hcd *hcd)
> >   		hcd->speed = HCD_USB3;
> >   		hcd->self.root_hub->speed = USB_SPEED_SUPER;
> >   	}
> > +
> > +	/* support SG.
> > +	 * sg_tablesize is an arbitrary vaule to alleviate memory pressure
> > +	 * on the host. */
> > +	hcd->self.sg_tablesize = 32;
> > +	hcd->self.no_sg_constraint = 1;
> > +
> >   	return 0;
> >   }
> > diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c
> > index 2fa26d0578d7..3472180f5af8 100644
> > --- a/drivers/usb/usbip/vhci_tx.c
> > +++ b/drivers/usb/usbip/vhci_tx.c
> > @@ -5,6 +5,7 @@
> >   #include <linux/kthread.h>
> >   #include <linux/slab.h>
> > +#include <linux/scatterlist.h>
> >   #include "usbip_common.h"
> >   #include "vhci.h"
> > @@ -51,12 +52,13 @@ static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
> >   static int vhci_send_cmd_submit(struct vhci_device *vdev)
> >   {
> >   	struct vhci_priv *priv = NULL;
> > -
> > +	struct scatterlist *sg;
> >   	struct msghdr msg;
> > -	struct kvec iov[3];
> > +	struct kvec *iov;
> >   	size_t txsize;
> > -
> >   	size_t total_size = 0;
> > +	int iovnum;
> > +	int i;
> >   	while ((priv = dequeue_from_priv_tx(vdev)) != NULL) {
> >   		int ret;
> > @@ -72,18 +74,41 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
> >   		usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n",
> >   				  priv->seqnum);
> > +		if (urb->num_sgs && usb_pipeout(urb->pipe))
> > +			iovnum = 2 + urb->num_sgs;
> 
> Don't you want to do some kind of range check on num_sgs?

I don't think I need it. I have not seen the code that checks the
range of num_sgs.

> > +		else
> > +			iovnum = 3;
> > +
> > +		iov = kzalloc(iovnum * sizeof(*iov), GFP_KERNEL);
> > +		if (!iov) {
> > +			usbip_event_add(&vdev->ud,
> > +						SDEV_EVENT_ERROR_MALLOC);
> > +			return -ENOMEM;
> > +		}
> > +
> >   		/* 1. setup usbip_header */
> >   		setup_cmd_submit_pdu(&pdu_header, urb);
> >   		usbip_header_correct_endian(&pdu_header, 1);
> > +		iovnum = 0;
> > -		iov[0].iov_base = &pdu_header;
> > -		iov[0].iov_len  = sizeof(pdu_header);
> > +		iov[iovnum].iov_base = &pdu_header;
> > +		iov[iovnum].iov_len  = sizeof(pdu_header);
> >   		txsize += sizeof(pdu_header);
> > +		iovnum++;
> >   		/* 2. setup transfer buffer */
> >   		if (!usb_pipein(urb->pipe) && urb->transfer_buffer_length > 0) {
> > -			iov[1].iov_base = urb->transfer_buffer;
> > -			iov[1].iov_len  = urb->transfer_buffer_length;
> > +			if (urb->num_sgs && !usb_endpoint_xfer_isoc(&urb->ep->desc)) {
> > +				for_each_sg(urb->sg, sg, urb->num_sgs ,i) {
> > +					iov[iovnum].iov_base = sg_virt(sg);
> > +					iov[iovnum].iov_len = sg->length;
> > +					iovnum++;
> > +				}
> > +			} else {
> > +				iov[iovnum].iov_base = urb->transfer_buffer;
> > +				iov[iovnum].iov_len  = urb->transfer_buffer_length;
> > +				iovnum++;
> 
> You would have allocated 2 + urb->num_sgs for the isochronous
> case as well and not use them?

Isoc pipe doesn't use SG (only urb->transfer_buffer is used).
So iovnum = 3 in the above conditional.

In usb_hcd_map_urb_for_dma(), it checks whether isoc pipe uses SG,
and I will also add this logic to vhci_map_urb_for_dma() of patch 1.

> > +			}
> >   			txsize += urb->transfer_buffer_length;
> >   		}
> > @@ -93,25 +118,29 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev)
> >   			iso_buffer = usbip_alloc_iso_desc_pdu(urb, &len);
> >   			if (!iso_buffer) {
> > +				kfree(iov);
> 
> Consolidate error handling for kfree(iov)
> 
> >   				usbip_event_add(&vdev->ud,
> >   						SDEV_EVENT_ERROR_MALLOC);
> >   				return -1;
> >   			}
> > -			iov[2].iov_base = iso_buffer;
> > -			iov[2].iov_len  = len;
> > +			iov[iovnum].iov_base = iso_buffer;
> > +			iov[iovnum].iov_len  = len;
> > +			iovnum++;
> >   			txsize += len;
> >   		}
> > -		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, 3, txsize);
> > +		ret = kernel_sendmsg(vdev->ud.tcp_socket, &msg, iov, iovnum, txsize);
> >   		if (ret != txsize) {
> >   			pr_err("sendmsg failed!, ret=%d for %zd\n", ret,
> >   			       txsize);
> > +			kfree(iov);
> >   			kfree(iso_buffer);
> 
> Consolidate error handling for kfree(iov) and kfree(iso_buffer)

Ok.

Regards
Suwan Kim

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[shuah]

On 7/29/19 8:52 AM, Suwan Kim wrote:
> Hi Shuah,
> 
> On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
>> Hi Suwan,
>>
>> On 7/5/19 10:43 AM, 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 breaks SG list
>>> into multiple URBs and it causes error 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 the 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 vhci.
>>>
>>> If HCD of the server doesn't support SG, the stub driver breaks a
>>> single SG reqeust into several URBs and submit them to the server's
>>> HCD. When all the split URBs are completed, the stub driver
>>> reassembles the URBs into a single return command and sends it to
>>> vhci.
>>>
>>> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
>>> ---
>>>    drivers/usb/usbip/stub.h         |   7 +-
>>>    drivers/usb/usbip/stub_main.c    |  52 +++++---
>>>    drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
>>>    drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
>>>    drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
>>>    drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
>>>    7 files changed, 372 insertions(+), 121 deletions(-)
>>
>> While you are working on v3 to fix chekpatch and other issues
>> I pointed out, I have more for you.
>>
>> What happens when you have mismatched server and client side?
>> i.e stub does and vhci doesn't and vice versa.
>>
>> Make sure to run checkpatch. Also check spelling errors in
>> comments and your commit log.
>>
>> I am not sure if your eeror paths are correct. Run usbip_test.sh
>>
>> tools/testing/selftests/drivers/usb/usbip
> 
> I don't know what mismatch you mentioned. Are you saying
> "match busid table" at the end of usbip_test.sh?
> How does it relate to SG support of this patch?
> Could you tell me more about the problem situation?
> 

What happens when usbip_host is running a kernel without the sg
support and vhci_hcd does? Just make sure this works with the
checks for sg support status as a one of your tests for this
sg feature.

Looking forward to seeing v3 soon!

thanks,
-- Shuah

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[Suwan Kim]

On Mon, Jul 29, 2019 at 10:32:31AM -0600, shuah wrote:
> On 7/29/19 8:52 AM, Suwan Kim wrote:
> > Hi Shuah,
> > 
> > On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
> > > Hi Suwan,
> > > 
> > > On 7/5/19 10:43 AM, 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 breaks SG list
> > > > into multiple URBs and it causes error 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 the 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 vhci.
> > > > 
> > > > If HCD of the server doesn't support SG, the stub driver breaks a
> > > > single SG reqeust into several URBs and submit them to the server's
> > > > HCD. When all the split URBs are completed, the stub driver
> > > > reassembles the URBs into a single return command and sends it to
> > > > vhci.
> > > > 
> > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > > > ---
> > > >    drivers/usb/usbip/stub.h         |   7 +-
> > > >    drivers/usb/usbip/stub_main.c    |  52 +++++---
> > > >    drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
> > > >    drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
> > > >    drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
> > > >    drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
> > > >    7 files changed, 372 insertions(+), 121 deletions(-)
> > > 
> > > While you are working on v3 to fix chekpatch and other issues
> > > I pointed out, I have more for you.
> > > 
> > > What happens when you have mismatched server and client side?
> > > i.e stub does and vhci doesn't and vice versa.
> > > 
> > > Make sure to run checkpatch. Also check spelling errors in
> > > comments and your commit log.
> > > 
> > > I am not sure if your eeror paths are correct. Run usbip_test.sh
> > > 
> > > tools/testing/selftests/drivers/usb/usbip
> > 
> > I don't know what mismatch you mentioned. Are you saying
> > "match busid table" at the end of usbip_test.sh?
> > How does it relate to SG support of this patch?
> > Could you tell me more about the problem situation?
> > 
> 
> What happens when usbip_host is running a kernel without the sg
> support and vhci_hcd does? Just make sure this works with the
> checks for sg support status as a one of your tests for this
> sg feature.

Now I understand. Thanks for the details!
As a result of testing, in the situation where vhci supports SG,
but stub does not, or vice versa, usbip works normally. Moreover,
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 doesn't, because vhci sends
only the total length of the buffer to stub as it did before the
patch applied, stub only needs to allocate the required length of
buffers regardless of whether vhci supports SG or not.

If stub needs to send data buffer to vhci because of IN pipe, stub
also sends only total length of buffer as metadata and then send real
data as vhci does. Then vhci receive data from stub and store it to
the corresponding buffer of SG list entry.

In the case of stub that supports SG, if SG buffer is requested by
vhci, buffer is allocated by sgl_alloc(). However, stub that does
not support SG allocates buffer using only kmalloc(). Therefore, if
vhci supports SG and stub doesn't, stub 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 supports SG and vhci doesn't, since the
USB storage driver checks that vhci doesn't support SG and sends
the request to stub by splitting the SG list into multiple URBs,
stub allocates a buffer with kmalloc() as it did before this patch.

Therefore, everything works normally in a mismatch situation.
I will send the v3 soon.

Regards
Suwan Kim

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[shuah]

On 8/1/19 12:38 AM, Suwan Kim wrote:
> On Mon, Jul 29, 2019 at 10:32:31AM -0600, shuah wrote:
>> On 7/29/19 8:52 AM, Suwan Kim wrote:
>>> Hi Shuah,
>>>
>>> On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
>>>> Hi Suwan,
>>>>
>>>> On 7/5/19 10:43 AM, 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 breaks SG list
>>>>> into multiple URBs and it causes error 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 the 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 vhci.
>>>>>
>>>>> If HCD of the server doesn't support SG, the stub driver breaks a
>>>>> single SG reqeust into several URBs and submit them to the server's
>>>>> HCD. When all the split URBs are completed, the stub driver
>>>>> reassembles the URBs into a single return command and sends it to
>>>>> vhci.
>>>>>
>>>>> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
>>>>> ---
>>>>>     drivers/usb/usbip/stub.h         |   7 +-
>>>>>     drivers/usb/usbip/stub_main.c    |  52 +++++---
>>>>>     drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
>>>>>     drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
>>>>>     drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
>>>>>     drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
>>>>>     7 files changed, 372 insertions(+), 121 deletions(-)
>>>>
>>>> While you are working on v3 to fix chekpatch and other issues
>>>> I pointed out, I have more for you.
>>>>
>>>> What happens when you have mismatched server and client side?
>>>> i.e stub does and vhci doesn't and vice versa.
>>>>
>>>> Make sure to run checkpatch. Also check spelling errors in
>>>> comments and your commit log.
>>>>
>>>> I am not sure if your eeror paths are correct. Run usbip_test.sh
>>>>
>>>> tools/testing/selftests/drivers/usb/usbip
>>>
>>> I don't know what mismatch you mentioned. Are you saying
>>> "match busid table" at the end of usbip_test.sh?
>>> How does it relate to SG support of this patch?
>>> Could you tell me more about the problem situation?
>>>
>>
>> What happens when usbip_host is running a kernel without the sg
>> support and vhci_hcd does? Just make sure this works with the
>> checks for sg support status as a one of your tests for this
>> sg feature.
> 
> Now I understand. Thanks for the details!
> As a result of testing, in the situation where vhci supports SG,
> but stub does not, or vice versa, usbip works normally. Moreover,
> 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 doesn't, because vhci sends
> only the total length of the buffer to stub as it did before the
> patch applied, stub only needs to allocate the required length of
> buffers regardless of whether vhci supports SG or not.
> 
> If stub needs to send data buffer to vhci because of IN pipe, stub
> also sends only total length of buffer as metadata and then send real
> data as vhci does. Then vhci receive data from stub and store it to
> the corresponding buffer of SG list entry.
> 
> In the case of stub that supports SG, if SG buffer is requested by
> vhci, buffer is allocated by sgl_alloc(). However, stub that does
> not support SG allocates buffer using only kmalloc(). Therefore, if
> vhci supports SG and stub doesn't, stub 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 supports SG and vhci doesn't, since the
> USB storage driver checks that vhci doesn't support SG and sends
> the request to stub by splitting the SG list into multiple URBs,
> stub allocates a buffer with kmalloc() as it did before this patch.
> 
> Therefore, everything works normally in a mismatch situation.

Looking for you add a test case for that. Please include this
in the commit log.

> I will send the v3 soon.
> 

Please send it soon. It would be nice to have this done as soon
as possible.

thanks,
-- Shuah

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[Suwan Kim]

On Thu, Aug 01, 2019 at 08:03:59AM -0600, shuah wrote:
> On 8/1/19 12:38 AM, Suwan Kim wrote:
> > On Mon, Jul 29, 2019 at 10:32:31AM -0600, shuah wrote:
> > > On 7/29/19 8:52 AM, Suwan Kim wrote:
> > > > Hi Shuah,
> > > > 
> > > > On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
> > > > > Hi Suwan,
> > > > > 
> > > > > On 7/5/19 10:43 AM, 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 breaks SG list
> > > > > > into multiple URBs and it causes error 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 the 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 vhci.
> > > > > > 
> > > > > > If HCD of the server doesn't support SG, the stub driver breaks a
> > > > > > single SG reqeust into several URBs and submit them to the server's
> > > > > > HCD. When all the split URBs are completed, the stub driver
> > > > > > reassembles the URBs into a single return command and sends it to
> > > > > > vhci.
> > > > > > 
> > > > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > > > > > ---
> > > > > >     drivers/usb/usbip/stub.h         |   7 +-
> > > > > >     drivers/usb/usbip/stub_main.c    |  52 +++++---
> > > > > >     drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
> > > > > >     drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
> > > > > >     drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
> > > > > >     drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
> > > > > >     7 files changed, 372 insertions(+), 121 deletions(-)
> > > > > 
> > > > > While you are working on v3 to fix chekpatch and other issues
> > > > > I pointed out, I have more for you.
> > > > > 
> > > > > What happens when you have mismatched server and client side?
> > > > > i.e stub does and vhci doesn't and vice versa.
> > > > > 
> > > > > Make sure to run checkpatch. Also check spelling errors in
> > > > > comments and your commit log.
> > > > > 
> > > > > I am not sure if your eeror paths are correct. Run usbip_test.sh
> > > > > 
> > > > > tools/testing/selftests/drivers/usb/usbip
> > > > 
> > > > I don't know what mismatch you mentioned. Are you saying
> > > > "match busid table" at the end of usbip_test.sh?
> > > > How does it relate to SG support of this patch?
> > > > Could you tell me more about the problem situation?
> > > > 
> > > 
> > > What happens when usbip_host is running a kernel without the sg
> > > support and vhci_hcd does? Just make sure this works with the
> > > checks for sg support status as a one of your tests for this
> > > sg feature.
> > 
> > Now I understand. Thanks for the details!
> > As a result of testing, in the situation where vhci supports SG,
> > but stub does not, or vice versa, usbip works normally. Moreover,
> > 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 doesn't, because vhci sends
> > only the total length of the buffer to stub as it did before the
> > patch applied, stub only needs to allocate the required length of
> > buffers regardless of whether vhci supports SG or not.
> > 
> > If stub needs to send data buffer to vhci because of IN pipe, stub
> > also sends only total length of buffer as metadata and then send real
> > data as vhci does. Then vhci receive data from stub and store it to
> > the corresponding buffer of SG list entry.
> > 
> > In the case of stub that supports SG, if SG buffer is requested by
> > vhci, buffer is allocated by sgl_alloc(). However, stub that does
> > not support SG allocates buffer using only kmalloc(). Therefore, if
> > vhci supports SG and stub doesn't, stub 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 supports SG and vhci doesn't, since the
> > USB storage driver checks that vhci doesn't support SG and sends
> > the request to stub by splitting the SG list into multiple URBs,
> > stub allocates a buffer with kmalloc() as it did before this patch.
> > 
> > Therefore, everything works normally in a mismatch situation.
> 
> Looking for you add a test case for that. Please include this
> in the commit log.

I'm confused about the test case. Do I add the test case for each
SG support status of vhci_hcd and usbip_host in usbip_test.sh?
Or, do I implement the test logic in vhci_hcd code that asks if
usbip_host supports SG when attaching a remote device?
I'm sorry but I don't know what exactly you want to add.

Regards
Suwan Kim

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[shuah]

On 8/2/19 1:41 AM, Suwan Kim wrote:
> On Thu, Aug 01, 2019 at 08:03:59AM -0600, shuah wrote:
>> On 8/1/19 12:38 AM, Suwan Kim wrote:
>>> On Mon, Jul 29, 2019 at 10:32:31AM -0600, shuah wrote:
>>>> On 7/29/19 8:52 AM, Suwan Kim wrote:
>>>>> Hi Shuah,
>>>>>
>>>>> On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
>>>>>> Hi Suwan,
>>>>>>
>>>>>> On 7/5/19 10:43 AM, 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 breaks SG list
>>>>>>> into multiple URBs and it causes error 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 the 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 vhci.
>>>>>>>
>>>>>>> If HCD of the server doesn't support SG, the stub driver breaks a
>>>>>>> single SG reqeust into several URBs and submit them to the server's
>>>>>>> HCD. When all the split URBs are completed, the stub driver
>>>>>>> reassembles the URBs into a single return command and sends it to
>>>>>>> vhci.
>>>>>>>
>>>>>>> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
>>>>>>> ---
>>>>>>>      drivers/usb/usbip/stub.h         |   7 +-
>>>>>>>      drivers/usb/usbip/stub_main.c    |  52 +++++---
>>>>>>>      drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
>>>>>>>      drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
>>>>>>>      drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
>>>>>>>      drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
>>>>>>>      7 files changed, 372 insertions(+), 121 deletions(-)
>>>>>>
>>>>>> While you are working on v3 to fix chekpatch and other issues
>>>>>> I pointed out, I have more for you.
>>>>>>
>>>>>> What happens when you have mismatched server and client side?
>>>>>> i.e stub does and vhci doesn't and vice versa.
>>>>>>
>>>>>> Make sure to run checkpatch. Also check spelling errors in
>>>>>> comments and your commit log.
>>>>>>
>>>>>> I am not sure if your eeror paths are correct. Run usbip_test.sh
>>>>>>
>>>>>> tools/testing/selftests/drivers/usb/usbip
>>>>>
>>>>> I don't know what mismatch you mentioned. Are you saying
>>>>> "match busid table" at the end of usbip_test.sh?
>>>>> How does it relate to SG support of this patch?
>>>>> Could you tell me more about the problem situation?
>>>>>
>>>>
>>>> What happens when usbip_host is running a kernel without the sg
>>>> support and vhci_hcd does? Just make sure this works with the
>>>> checks for sg support status as a one of your tests for this
>>>> sg feature.
>>>
>>> Now I understand. Thanks for the details!
>>> As a result of testing, in the situation where vhci supports SG,
>>> but stub does not, or vice versa, usbip works normally. Moreover,
>>> 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 doesn't, because vhci sends
>>> only the total length of the buffer to stub as it did before the
>>> patch applied, stub only needs to allocate the required length of
>>> buffers regardless of whether vhci supports SG or not.
>>>
>>> If stub needs to send data buffer to vhci because of IN pipe, stub
>>> also sends only total length of buffer as metadata and then send real
>>> data as vhci does. Then vhci receive data from stub and store it to
>>> the corresponding buffer of SG list entry.
>>>
>>> In the case of stub that supports SG, if SG buffer is requested by
>>> vhci, buffer is allocated by sgl_alloc(). However, stub that does
>>> not support SG allocates buffer using only kmalloc(). Therefore, if
>>> vhci supports SG and stub doesn't, stub 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 supports SG and vhci doesn't, since the
>>> USB storage driver checks that vhci doesn't support SG and sends
>>> the request to stub by splitting the SG list into multiple URBs,
>>> stub allocates a buffer with kmalloc() as it did before this patch.
>>>
>>> Therefore, everything works normally in a mismatch situation.
>>
>> Looking for you add a test case for that. Please include this
>> in the commit log.
> 
> I'm confused about the test case. Do I add the test case for each
> SG support status of vhci_hcd and usbip_host in usbip_test.sh?
> Or, do I implement the test logic in vhci_hcd code that asks if
> usbip_host supports SG when attaching a remote device?
> I'm sorry but I don't know what exactly you want to add.
> 

What I am asking you do is:

1. test with mismatched host and client.
2. run usbip_test.sh

These two are separate tests. I am not asking you to add any tests.
If you want to add it, I am not going say no :)

How close are you sending the patch?

thanks,
-- Shuah

Re: [PATCH v2 2/2] usbip: Implement SG support to vhci

[Suwan Kim]

On Fri, Aug 02, 2019 at 07:33:59AM -0600, shuah wrote:
> On 8/2/19 1:41 AM, Suwan Kim wrote:
> > On Thu, Aug 01, 2019 at 08:03:59AM -0600, shuah wrote:
> > > On 8/1/19 12:38 AM, Suwan Kim wrote:
> > > > On Mon, Jul 29, 2019 at 10:32:31AM -0600, shuah wrote:
> > > > > On 7/29/19 8:52 AM, Suwan Kim wrote:
> > > > > > Hi Shuah,
> > > > > > 
> > > > > > On Tue, Jul 23, 2019 at 06:21:53PM -0600, shuah wrote:
> > > > > > > Hi Suwan,
> > > > > > > 
> > > > > > > On 7/5/19 10:43 AM, 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 breaks SG list
> > > > > > > > into multiple URBs and it causes error 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 the 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 vhci.
> > > > > > > > 
> > > > > > > > If HCD of the server doesn't support SG, the stub driver breaks a
> > > > > > > > single SG reqeust into several URBs and submit them to the server's
> > > > > > > > HCD. When all the split URBs are completed, the stub driver
> > > > > > > > reassembles the URBs into a single return command and sends it to
> > > > > > > > vhci.
> > > > > > > > 
> > > > > > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
> > > > > > > > ---
> > > > > > > >      drivers/usb/usbip/stub.h         |   7 +-
> > > > > > > >      drivers/usb/usbip/stub_main.c    |  52 +++++---
> > > > > > > >      drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
> > > > > > > >      drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
> > > > > > > >      drivers/usb/usbip/usbip_common.c |  60 +++++++-- >   drivers/usb/usbip/vhci_hcd.c     |  10 +-
> > > > > > > >      drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
> > > > > > > >      7 files changed, 372 insertions(+), 121 deletions(-)
> > > > > > > 
> > > > > > > While you are working on v3 to fix chekpatch and other issues
> > > > > > > I pointed out, I have more for you.
> > > > > > > 
> > > > > > > What happens when you have mismatched server and client side?
> > > > > > > i.e stub does and vhci doesn't and vice versa.
> > > > > > > 
> > > > > > > Make sure to run checkpatch. Also check spelling errors in
> > > > > > > comments and your commit log.
> > > > > > > 
> > > > > > > I am not sure if your eeror paths are correct. Run usbip_test.sh
> > > > > > > 
> > > > > > > tools/testing/selftests/drivers/usb/usbip
> > > > > > 
> > > > > > I don't know what mismatch you mentioned. Are you saying
> > > > > > "match busid table" at the end of usbip_test.sh?
> > > > > > How does it relate to SG support of this patch?
> > > > > > Could you tell me more about the problem situation?
> > > > > > 
> > > > > 
> > > > > What happens when usbip_host is running a kernel without the sg
> > > > > support and vhci_hcd does? Just make sure this works with the
> > > > > checks for sg support status as a one of your tests for this
> > > > > sg feature.
> > > > 
> > > > Now I understand. Thanks for the details!
> > > > As a result of testing, in the situation where vhci supports SG,
> > > > but stub does not, or vice versa, usbip works normally. Moreover,
> > > > 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 doesn't, because vhci sends
> > > > only the total length of the buffer to stub as it did before the
> > > > patch applied, stub only needs to allocate the required length of
> > > > buffers regardless of whether vhci supports SG or not.
> > > > 
> > > > If stub needs to send data buffer to vhci because of IN pipe, stub
> > > > also sends only total length of buffer as metadata and then send real
> > > > data as vhci does. Then vhci receive data from stub and store it to
> > > > the corresponding buffer of SG list entry.
> > > > 
> > > > In the case of stub that supports SG, if SG buffer is requested by
> > > > vhci, buffer is allocated by sgl_alloc(). However, stub that does
> > > > not support SG allocates buffer using only kmalloc(). Therefore, if
> > > > vhci supports SG and stub doesn't, stub 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 supports SG and vhci doesn't, since the
> > > > USB storage driver checks that vhci doesn't support SG and sends
> > > > the request to stub by splitting the SG list into multiple URBs,
> > > > stub allocates a buffer with kmalloc() as it did before this patch.
> > > > 
> > > > Therefore, everything works normally in a mismatch situation.
> > > 
> > > Looking for you add a test case for that. Please include this
> > > in the commit log.
> > 
> > I'm confused about the test case. Do I add the test case for each
> > SG support status of vhci_hcd and usbip_host in usbip_test.sh?
> > Or, do I implement the test logic in vhci_hcd code that asks if
> > usbip_host supports SG when attaching a remote device?
> > I'm sorry but I don't know what exactly you want to add.
> > 
> 
> What I am asking you do is:
> 
> 1. test with mismatched host and client.

I already tested this case with two different machines. One is the
5.2-rc6 kernel with SG patch and the other is default fedora kernel
version 5.1.20-200.fc29.x86_64. As I said, I run vhci_hcd with SG
and usbip_host without SG and also vice versa.

I tested it with Sandisk USB 3.0 storage deivce and tested read and
write operation which transfer files whose size is over 1G like movie
file. All the opreations work well.

> 2. run usbip_test.sh

One thing goes wrong. But others except the one worked as expected.

==============================================================
List imported devices - expect to see imported devices
Imported USB devices
====================
Port 08: <Port in Use> at Super Speed(5000Mbps)
       SanDisk Corp. : Ultra Flair (0781:5591)
       4-1 -> usbip://localhost:3240/2-6
           -> remote bus/dev 002/003
==============================================================
Import devices from localhost - expect already imported messages
usbip: error: Attach Request for 2-6 failed - Device busy (exported)

==============================================================
Un-import devices
usbip: info: Port 0 is already detached!

usbip: info: Port 1 is already detached!

==============================================================
List imported devices - expect to see none
Imported USB devices
====================
Port 08: <Port in Use> at Super Speed(5000Mbps)
       SanDisk Corp. : Ultra Flair (0781:5591)
       4-1 -> usbip://localhost:3240/2-6
           -> remote bus/dev 002/003
==============================================================

My SanDisk device is bound port 8 but the test script detachs only
port 0 and 1. This error is not related with SG patch but we have to
modifiy the test script to try to detach all the ports.

> These two are separate tests. I am not asking you to add any tests.
> If you want to add it, I am not going say no :)
> 
> How close are you sending the patch?

My work is done. I will send it now.

Regards
Suwan Kim

Re: [PATCH v2 0/2] usbip: Implement SG support

[shuah]

On 7/5/19 10:43 AM, 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 breaks SG list
> into multiple URBs and it causes error that a transfer got terminated
> too early because the transfer length for one of the URBs was not
> divisible by the maxpacket size.
> 
> To support SG, vhci doesn't map and unmap URB for DMA to use native
> SG list (urb->num_sgs). In DMA mapping function of vhci, it sets
> URB_DMA_MAP_SG flag in urb->transfer_flags if URB has SG list and
> this flag will tell the stub driver to use SG list.
> 
> In this patch, vhci basically support SG and it sends each SG list
> entry to the stub driver. Then, the 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 the stub driver receives
> completed URB, it again sends each SG list entry to vhci.
> 
> If HCD of the server doesn't support SG, the stub driver breaks a
> single SG reqeust into several URBs and submit them to the server's
> HCD. When all the split URBs are completed, the stub driver
> reassembles the URBs into a single return command and sends it to
> vhci.
> 
> 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.
> 
> Suwan Kim (2):
>    usbip: Skip DMA mapping and unmapping for urb at vhci
>    usbip: Implement SG support to vhci
> 
>   drivers/usb/usbip/stub.h         |   7 +-
>   drivers/usb/usbip/stub_main.c    |  52 +++++---
>   drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
>   drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
>   drivers/usb/usbip/usbip_common.c |  60 +++++++--
>   drivers/usb/usbip/vhci_hcd.c     |  29 ++++-
>   drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
>   7 files changed, 391 insertions(+), 121 deletions(-)
> 

Hi Suwan,

I have been traveling and would like to test this series before I ask
Greg to pick it up.

Just a quick note that I will get to this early next week.

thanks,
-- Shuah

Re: [PATCH v2 0/2] usbip: Implement SG support

[Suwan Kim]

On Fri, Jul 19, 2019 at 04:04:20PM -0600, shuah wrote:
> On 7/5/19 10:43 AM, 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 breaks SG list
> > into multiple URBs and it causes error that a transfer got terminated
> > too early because the transfer length for one of the URBs was not
> > divisible by the maxpacket size.
> > 
> > To support SG, vhci doesn't map and unmap URB for DMA to use native
> > SG list (urb->num_sgs). In DMA mapping function of vhci, it sets
> > URB_DMA_MAP_SG flag in urb->transfer_flags if URB has SG list and
> > this flag will tell the stub driver to use SG list.
> > 
> > In this patch, vhci basically support SG and it sends each SG list
> > entry to the stub driver. Then, the 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 the stub driver receives
> > completed URB, it again sends each SG list entry to vhci.
> > 
> > If HCD of the server doesn't support SG, the stub driver breaks a
> > single SG reqeust into several URBs and submit them to the server's
> > HCD. When all the split URBs are completed, the stub driver
> > reassembles the URBs into a single return command and sends it to
> > vhci.
> > 
> > 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.
> > 
> > Suwan Kim (2):
> >    usbip: Skip DMA mapping and unmapping for urb at vhci
> >    usbip: Implement SG support to vhci
> > 
> >   drivers/usb/usbip/stub.h         |   7 +-
> >   drivers/usb/usbip/stub_main.c    |  52 +++++---
> >   drivers/usb/usbip/stub_rx.c      | 207 ++++++++++++++++++++++---------
> >   drivers/usb/usbip/stub_tx.c      | 108 +++++++++++-----
> >   drivers/usb/usbip/usbip_common.c |  60 +++++++--
> >   drivers/usb/usbip/vhci_hcd.c     |  29 ++++-
> >   drivers/usb/usbip/vhci_tx.c      |  49 ++++++--
> >   7 files changed, 391 insertions(+), 121 deletions(-)
> > 
> 
> Hi Suwan,
> 
> I have been traveling and would like to test this series before I ask
> Greg to pick it up.
> 
> Just a quick note that I will get to this early next week.

Ok. Thank you for reviewing the patch, Shuah.
Please let me know if you have any problems reviewing the patch :)

Regards

Suwan Kim