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:3.581 / U:0.364 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site