lkml.org 
[lkml]   [2019]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/2] usbip: Implement SG support to vhci
From
Date
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

\
 
 \ /
  Last update: 2019-07-23 05:52    [W:0.133 / U:7.472 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site