Re: gs_start_io() and gs_close()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



As I mentioned, I'd like to see a few
Tested-By: reports confirming this
patch works, before this gets into
Greg's merge queue ...

So -- can folk using the serial gadget code
please give this a whirl and see if the patch
behaves (or instead makes trouble).




--- On Mon, 8/30/10, Jim Sung <jsung@xxxxxxxxxxxxxx> wrote:

> From: Jim Sung <jsung@xxxxxxxxxxxxxx>
> Subject: Re: gs_start_io() and gs_close()
> To: "David Brownell" <david-b@xxxxxxxxxxx>
> Cc: "Igor Grinberg" <grinberg@xxxxxxxxxxxxxx>, linux-usb@xxxxxxxxxxxxxxx, "linux-arm-kernel" <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>
> Date: Monday, August 30, 2010, 6:30 PM
> Hi Dave:
> 
> >>>    seems wrong.
> > 
> > 
> > Does it cause an actual problem though?  Back
> when
> 
> The short answer is "yes", but not in a way that is easily
> noticeable.
> 
> > that code was written and submitted, ISTR doing a
> > leak analysis and finding none on start_io() paths
> > or their cleanup siblings.  Did something change
> 
> OK, the USB gadget serial driver actually has a couple of
> problems.  On
> gs_open(), it always allocates and queues an additional
> QUEUE_SIZE (16)
> worth of requests, so with a loop like this:
> 
>     i=1 ; while echo $i > /dev/ttyGS0 ; do let
> i++ ; done
> 
> eventually we run into OOM (Out of Memory).
> 
> Technically, it is not a leak as everything gets freed up
> when the USB
> connection is broken, but not on gs_close().
> 
> With a USB device/gadget controller driver that has limited
> resources
> (e.g., Marvell has a this MAX_XDS_FOR_TR_CALLS of 64 for
> transmit and
> receive), so even after 4
> 
>     stty -F /dev/ttyGS0
> 
> we cannot transmit anymore.  We can still receive (not
> necessarily
> reliably) as now we have 16 * 4 = 64 descriptors/buffers
> ready, but the
> device is otherwise not usable.
> 
> Got the
> 0001-pxa168-Dequeue-all-pending-USB-Req-s-submitted-to-U.patch
> from Marvell on 8-27-10, but it still has problems, so I'm
> not going to
> post it here.
> 
> > since then?
> 
> I don't believe so.
> 
> > I don't recall problems being reported in this area
> > over the past several years, either...
> 
> My take is that unless you explicitly look for it, you are
> probably not
> going to notice this.
> 
> Anyway, I tried not to change too much and to follow the
> basic design,
> but you have to be the judge of that.  I ran through a
> handful of tests,
> and it seemed to behave much better now.  Here is the
> patch (against
> 2.6.28), and you can decide whatever you want to do with
> it.
> 
> Jim
> 
> Index: drivers/usb/gadget/u_serial.c
> ===================================================================
> --- drivers/usb/gadget/u_serial.c   
>    (revision 10419)
> +++ drivers/usb/gadget/u_serial.c   
>    (working copy)
> @@ -103,11 +103,15 @@
>         wait_queue_head_t   
>    close_wait;     /*
> wait for last close */
> 
>         struct list_head   
>     read_pool;
> +       int read_started;
> +       int read_allocated;
>         struct list_head   
>     read_queue;
>         unsigned     
>           n_read;
>         struct
> tasklet_struct   push;
> 
>         struct list_head   
>     write_pool;
> +       int write_started;
> +       int write_allocated;
>         struct gs_buf   
>        port_write_buf;
>         wait_queue_head_t   
>    drain_wait;     /*
> wait while writes drain */
> 
> @@ -361,6 +365,9 @@
>                
> struct usb_request      *req;
>                
> int               
>      len;
> 
> +           
>    if (port->write_started >=
> QUEUE_SIZE)
> +               
>        break;
> +
>                 req
> = list_entry(pool->next, struct usb_request, list);
>                 len
> = gs_send_packet(port, req->buf, in->maxpacket);
>                 if
> (len == 0) {
> @@ -394,6 +401,8 @@
>                
>         break;
>                 }
> 
> +           
>    port->write_started++;
> +
>                 /*
> abort immediately after disconnect */
>                 if
> (!port->port_usb)
>                
>         break;
> @@ -415,7 +424,6 @@
>  {
>         struct list_head   
>     *pool = &port->read_pool;
>         struct usb_ep   
>        *out =
> port->port_usb->out;
> -       unsigned   
>             started = 0;
> 
>         while (!list_empty(pool)) {
>                
> struct usb_request      *req;
> @@ -427,6 +435,9 @@
>                 if
> (!tty)
>                
>         break;
> 
> +           
>    if (port->read_started >=
> QUEUE_SIZE)
> +               
>        break;
> +
>                 req
> = list_entry(pool->next, struct usb_request, list);
>                
> list_del(&req->list);
>                
> req->length = out->maxpacket;
> @@ -444,13 +455,13 @@
>                
>         list_add(&req->list,
> pool);
>                
>         break;
>                 }
> -           
>    started++;
> +           
>    port->read_started++;
> 
>                 /*
> abort immediately after disconnect */
>                 if
> (!port->port_usb)
>                
>         break;
>         }
> -       return started;
> +       return
> port->read_started;
>  }
> 
>  /*
> @@ -532,6 +543,7 @@
>                 }
>  recycle:
>                
> list_move(&req->list, &port->read_pool);
> +           
>    port->read_started--;
>         }
> 
>         /* Push from tty to ldisc; this
> is immediate with low_latency, and
> @@ -590,6 +602,7 @@
> 
>        
> spin_lock(&port->port_lock);
>         list_add(&req->list,
> &port->write_pool);
> +       port->write_started--;
> 
>         switch (req->status) {
>         default:
> @@ -611,7 +624,7 @@
>        
> spin_unlock(&port->port_lock);
>  }
> 
> -static void gs_free_requests(struct usb_ep *ep, struct
> list_head *head)
> +static void gs_free_requests(struct usb_ep *ep, struct
> list_head *head, int *allocated)
>  {
>         struct usb_request   
>   *req;
> 
> @@ -619,25 +632,30 @@
>                 req
> = list_entry(head->next, struct usb_request, list);
>                
> list_del(&req->list);
>                
> gs_free_req(ep, req);
> +           
>    if (allocated)
> +               
>        (*allocated)--;
>         }
>  }
> 
>  static int gs_alloc_requests(struct usb_ep *ep, struct
> list_head *head,
> -           
>    void (*fn)(struct usb_ep *, struct
> usb_request *))
> +           
>    void (*fn)(struct usb_ep *, struct
> usb_request *), int *allocated)
>  {
>         int       
>              i;
>         struct usb_request   
>   *req;
> +       int n = allocated ?
> QUEUE_SIZE - *allocated : QUEUE_SIZE;
> 
>         /* Pre-allocate up to
> QUEUE_SIZE transfers, but if we can't
>          * do quite that many
> this time, don't fail ... we just won't
>          * be as speedy as we
> might otherwise be.
>          */
> -       for (i = 0; i <
> QUEUE_SIZE; i++) {
> +       for (i = 0; i < n; i++)
> {
>                 req
> = gs_alloc_req(ep, ep->maxpacket, GFP_ATOMIC);
>                 if
> (!req)
>                
>         return list_empty(head) ?
> -ENOMEM : 0;
>                
> req->complete = fn;
>                
> list_add_tail(&req->list, head);
> +        if (allocated)
> +            (*allocated)++;
>         }
>         return 0;
>  }
> @@ -664,14 +682,14 @@
>          * configurations may
> use different endpoints with a given port;
>          * and high speed vs
> full speed changes packet sizes too.
>          */
> -       status =
> gs_alloc_requests(ep, head, gs_read_complete);
> +       status =
> gs_alloc_requests(ep, head, gs_read_complete,
> &port->read_allocated);
>         if (status)
>                
> return status;
> 
>         status =
> gs_alloc_requests(port->port_usb->in,
> &port->write_pool,
> -               
>        gs_write_complete);
> +               
>        gs_write_complete,
> &port->write_allocated);
>         if (status) {
> -           
>    gs_free_requests(ep, head);
> +           
>    gs_free_requests(ep, head,
> &port->read_allocated);
>                
> return status;
>         }
> 
> @@ -683,8 +701,8 @@
>         if (started) {
>                
> tty_wakeup(port->port_tty);
>         } else {
> -           
>    gs_free_requests(ep, head);
> -           
>    gs_free_requests(port->port_usb->in,
> &port->write_pool);
> +           
>    gs_free_requests(ep, head,
> &port->read_allocated);
> +           
>    gs_free_requests(port->port_usb->in,
> &port->write_pool, &port->write_allocated);
>                
> status = -EIO;
>         }
> 
> @@ -1323,8 +1341,11 @@
>        
> spin_lock_irqsave(&port->port_lock, flags);
>         if (port->open_count == 0
> && !port->openclose)
>                
> gs_buf_free(&port->port_write_buf);
> -   
>    gs_free_requests(gser->out,
> &port->read_pool);
> -   
>    gs_free_requests(gser->out,
> &port->read_queue);
> -   
>    gs_free_requests(gser->in,
> &port->write_pool);
> +   
>    gs_free_requests(gser->out,
> &port->read_pool, NULL);
> +   
>    gs_free_requests(gser->out,
> &port->read_queue, NULL);
> +   
>    gs_free_requests(gser->in,
> &port->write_pool, NULL);
> +
> +       port->read_allocated =
> port->read_started = port->write_allocated =
> port->write_started = 0;
> +
>        
> spin_unlock_irqrestore(&port->port_lock, flags);
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux