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
