Subject: usb/gadget/u_serial: Simplify output buffer implementation.
There's no need to store buf_size in a variable, given that it's a
compile-time cosntant. And since it's a pwoer of 2, use a different
indexing scheme that lets us fill the circular buffer rather than
needing to reserve one byte to disambiguate empty and full.
---
> Patches accepted. :)
Something like this? One question I have is whether it's worth removing
the locking from gs_write_room and gs_chars_in_buffer. I avoided it for
now, on the assumption that those aren't hot code paths, but it could
be done.
Does anyone have opinions on the matter?
(Indeed, you could have independent read and write locking, so readers
and writers could overlap, but that's a bigger design question that
I don't feel like researching right now.)
diff --git a/drivers/usb/gadget/u_serial.c b/drivers/usb/gadget/u_serial.c
index fc6e709..8f0f1f8 100644
--- a/drivers/usb/gadget/u_serial.c
+++ b/drivers/usb/gadget/u_serial.c
@@ -80,10 +80,8 @@
/* circular buffer */
struct gs_buf {
- unsigned buf_size;
- char *buf_buf;
- char *buf_get;
- char *buf_put;
+ char *buf_buf; /* WRITE_BUF_SIZE */
+ unsigned buf_get, buf_put; /* Offsets */
};
/*
@@ -144,15 +142,13 @@ static unsigned n_ports;
*
* Allocate a circular buffer and all associated memory.
*/
-static int gs_buf_alloc(struct gs_buf *gb, unsigned size)
+static int gs_buf_alloc(struct gs_buf *gb)
{
- gb->buf_buf = kmalloc(size, GFP_KERNEL);
+ gb->buf_buf = kmalloc(WRITE_BUF_SIZE, GFP_KERNEL);
if (gb->buf_buf == NULL)
return -ENOMEM;
- gb->buf_size = size;
- gb->buf_put = gb->buf_buf;
- gb->buf_get = gb->buf_buf;
+ gb->buf_put = gb->buf_get = 0;
return 0;
}
@@ -161,6 +157,8 @@ static int gs_buf_alloc(struct gs_buf *gb, unsigned size)
* gs_buf_free
*
* Free the buffer and all associated memory.
+ * Must be idempotent; we call this early if possible, and again on
+ * final disconnect.
*/
static void gs_buf_free(struct gs_buf *gb)
{
@@ -185,9 +183,9 @@ static void gs_buf_clear(struct gs_buf *gb)
* Return the number of bytes of data written into the circular
* buffer.
*/
-static unsigned gs_buf_data_avail(struct gs_buf *gb)
+static unsigned gs_buf_data_avail(struct gs_buf const *gb)
{
- return (gb->buf_size + gb->buf_put - gb->buf_get) % gb->buf_size;
+ return gb->buf_put - gb->buf_get;
}
/*
@@ -196,9 +194,9 @@ static unsigned gs_buf_data_avail(struct gs_buf *gb)
* Return the number of bytes of space available in the circular
* buffer.
*/
-static unsigned gs_buf_space_avail(struct gs_buf *gb)
+static unsigned gs_buf_space_avail(struct gs_buf const *gb)
{
- return (gb->buf_size + gb->buf_get - gb->buf_put - 1) % gb->buf_size;
+ return WRITE_BUF_SIZE - gs_buf_data_avail(gb);
}
/*
@@ -212,27 +210,26 @@ static unsigned gs_buf_space_avail(struct gs_buf *gb)
static unsigned
gs_buf_put(struct gs_buf *gb, const char *buf, unsigned count)
{
- unsigned len;
+ unsigned len = gs_buf_space_avail(gb);
+ unsigned put_pos = gb->buf_put % WRITE_BUF_SIZE;
- len = gs_buf_space_avail(gb);
if (count > len)
count = len;
- if (count == 0)
+ if (count == 0) /* This short-cut isn't actually necessary */
return 0;
- len = gb->buf_buf + gb->buf_size - gb->buf_put;
+ len = WRITE_BUF_SIZE - put_pos;
+
if (count > len) {
- memcpy(gb->buf_put, buf, len);
- memcpy(gb->buf_buf, buf+len, count - len);
- gb->buf_put = gb->buf_buf + count - len;
+ memcpy(gb->buf_buf + put_pos, buf, len);
+ len = count - len;
+ put_pos = 0;
} else {
- memcpy(gb->buf_put, buf, count);
- if (count < len)
- gb->buf_put += count;
- else /* count == len */
- gb->buf_put = gb->buf_buf;
+ len = count;
}
+ memcpy(gb->buf_buf + put_pos, buf, len);
+ gb->buf_put += count;
return count;
}
@@ -248,27 +245,25 @@ gs_buf_put(struct gs_buf *gb, const char *buf, unsigned count)
static unsigned
gs_buf_get(struct gs_buf *gb, char *buf, unsigned count)
{
- unsigned len;
+ unsigned len = gs_buf_data_avail(gb);
+ unsigned get_pos = gb->buf_get % WRITE_BUF_SIZE;
- len = gs_buf_data_avail(gb);
if (count > len)
count = len;
- if (count == 0)
+ if (count == 0) /* This short-cut isn't actually necessary */
return 0;
- len = gb->buf_buf + gb->buf_size - gb->buf_get;
+ len = WRITE_BUF_SIZE - get_pos;
if (count > len) {
- memcpy(buf, gb->buf_get, len);
- memcpy(buf+len, gb->buf_buf, count - len);
- gb->buf_get = gb->buf_buf + count - len;
+ memcpy(buf, gb->buf_buf + get_pos, len);
+ len = count - len;
+ get_pos = 0;
} else {
- memcpy(buf, gb->buf_get, count);
- if (count < len)
- gb->buf_get += count;
- else /* count == len */
- gb->buf_get = gb->buf_buf;
+ len = count;
}
+ memcpy(buf, gb->buf_buf + get_pos, len);
+ gb->buf_get += count;
return count;
}
@@ -325,14 +320,7 @@ void gs_free_req(struct usb_ep *ep, struct usb_request *req)
static unsigned
gs_send_packet(struct gs_port *port, char *packet, unsigned size)
{
- unsigned len;
-
- len = gs_buf_data_avail(&port->port_write_buf);
- if (len < size)
- size = len;
- if (size != 0)
- size = gs_buf_get(&port->port_write_buf, packet, size);
- return size;
+ return gs_buf_get(&port->port_write_buf, packet, size);
}
/*
@@ -760,7 +748,7 @@ static int gs_open(struct tty_struct *tty, struct file *file)
if (port->port_write_buf.buf_buf == NULL) {
spin_unlock_irq(&port->port_lock);
- status = gs_buf_alloc(&port->port_write_buf, WRITE_BUF_SIZE);
+ status = gs_buf_alloc(&port->port_write_buf);
spin_lock_irq(&port->port_lock);
if (status) {
@@ -891,8 +879,7 @@ static int gs_write(struct tty_struct *tty, const unsigned char *buf, int count)
port->port_num, tty, count);
spin_lock_irqsave(&port->port_lock, flags);
- if (count)
- count = gs_buf_put(&port->port_write_buf, buf, count);
+ count = gs_buf_put(&port->port_write_buf, buf, count);
/* treat count == 0 as flush_chars() */
if (port->port_usb)
status = gs_start_tx(port);
@@ -930,6 +917,10 @@ static void gs_flush_chars(struct tty_struct *tty)
spin_unlock_irqrestore(&port->port_lock, flags);
}
+/*
+ * This could be made lock-free, if gs_buf's buf_get and buf_put were
+ * made atomic. Is that worth doing?
+ */
static int gs_write_room(struct tty_struct *tty)
{
struct gs_port *port = tty->driver_data;
@@ -947,6 +938,7 @@ static int gs_write_room(struct tty_struct *tty)
return room;
}
+/* Likewise */
static int gs_chars_in_buffer(struct tty_struct *tty)
{
struct gs_port *port = tty->driver_data;
--
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
