Re: [PATCH v2 2/2] usbip: Implement SG support to vhci
From: shuah
Date: Mon Jul 22 2019 - 23:51:34 EST
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@xxxxxxxxx>
---
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