[PATCH] ftdi_sio patch

["Vitja Makarov" <vitja.makarov@gmail.com>]

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 */
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/