[PATCH] ftdi_sio patch

[PATCH] ftdi_sio patch

[Vitja Makarov]

Hi!

I've found that ftdi_sio driver have problems it could ran out of memory,
or lead kernel panic. The problem is in ftdi_write function, it
allocates new urb structure each time write() is called.

The following patch fixes this problems. Code is mostly based on pl2303 code.
I moved buffer code into separate file serial-buf.h as it could be
userful for other drivers.

Signed-off-by: Vitja Makarov <vitja.makarov@gmail.com>

vitja.

diff -uprN orig/drivers/usb/serial/ftdi_sio.c new/drivers/usb/serial/ftdi_sio.c
--- orig/drivers/usb/serial/ftdi_sio.c	2006-03-28 01:05:57.000000000 +0400
+++ new/drivers/usb/serial/ftdi_sio.c	2006-05-28 23:58:50.000000000 +0400
@@ -260,6 +260,7 @@
 #include <linux/serial.h>
 #include "usb-serial.h"
 #include "ftdi_sio.h"
+#include "serial-buf.h"

 /*
  * Version Information
@@ -268,6 +269,8 @@
 #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>, Bill
Ryder <bryder@sgi.com>, Kuba Ober <kuba@mareimbrium.org>"
 #define DRIVER_DESC "USB FTDI Serial Converters Driver"

+#define MIN(a,b) ((a)>(b)?(b):(a))
+
 static int debug;
 static __u16 vendor = FTDI_VID;
 static __u16 product;
@@ -545,6 +548,11 @@ struct ftdi_private {

 	int force_baud;		/* if non-zero, force the baud rate to this value */
 	int force_rtscts;	/* if non-zero, force RTS-CTS to always be enabled */
+
+	spinlock_t lock;		   /* private lock */
+	
+	struct serial_buf *buf; /* write buffer */
+	int write_urb_in_use;   /* write urb in use indicator */
 };

 /* Used for TIOCMIWAIT */
@@ -562,6 +570,7 @@ static void ftdi_shutdown		(struct usb_s
 static int  ftdi_open			(struct usb_serial_port *port, struct file *filp);
 static void ftdi_close			(struct usb_serial_port *port, struct file *filp);
 static int  ftdi_write			(struct usb_serial_port *port, const
unsigned char *buf, int count);
+static void ftdi_send                   (struct usb_serial_port *port);
 static int  ftdi_write_room		(struct usb_serial_port *port);
 static int  ftdi_chars_in_buffer	(struct usb_serial_port *port);
 static void ftdi_write_bulk_callback	(struct urb *urb, struct pt_regs *regs);
@@ -1066,6 +1075,8 @@ static ssize_t store_event_char(struct d
 			     v, priv->interface,
 			     buf, 0, WDR_TIMEOUT);
 	
+	
+	
 	if (rv < 0) {
 		dbg("Unable to write event character: %i", rv);
 		return -EIO;
@@ -1138,6 +1149,7 @@ static int ftdi_sio_attach (struct usb_s
 	struct usb_serial_port *port = serial->port[0];
 	struct ftdi_private *priv;
 	struct ftdi_sio_quirk *quirk;
+	unsigned char *transfer_buffer;
 	
 	dbg("%s",__FUNCTION__);

@@ -1147,6 +1159,7 @@ static int ftdi_sio_attach (struct usb_s
 		return -ENOMEM;
 	}
 	memset(priv, 0, sizeof(*priv));
+	spin_lock_init(&priv->lock);

 	spin_lock_init(&priv->rx_lock);
         init_waitqueue_head(&priv->delta_msr_wait);
@@ -1167,14 +1180,22 @@ static int ftdi_sio_attach (struct usb_s
 	}

 	INIT_WORK(&priv->rx_work, ftdi_process_read, port);
+
+	/* Try to increase the size of write buffer */
+	transfer_buffer = (unsigned char *) kmalloc(SERIAL_BUF_SIZE, GFP_KERNEL);
+
+	if (transfer_buffer) {
+                port->write_urb->transfer_buffer = transfer_buffer;
+                port->write_urb->transfer_buffer_length = SERIAL_BUF_SIZE;
+	}

 	/* Free port's existing write urb and transfer buffer. */
-	if (port->write_urb) {
-		usb_free_urb (port->write_urb);
-		port->write_urb = NULL;
+	priv->buf = serial_buf_alloc(SERIAL_BUF_SIZE);
+	if (priv->buf == NULL) {
+		kfree(port->bulk_in_buffer);
+		kfree(priv);
+		return -ENOMEM;
 	}
-	kfree(port->bulk_out_buffer);
-	port->bulk_out_buffer = NULL;

 	usb_set_serial_port_data(serial->port[0], priv);

@@ -1246,6 +1267,7 @@ static void ftdi_shutdown (struct usb_se
 	 */

 	if (priv) {
+		serial_buf_free(priv->buf);
 		usb_set_serial_port_data(port, NULL);
 		kfree(priv);
 	}
@@ -1345,128 +1367,137 @@ static void ftdi_close (struct usb_seria
 	/* shutdown our bulk read */
 	if (port->read_urb)
 		usb_kill_urb(port->read_urb);
+
+	/* shutdown our bulk write */
+	if (port->write_urb)	
+	    usb_kill_urb(port->write_urb);
+
 } /* ftdi_close */

+static int ftdi_write(struct usb_serial_port *port, const unsigned
char *buf, int count)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	
+	dbg("%s - port %d, %d bytes", __FUNCTION__, port->number, count);

-
-/* The SIO requires the first byte to have:
- *  B0 1
- *  B1 0
- *  B2..7 length of message excluding byte 0
- *
- * The new devices do not require this byte
- */
-static int ftdi_write (struct usb_serial_port *port,
-			   const unsigned char *buf, int count)
-{ /* ftdi_write */
+	if (!count)
+		return count;
+	
+	spin_lock_irqsave(&priv->lock, flags);
+	count = serial_buf_put(priv->buf, buf, count);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	ftdi_send(port);
+
+	return count;
+} /* ftdi_write */
+
+
+static void ftdi_send(struct usb_serial_port *port)
+{
+	int count, result;
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
-	struct urb *urb;
-	unsigned char *buffer;
+	unsigned long flags;
 	int data_offset ;       /* will be 1 for the SIO and 0 otherwise */
-	int status;
-	int transfer_size;

-	dbg("%s port %d, %d bytes", __FUNCTION__, port->number, count);
+	dbg("%s - port %d", __FUNCTION__, port->number);
+
+	spin_lock_irqsave(&priv->lock, flags);

-	if (count == 0) {
-		dbg("write request of 0 bytes");
-		return 0;
+	if (priv->write_urb_in_use) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		return;
 	}
-	
+
 	data_offset = priv->write_offset;
         dbg("data_offset set to %d",data_offset);

-	/* Determine total transfer size */
-	transfer_size = count;
-	if (data_offset > 0) {
-		/* Original sio needs control bytes too... */
-		transfer_size += (data_offset *
-				((count + (PKTSZ - 1 - data_offset)) /
-				 (PKTSZ - data_offset)));
-	}

-	buffer = kmalloc (transfer_size, GFP_ATOMIC);
-	if (!buffer) {
-		err("%s ran out of kernel memory for urb ...", __FUNCTION__);
-		return -ENOMEM;
+        if (data_offset > 0) {
+                int len = port->bulk_out_size;
+		unsigned char *first_byte = port->write_urb->transfer_buffer;
+
+                count = 0;
+
+                while (serial_buf_data_avail(priv->buf) && len > 1) {
+                        int toget = MIN(len - data_offset, PKTSZ -
data_offset);
+                        int cnt;
+
+        	        cnt = serial_buf_get(priv->buf, first_byte + data_offset,
+		                                toget);
+                        *first_byte |= 1 | (cnt << 2);
+                        first_byte += cnt + data_offset;
+                        len -= cnt + data_offset;
+                        count += cnt + data_offset;
+                }
+        } else {
+	        count = serial_buf_get(priv->buf, port->write_urb->transfer_buffer,
+		                            port->bulk_out_size);
 	}

-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		err("%s - no more free urbs", __FUNCTION__);
-		kfree (buffer);
-		return -ENOMEM;
+	if (count == 0) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		return;
 	}

-	/* Copy data */
-	if (data_offset > 0) {
-		/* Original sio requires control byte at start of each packet. */
-		int user_pktsz = PKTSZ - data_offset;
-		int todo = count;
-		unsigned char *first_byte = buffer;
-		const unsigned char *current_position = buf;
-
-		while (todo > 0) {
-			if (user_pktsz > todo) {
-				user_pktsz = todo;
-			}
-			/* Write the control byte at the front of the packet*/
-			*first_byte = 1 | ((user_pktsz) << 2);
-			/* Copy data for packet */
-			memcpy (first_byte + data_offset,
-				current_position, user_pktsz);
-			first_byte += user_pktsz + data_offset;
-			current_position += user_pktsz;
-			todo -= user_pktsz;
-		}
-	} else {
-		/* No control byte required. */
-		/* Copy in the data to send */
-		memcpy (buffer, buf, count);
-	}
+	priv->write_urb_in_use = 1;

-	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, transfer_size, buffer);
+	spin_unlock_irqrestore(&priv->lock, flags);

-	/* fill the buffer and send it */
-	usb_fill_bulk_urb(urb, port->serial->dev,
-		      usb_sndbulkpipe(port->serial->dev, port->bulk_out_endpointAddress),
-		      buffer, transfer_size,
-		      ftdi_write_bulk_callback, port);
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count,
port->write_urb->transfer_buffer);

-	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status) {
-		err("%s - failed submitting write urb, error %d", __FUNCTION__, status);
-		count = status;
-		kfree (buffer);
+	port->write_urb->transfer_buffer_length = count;
+	port->write_urb->dev = port->serial->dev;
+	result = usb_submit_urb (port->write_urb, GFP_ATOMIC);
+	if (result) {
+		dev_err(&port->dev, "%s - failed submitting write urb, error %d\n",
__FUNCTION__, result);
+		priv->write_urb_in_use = 0;
+		// TODO: reschedule pl2303_send
 	}

-	/* we are done with this urb, so let the host driver
-	 * really free it when it is finished with it */
-	usb_free_urb (urb);
-
-	dbg("%s write returning: %d", __FUNCTION__, count);
-	return count;
-} /* ftdi_write */
-
+	schedule_work(&port->work);
+}

-/* This function may get called when the device is closed */

 static void ftdi_write_bulk_callback (struct urb *urb, struct pt_regs *regs)
 {
-	struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
-
-	/* free up the transfer buffer, as usb_free_urb() does not do this */
-	kfree (urb->transfer_buffer);
+	struct usb_serial_port *port = (struct usb_serial_port *) urb->context;
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	int result;

 	dbg("%s - port %d", __FUNCTION__, port->number);
-	
-	if (urb->status) {
-		dbg("nonzero write bulk status received: %d", urb->status);
+
+	switch (urb->status) {
+	case 0:
+		/* success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* this urb is terminated, clean up */
+		dbg("%s - urb shutting down with status: %d", __FUNCTION__, urb->status);
+		priv->write_urb_in_use = 0;
 		return;
+	default:
+		/* error in the urb, so we have to resubmit it */
+		dbg("%s - Overflow in write", __FUNCTION__);
+		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__,
urb->status);
+		port->write_urb->transfer_buffer_length = 1;
+		port->write_urb->dev = port->serial->dev;
+		result = usb_submit_urb (port->write_urb, GFP_ATOMIC);
+		if (result)
+			dev_err(&urb->dev->dev, "%s - failed resubmitting write urb, error
%d\n", __FUNCTION__, result);
+		else
+			return;
 	}

-	schedule_work(&port->work);
-} /* ftdi_write_bulk_callback */
+	priv->write_urb_in_use = 0;
+
+	/* send any buffered data */
+	ftdi_send(port);
+}
+


 static int ftdi_write_room( struct usb_serial_port *port )
diff -uprN orig/drivers/usb/serial/serial-buf.h
new/drivers/usb/serial/serial-buf.h
--- orig/drivers/usb/serial/serial-buf.h	1970-01-01 03:00:00.000000000 +0300
+++ new/drivers/usb/serial/serial-buf.h	2006-05-28 23:58:50.000000000 +0400
@@ -0,0 +1,211 @@
+#ifndef __LINUX_USB_SERIAL_BUF_H
+#define __LINUX_USB_SERIAL_BUF_H
+
+/*****************************************************************************
+ * Write buffer functions - buffering code from pl2303 used
+ *****************************************************************************/
+
+#ifndef SERIAL_BUF_SIZE
+# define SERIAL_BUF_SIZE		1024
+#endif
+
+struct serial_buf {
+	unsigned int	buf_size;
+	char		*buf_buf;
+	char		*buf_get;
+	char		*buf_put;
+};
+
+/* write buffer functions */
+static struct serial_buf *serial_buf_alloc(unsigned int size);
+static void 		  serial_buf_free(struct serial_buf *cb);
+static void		  serial_buf_clear(struct serial_buf *cb);
+static unsigned int	  serial_buf_data_avail(struct serial_buf *cb);
+static unsigned int	  serial_buf_space_avail(struct serial_buf *cb);
+static unsigned int	  serial_buf_put(struct serial_buf *cb, const
char *buf, unsigned int count);
+static unsigned int	  serial_buf_get(struct serial_buf *cb, char
*buf, unsigned int count);
+
+
+
+/*
+ * serial_buf_alloc
+ *
+ * Allocate a circular buffer and all associated memory.
+ */
+
+static inline struct serial_buf *serial_buf_alloc(unsigned int size)
+{
+
+	struct serial_buf *cb;
+
+
+	if (size == 0)
+		return NULL;
+
+	cb = (struct serial_buf *)kmalloc(sizeof(struct serial_buf), GFP_KERNEL);
+	if (cb == NULL)
+		return NULL;
+
+	cb->buf_buf = kmalloc(size, GFP_KERNEL);
+	if (cb->buf_buf == NULL) {
+		kfree(cb);
+		return NULL;
+	}
+
+	cb->buf_size = size;
+	cb->buf_get = cb->buf_put = cb->buf_buf;
+
+	return cb;
+
+}
+
+
+/*
+ * serial_buf_free
+ *
+ * Free the buffer and all associated memory.
+ */
+
+static inline void serial_buf_free(struct serial_buf *cb)
+{
+	if (cb) {
+		kfree(cb->buf_buf);
+		kfree(cb);
+	}
+}
+
+
+/*
+ * serial_buf_clear
+ *
+ * Clear out all data in the circular buffer.
+ */
+
+static inline void serial_buf_clear(struct serial_buf *cb)
+{
+	if (cb != NULL)
+		cb->buf_get = cb->buf_put;
+		/* equivalent to a get of all data available */
+}
+
+
+/*
+ * serial_buf_data_avail
+ *
+ * Return the number of bytes of data available in the circular
+ * buffer.
+ */
+
+static inline unsigned int serial_buf_data_avail(struct serial_buf *cb)
+{
+	if (cb != NULL)
+		return ((cb->buf_size + cb->buf_put - cb->buf_get) % cb->buf_size);
+	else
+		return 0;
+}
+
+
+/*
+ * serial_buf_space_avail
+ *
+ * Return the number of bytes of space available in the circular
+ * buffer.
+ */
+
+static inline unsigned int serial_buf_space_avail(struct serial_buf *cb)
+{
+	if (cb != NULL)
+		return ((cb->buf_size + cb->buf_get - cb->buf_put - 1) % cb->buf_size);
+	else
+		return 0;
+}
+
+
+/*
+ * serial_buf_put
+ *
+ * Copy data data from a user buffer and put it into the circular buffer.
+ * Restrict to the amount of space available.
+ *
+ * Return the number of bytes copied.
+ */
+
+static inline unsigned int serial_buf_put(struct serial_buf *cb,
const char *buf,
+	unsigned int count)
+{
+
+	unsigned int len;
+
+
+	if (cb == NULL)
+		return 0;
+
+	len  = serial_buf_space_avail(cb);
+	if (count > len)
+		count = len;
+
+	if (count == 0)
+		return 0;
+
+	len = cb->buf_buf + cb->buf_size - cb->buf_put;
+	if (count > len) {
+		memcpy(cb->buf_put, buf, len);
+		memcpy(cb->buf_buf, buf+len, count - len);
+		cb->buf_put = cb->buf_buf + count - len;
+	} else {
+		memcpy(cb->buf_put, buf, count);
+		if (count < len)
+			cb->buf_put += count;
+		else /* count == len */
+			cb->buf_put = cb->buf_buf;
+	}
+
+	return count;
+
+}
+
+
+/*
+ * serial_buf_get
+ *
+ * Get data from the circular buffer and copy to the given buffer.
+ * Restrict to the amount of data available.
+ *
+ * Return the number of bytes copied.
+ */
+
+static inline unsigned int serial_buf_get(struct serial_buf *cb, char *buf,
+	unsigned int count)
+{
+
+	unsigned int len;
+
+
+	if (cb == NULL)
+		return 0;
+
+	len = serial_buf_data_avail(cb);
+	if (count > len)
+		count = len;
+
+	if (count == 0)
+		return 0;
+
+	len = cb->buf_buf + cb->buf_size - cb->buf_get;
+	if (count > len) {
+		memcpy(buf, cb->buf_get, len);
+		memcpy(buf+len, cb->buf_buf, count - len);
+		cb->buf_get = cb->buf_buf + count - len;
+	} else {
+		memcpy(buf, cb->buf_get, count);
+		if (count < len)
+			cb->buf_get += count;
+		else /* count == len */
+			cb->buf_get = cb->buf_buf;
+	}
+
+	return count;
+
+}
+
+#endif /* ifdef __LINUX_USB_SERIAL_BUF_H */

Re: [PATCH] ftdi_sio patch

[Greg KH]

On Sun, Jun 04, 2006 at 11:48:54PM +0400, Vitja Makarov wrote:
> Hi!
> 
> I've found that ftdi_sio driver have problems it could ran out of memory,
> or lead kernel panic. The problem is in ftdi_write function, it
> allocates new urb structure each time write() is called.
> 
> The following patch fixes this problems. Code is mostly based on pl2303 
> code.
> I moved buffer code into separate file serial-buf.h as it could be
> userful for other drivers.
> 
> Signed-off-by: Vitja Makarov <vitja.makarov@gmail.com>
> 
> vitja.
> 
> diff -uprN orig/drivers/usb/serial/ftdi_sio.c 
> new/drivers/usb/serial/ftdi_sio.c
> --- orig/drivers/usb/serial/ftdi_sio.c	2006-03-28 01:05:57.000000000 +0400
> +++ new/drivers/usb/serial/ftdi_sio.c	2006-05-28 23:58:50.000000000 +0400
> @@ -260,6 +260,7 @@
> #include <linux/serial.h>
> #include "usb-serial.h"
> #include "ftdi_sio.h"
> +#include "serial-buf.h"
> 
> /*
>  * Version Information
> @@ -268,6 +269,8 @@
> #define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>, Bill
> Ryder <bryder@sgi.com>, Kuba Ober <kuba@mareimbrium.org>"
> #define DRIVER_DESC "USB FTDI Serial Converters Driver"
> 
> +#define MIN(a,b) ((a)>(b)?(b):(a))

Please us the built in kernel min() function.  This one is wrong...

> static int debug;
> static __u16 vendor = FTDI_VID;
> static __u16 product;
> @@ -545,6 +548,11 @@ struct ftdi_private {
> 
> 	int force_baud;		/* if non-zero, force the baud rate to this 
> 	value */
> 	int force_rtscts;	/* if non-zero, force RTS-CTS to always be 
> 	enabled */
> +
> +	spinlock_t lock;		   /* private lock */
> +	
> +	struct serial_buf *buf; /* write buffer */
> +	int write_urb_in_use;   /* write urb in use indicator */
> };
> 
> /* Used for TIOCMIWAIT */
> @@ -562,6 +570,7 @@ static void ftdi_shutdown		(struct usb_s
> static int  ftdi_open			(struct usb_serial_port *port, 
> struct file *filp);
> static void ftdi_close			(struct usb_serial_port *port, 
> struct file *filp);
> static int  ftdi_write			(struct usb_serial_port *port, const
> unsigned char *buf, int count);
> +static void ftdi_send                   (struct usb_serial_port *port);
> static int  ftdi_write_room		(struct usb_serial_port *port);
> static int  ftdi_chars_in_buffer	(struct usb_serial_port *port);
> static void ftdi_write_bulk_callback	(struct urb *urb, struct pt_regs 
> *regs);
> @@ -1066,6 +1075,8 @@ static ssize_t store_event_char(struct d
> 			     v, priv->interface,
> 			     buf, 0, WDR_TIMEOUT);
> 	
> +	
> +	

Why add lines of whitespace?  Especially ones with trailing spaces :(

> 	if (rv < 0) {
> 		dbg("Unable to write event character: %i", rv);
> 		return -EIO;
> @@ -1138,6 +1149,7 @@ static int ftdi_sio_attach (struct usb_s
> 	struct usb_serial_port *port = serial->port[0];
> 	struct ftdi_private *priv;
> 	struct ftdi_sio_quirk *quirk;
> +	unsigned char *transfer_buffer;
> 	
> 	dbg("%s",__FUNCTION__);
> 
> @@ -1147,6 +1159,7 @@ static int ftdi_sio_attach (struct usb_s
> 		return -ENOMEM;
> 	}
> 	memset(priv, 0, sizeof(*priv));
> +	spin_lock_init(&priv->lock);
> 
> 	spin_lock_init(&priv->rx_lock);
>         init_waitqueue_head(&priv->delta_msr_wait);
> @@ -1167,14 +1180,22 @@ static int ftdi_sio_attach (struct usb_s
> 	}
> 
> 	INIT_WORK(&priv->rx_work, ftdi_process_read, port);
> +
> +	/* Try to increase the size of write buffer */
> +	transfer_buffer = (unsigned char *) kmalloc(SERIAL_BUF_SIZE, 
> GFP_KERNEL);

cast is not needed.

> +
> +	if (transfer_buffer) {
> +                port->write_urb->transfer_buffer = transfer_buffer;
> +                port->write_urb->transfer_buffer_length = SERIAL_BUF_SIZE;
> +	}
> 
> 	/* Free port's existing write urb and transfer buffer. */
> -	if (port->write_urb) {
> -		usb_free_urb (port->write_urb);
> -		port->write_urb = NULL;
> +	priv->buf = serial_buf_alloc(SERIAL_BUF_SIZE);
> +	if (priv->buf == NULL) {
> +		kfree(port->bulk_in_buffer);
> +		kfree(priv);
> +		return -ENOMEM;
> 	}
> -	kfree(port->bulk_out_buffer);
> -	port->bulk_out_buffer = NULL;
> 
> 	usb_set_serial_port_data(serial->port[0], priv);
> 
> @@ -1246,6 +1267,7 @@ static void ftdi_shutdown (struct usb_se
> 	 */
> 
> 	if (priv) {
> +		serial_buf_free(priv->buf);
> 		usb_set_serial_port_data(port, NULL);
> 		kfree(priv);
> 	}
> @@ -1345,128 +1367,137 @@ static void ftdi_close (struct usb_seria
> 	/* shutdown our bulk read */
> 	if (port->read_urb)
> 		usb_kill_urb(port->read_urb);
> +
> +	/* shutdown our bulk write */
> +	if (port->write_urb)	
> +	    usb_kill_urb(port->write_urb);

Trailing white space and improper indentation :(

> +
> } /* ftdi_close */
> 
> +static int ftdi_write(struct usb_serial_port *port, const unsigned
> char *buf, int count)

Patch is linewrapped :(

> +{
> +	struct ftdi_private *priv = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +	

Trailing whitespace added :(

> +struct serial_buf {
> +	unsigned int	buf_size;
> +	char		*buf_buf;
> +	char		*buf_get;
> +	char		*buf_put;
> +};


We already have a circular buffer structure in the kernel.  Please use
that one instead of creating your own again.

thanks,

greg k-h

Re: [PATCH] ftdi_sio patch

[Vitja Makarov]

Sorry for my bad indentation and linewraps, I used circular buffer 
implementation from ti_usb_3410_5052 that uses kerenl's circ_buf structure.

Signed-off-by: Vitja Makarov <vitja.makarov@gmail.com>

vitja.

diff -uprN orig/drivers/usb/serial/ftdi_sio.c new/drivers/usb/serial/ftdi_sio.c
--- orig/drivers/usb/serial/ftdi_sio.c	2006-03-28 01:05:57.000000000 +0400
+++ new/drivers/usb/serial/ftdi_sio.c	2006-06-07 10:48:26.000000000 +0400
@@ -258,6 +258,7 @@
 #include <asm/uaccess.h>
 #include <linux/usb.h>
 #include <linux/serial.h>
+#include <linux/circ_buf.h>
 #include "usb-serial.h"
 #include "ftdi_sio.h"
 
@@ -517,6 +518,8 @@ static const char *ftdi_chip_name[] = {
 /* Constants for read urb and write urb */
 #define BUFSZ 512
 #define PKTSZ 64
+/* size of write buffer */
+#define SERIAL_BUF_SIZE		1024
 
 /* rx_flags */
 #define THROTTLED		0x01
@@ -545,6 +548,11 @@ struct ftdi_private {
 
 	int force_baud;		/* if non-zero, force the baud rate to this value */
 	int force_rtscts;	/* if non-zero, force RTS-CTS to always be enabled */
+
+	spinlock_t lock;		   /* private lock */
+	
+	struct circ_buf *buf; /* write buffer */
+	int write_urb_in_use;   /* write urb in use indicator */
 };
 
 /* Used for TIOCMIWAIT */
@@ -562,6 +570,7 @@ static void ftdi_shutdown		(struct usb_s
 static int  ftdi_open			(struct usb_serial_port *port, struct file *filp);
 static void ftdi_close			(struct usb_serial_port *port, struct file *filp);
 static int  ftdi_write			(struct usb_serial_port *port, const unsigned char *buf, int count);
+static void ftdi_send                   (struct usb_serial_port *port);
 static int  ftdi_write_room		(struct usb_serial_port *port);
 static int  ftdi_chars_in_buffer	(struct usb_serial_port *port);
 static void ftdi_write_bulk_callback	(struct urb *urb, struct pt_regs *regs);
@@ -575,6 +584,14 @@ static void ftdi_break_ctl		(struct usb_
 static void ftdi_throttle		(struct usb_serial_port *port);
 static void ftdi_unthrottle		(struct usb_serial_port *port);
 
+/* circular buffer */
+static struct circ_buf *serial_buf_alloc(void);
+static void serial_buf_free(struct circ_buf *cb);
+static void serial_buf_clear(struct circ_buf *cb);
+static int serial_buf_data_avail(struct circ_buf *cb);
+static int serial_buf_put(struct circ_buf *cb, const char *buf, int count);
+static int serial_buf_get(struct circ_buf *cb, char *buf, int count);
+
 static unsigned short int ftdi_232am_baud_base_to_divisor (int baud, int base);
 static unsigned short int ftdi_232am_baud_to_divisor (int baud);
 static __u32 ftdi_232bm_baud_base_to_divisor (int baud, int base);
@@ -1138,6 +1155,7 @@ static int ftdi_sio_attach (struct usb_s
 	struct usb_serial_port *port = serial->port[0];
 	struct ftdi_private *priv;
 	struct ftdi_sio_quirk *quirk;
+	char *transfer_buffer;
 	
 	dbg("%s",__FUNCTION__);
 
@@ -1147,6 +1165,7 @@ static int ftdi_sio_attach (struct usb_s
 		return -ENOMEM;
 	}
 	memset(priv, 0, sizeof(*priv));
+	spin_lock_init(&priv->lock);
 
 	spin_lock_init(&priv->rx_lock);
         init_waitqueue_head(&priv->delta_msr_wait);
@@ -1167,14 +1186,22 @@ static int ftdi_sio_attach (struct usb_s
 	}
 
 	INIT_WORK(&priv->rx_work, ftdi_process_read, port);
+    
+	/* Try to increase the size of write buffer */
+	transfer_buffer = kmalloc(SERIAL_BUF_SIZE, GFP_KERNEL);
+
+	if (transfer_buffer) {
+                port->write_urb->transfer_buffer = transfer_buffer;
+                port->write_urb->transfer_buffer_length = SERIAL_BUF_SIZE;
+	} 
 
 	/* Free port's existing write urb and transfer buffer. */
-	if (port->write_urb) {
-		usb_free_urb (port->write_urb);
-		port->write_urb = NULL;
+	priv->buf = serial_buf_alloc();
+	if (priv->buf == NULL) {
+		kfree(port->bulk_in_buffer);
+		kfree(priv);
+		return -ENOMEM;
 	}
-	kfree(port->bulk_out_buffer);
-	port->bulk_out_buffer = NULL;
 
 	usb_set_serial_port_data(serial->port[0], priv);
 
@@ -1246,6 +1273,7 @@ static void ftdi_shutdown (struct usb_se
 	 */
 
 	if (priv) {
+		serial_buf_free(priv->buf);
 		usb_set_serial_port_data(port, NULL);
 		kfree(priv);
 	}
@@ -1258,7 +1286,7 @@ static int  ftdi_open (struct usb_serial
 	struct usb_device *dev = port->serial->dev;
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 	unsigned long flags;
-	
+
 	int result = 0;
 	char buf[1]; /* Needed for the usb_control_msg I think */
 
@@ -1341,132 +1369,139 @@ static void ftdi_close (struct usb_seria
 	/* cancel any scheduled reading */
 	cancel_delayed_work(&priv->rx_work);
 	flush_scheduled_work();
-	
+
 	/* shutdown our bulk read */
 	if (port->read_urb)
 		usb_kill_urb(port->read_urb);
+
+	/* shutdown our bulk write */
+	if (port->write_urb)
+	        usb_kill_urb(port->write_urb);
 } /* ftdi_close */
 
+static int ftdi_write(struct usb_serial_port *port, const unsigned char *buf, int count)
+{
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	unsigned long flags;
+	
+	dbg("%s - port %d, %d bytes", __FUNCTION__, port->number, count);
 
-  
-/* The SIO requires the first byte to have:
- *  B0 1
- *  B1 0
- *  B2..7 length of message excluding byte 0
- *
- * The new devices do not require this byte
- */
-static int ftdi_write (struct usb_serial_port *port,
-			   const unsigned char *buf, int count)
-{ /* ftdi_write */
+	if (!count)
+		return count;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	count = serial_buf_put(priv->buf, buf, count);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	ftdi_send(port);
+
+	return count;
+} /* ftdi_write */
+
+
+static void ftdi_send(struct usb_serial_port *port)
+{
+	int count, result;
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
-	struct urb *urb;
-	unsigned char *buffer;
+	unsigned long flags;
 	int data_offset ;       /* will be 1 for the SIO and 0 otherwise */
-	int status;
-	int transfer_size;
 
-	dbg("%s port %d, %d bytes", __FUNCTION__, port->number, count);
+	dbg("%s - port %d", __FUNCTION__, port->number);
 
-	if (count == 0) {
-		dbg("write request of 0 bytes");
-		return 0;
+	spin_lock_irqsave(&priv->lock, flags);
+
+	if (priv->write_urb_in_use) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		return;
 	}
-	
+
 	data_offset = priv->write_offset;
         dbg("data_offset set to %d",data_offset);
 
-	/* Determine total transfer size */
-	transfer_size = count;
-	if (data_offset > 0) {
-		/* Original sio needs control bytes too... */
-		transfer_size += (data_offset *
-				((count + (PKTSZ - 1 - data_offset)) /
-				 (PKTSZ - data_offset)));
-	}
-
-	buffer = kmalloc (transfer_size, GFP_ATOMIC);
-	if (!buffer) {
-		err("%s ran out of kernel memory for urb ...", __FUNCTION__);
-		return -ENOMEM;
-	}
 
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb) {
-		err("%s - no more free urbs", __FUNCTION__);
-		kfree (buffer);
-		return -ENOMEM;
+        if (data_offset > 0) {
+                int len = port->bulk_out_size;
+		unsigned char *first_byte = port->write_urb->transfer_buffer;
+
+                count = 0;
+
+                while (serial_buf_data_avail(priv->buf) && len > 1) {
+                        int toget = min(len - data_offset, PKTSZ - data_offset);
+                        int cnt;
+
+        	        cnt = serial_buf_get(priv->buf, first_byte + data_offset,
+		                                toget);
+                        *first_byte |= 1 | (cnt << 2);
+                        first_byte += cnt + data_offset;
+                        len -= cnt + data_offset;
+                        count += cnt + data_offset;
+                }
+        } else {
+	        count = serial_buf_get(priv->buf, port->write_urb->transfer_buffer,
+		                            port->bulk_out_size);
 	}
 
-	/* Copy data */
-	if (data_offset > 0) {
-		/* Original sio requires control byte at start of each packet. */
-		int user_pktsz = PKTSZ - data_offset;
-		int todo = count;
-		unsigned char *first_byte = buffer;
-		const unsigned char *current_position = buf;
-
-		while (todo > 0) {
-			if (user_pktsz > todo) {
-				user_pktsz = todo;
-			}
-			/* Write the control byte at the front of the packet*/
-			*first_byte = 1 | ((user_pktsz) << 2); 
-			/* Copy data for packet */
-			memcpy (first_byte + data_offset,
-				current_position, user_pktsz);
-			first_byte += user_pktsz + data_offset;
-			current_position += user_pktsz;
-			todo -= user_pktsz;
-		}
-	} else {
-		/* No control byte required. */
-		/* Copy in the data to send */
-		memcpy (buffer, buf, count);
+	if (count == 0) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		return;
 	}
 
-	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, transfer_size, buffer);
+	priv->write_urb_in_use = 1;
+	spin_unlock_irqrestore(&priv->lock, flags);
 
-	/* fill the buffer and send it */
-	usb_fill_bulk_urb(urb, port->serial->dev, 
-		      usb_sndbulkpipe(port->serial->dev, port->bulk_out_endpointAddress),
-		      buffer, transfer_size,
-		      ftdi_write_bulk_callback, port);
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, count, port->write_urb->transfer_buffer);
 
-	status = usb_submit_urb(urb, GFP_ATOMIC);
-	if (status) {
-		err("%s - failed submitting write urb, error %d", __FUNCTION__, status);
-		count = status;
-		kfree (buffer);
+	port->write_urb->transfer_buffer_length = count;
+	port->write_urb->dev = port->serial->dev;
+	result = usb_submit_urb (port->write_urb, GFP_ATOMIC);
+	if (result) {
+		dev_err(&port->dev, "%s - failed submitting write urb, error %d\n", __FUNCTION__, result);
+		priv->write_urb_in_use = 0;
+		// TODO: reschedule pl2303_send
 	}
 
-	/* we are done with this urb, so let the host driver
-	 * really free it when it is finished with it */
-	usb_free_urb (urb);
-
-	dbg("%s write returning: %d", __FUNCTION__, count);
-	return count;
-} /* ftdi_write */
-
+	schedule_work(&port->work);
+}
 
-/* This function may get called when the device is closed */
 
 static void ftdi_write_bulk_callback (struct urb *urb, struct pt_regs *regs)
 {
-	struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
-
-	/* free up the transfer buffer, as usb_free_urb() does not do this */
-	kfree (urb->transfer_buffer);
+	struct usb_serial_port *port = (struct usb_serial_port *) urb->context;
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+	int result;
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
-	
-	if (urb->status) {
-		dbg("nonzero write bulk status received: %d", urb->status);
+
+	switch (urb->status) {
+	case 0:
+		/* success */
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* this urb is terminated, clean up */
+		dbg("%s - urb shutting down with status: %d", __FUNCTION__, urb->status);
+		priv->write_urb_in_use = 0;
 		return;
+	default:
+		/* error in the urb, so we have to resubmit it */
+		dbg("%s - Overflow in write", __FUNCTION__);
+		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
+		port->write_urb->transfer_buffer_length = 1;
+		port->write_urb->dev = port->serial->dev;
+		result = usb_submit_urb (port->write_urb, GFP_ATOMIC);
+		if (result)
+			dev_err(&urb->dev->dev, "%s - failed resubmitting write urb, error %d\n", __FUNCTION__, result);
+		else
+			return;
 	}
 
-	schedule_work(&port->work);
-} /* ftdi_write_bulk_callback */
+	priv->write_urb_in_use = 0;
+
+	/* send any buffered data */
+	ftdi_send(port);
+}
+
 
 
 static int ftdi_write_room( struct usb_serial_port *port )
@@ -2078,6 +2113,129 @@ static void ftdi_unthrottle (struct usb_
 		schedule_work(&priv->rx_work);
 }
 
+/* Circular Buffer Functions, code from ti_usb_3410_5052 used */
+
+/*
+ * serial_buf_alloc
+ *
+ * Allocate a circular buffer and all associated memory.
+ */
+
+static struct circ_buf *serial_buf_alloc(void)
+{
+	struct circ_buf *cb;
+
+	cb = (struct circ_buf *)kmalloc(sizeof(struct circ_buf), GFP_KERNEL);
+	if (cb == NULL)
+		return NULL;
+
+	cb->buf = kmalloc(SERIAL_BUF_SIZE, GFP_KERNEL);
+	if (cb->buf == NULL) {
+		kfree(cb);
+		return NULL;
+	}
+
+	serial_buf_clear(cb);
+
+	return cb;
+}
+
+
+/*
+ * serial_buf_free
+ *
+ * Free the buffer and all associated memory.
+ */
+
+static void serial_buf_free(struct circ_buf *cb)
+{
+	kfree(cb->buf);
+	kfree(cb);
+}
+
+
+/*
+ * serial_buf_clear
+ *
+ * Clear out all data in the circular buffer.
+ */
+
+static void serial_buf_clear(struct circ_buf *cb)
+{
+	cb->head = cb->tail = 0;
+}
+
+
+/*
+ * serial_buf_data_avail
+ *
+ * Return the number of bytes of data available in the circular
+ * buffer.
+ */
+
+static int serial_buf_data_avail(struct circ_buf *cb)
+{
+	return CIRC_CNT(cb->head,cb->tail,SERIAL_BUF_SIZE);
+}
+
+/*
+ * serial_buf_put
+ *
+ * Copy data data from a user buffer and put it into the circular buffer.
+ * Restrict to the amount of space available.
+ *
+ * Return the number of bytes copied.
+ */
+
+static int serial_buf_put(struct circ_buf *cb, const char *buf, int count)
+{
+	int c, ret = 0;
+
+	while (1) {
+		c = CIRC_SPACE_TO_END(cb->head, cb->tail, SERIAL_BUF_SIZE);
+		if (count < c)
+			c = count;
+		if (c <= 0)
+			break;
+		memcpy(cb->buf + cb->head, buf, c);
+		cb->head = (cb->head + c) & (SERIAL_BUF_SIZE-1);
+		buf += c;
+		count -= c;
+		ret += c;
+	}
+
+	return ret;
+}
+
+/*
+ * serial_buf_get
+ *
+ * Get data from the circular buffer and copy to the given buffer.
+ * Restrict to the amount of data available.
+ *
+ * Return the number of bytes copied.
+ */
+
+static int serial_buf_get(struct circ_buf *cb, char *buf, int count)
+{
+	int c, ret = 0;
+
+	while (1) {
+		c = CIRC_CNT_TO_END(cb->head, cb->tail, SERIAL_BUF_SIZE);
+		if (count < c)
+			c = count;
+		if (c <= 0)
+			break;
+		memcpy(buf, cb->buf + cb->tail, c);
+		cb->tail = (cb->tail + c) & (SERIAL_BUF_SIZE-1);
+		buf += c;
+		count -= c;
+		ret += c;
+	}
+
+	return ret;
+}
+
 static int __init ftdi_init (void)
 {
 	int retval;