Patch for 2.4 USB serial

[Pete Zaitcev]

I got around to fixing a number of teething problems with the usbserial
helper process and the usbserial in general, just in time when 2.4 started
to wind down. Nonetheless, here it goes, against 2.4.22-1.2176.

Please consider for a future update, if that ever happens.

Related bug reports:

 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=112806
  Not sure if I fixed it completely this time, but I hope so.

 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=114614

 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=116011
  Regression in Fedora which I caused with fixes for PDA syncs

 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=117423

 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=118436
  RHEL bug, but fixes are all mixed together in the code.

If everything goes well, I will push this to Marcelo for 2.4.27.
I held back hoping for a backport, but now that I looked even more
at it, I think 2.6 gets it wrong, too. It will be a separate work.
So let's just fix this.

Code reviews are always appreciated.

-- Pete

diff -urN -X dontdiff linux-2.4.22-1.2176/drivers/usb/serial/usbserial.c
linux-2.4.22-1.2176-u1/drivers/usb/serial/usbserial.c
--- linux-2.4.22-1.2176/drivers/usb/serial/usbserial.c	2004-03-11 20:53:43.000000000
-0800
+++ linux-2.4.22-1.2176-u1/drivers/usb/serial/usbserial.c	2004-03-23 11:07:12.000000000
-0800
@@ -367,6 +367,10 @@
 static void serial_close (struct tty_struct *tty, struct file * filp);
 static int __serial_write (struct usb_serial_port *port, int from_user, const unsigned
char *buf, int count);
 static int  serial_write (struct tty_struct * tty, int from_user, const unsigned char
*buf, int count);
+static int  serial_post_job(struct usb_serial_port *port, int from_user,
+    int gfp, const unsigned char *buf, int count);
+static int  serial_post_one(struct usb_serial_port *port, int from_user,
+    int gfp, const unsigned char *buf, int count);
 static int  serial_write_room (struct tty_struct *tty);
 static int  serial_chars_in_buffer (struct tty_struct *tty);
 static void serial_throttle (struct tty_struct * tty);
@@ -477,35 +481,45 @@
  * Rather than open the whole can of worms again, we just post writes
  * into a helper which can sleep.
  *
- * Kernel 2.6 has a proper fix, reportedly.
- *
- * XXX Nothing prevents this from looping forever.
+ * Kernel 2.6 has a proper fix. It replaces semaphores with proper locking.
  */
 static void post_helper(void *arg)
 {
+	struct list_head *pos;
 	struct usb_serial_post_job *job;
 	struct usb_serial_port *port;
 	struct usb_serial *serial;
 	unsigned int flags;
 
 	spin_lock_irqsave(&post_lock, flags);
-	while (!list_empty(&post_list)) {
-		job = list_entry(post_list.next, struct usb_serial_post_job, link);
+	pos = post_list.next;
+	while (pos != &post_list) {
+		job = list_entry(pos, struct usb_serial_post_job, link);
+		port = job->port;
+		/* get_usb_serial checks port->tty, so cannot be used */
+		serial = port->serial;
+		if (port->write_busy) {
+			dbg("%s - port %d busy", __FUNCTION__, port->number);
+			pos = pos->next;
+			continue;
+		}
 		list_del(&job->link);
 		spin_unlock_irqrestore(&post_lock, flags);
 
-		port = job->port;
-		serial = get_usb_serial (port, __FUNCTION__);
-
 		down(&port->sem);
+		dbg("%s - port %d len %d backlog %d", __FUNCTION__,
+		    port->number, job->len, port->write_backlog);
 		if (port->tty != NULL)
 			__serial_write(port, 0, job->buff, job->len);
 		up(&port->sem);
 
-		kfree(job);
 		spin_lock_irqsave(&post_lock, flags);
+		port->write_backlog -= job->len;
+		kfree(job);
 		if (--serial->ref == 0)
 			kfree(serial);
+		/* Have to reset because we dropped spinlock */
+		pos = post_list.next;
 	}
 	spin_unlock_irqrestore(&post_lock, flags);
 }
@@ -642,7 +656,20 @@
 
 	/* if disconnect beat us to the punch here, there's nothing to do */
 	if (tty->driver_data) {
-		/* post_helper(NULL); */ /* Correct, but unimportant for echo.*/
+		/*
+		 * XXX The right thing would be to wait for the output to drain.
+		 * But we are not sufficiently daring to experiment in 2.4.
+		 * N.B. If we do wait, no need to run post_helper here.
+		 * Normall callback mechanism wakes it up just fine.
+		 */
+#if I_AM_A_DARING_HACKER
+		tty->closing = 1;
+		up (&port->sem);
+		if (info->closing_wait != ASYNC_CLOSING_WAIT_NONE)
+			tty_wait_until_sent(tty, info->closing_wait);
+		down (&port->sem);
+		if (!tty->driver_data) /* woopsie, disconnect, now what */ ;
+#endif
 		__serial_close(port, filp);
 	}
 
@@ -677,9 +704,6 @@
 static int serial_write (struct tty_struct * tty, int from_user, const unsigned char
*buf, int count)
 {
 	struct usb_serial_port *port = (struct usb_serial_port *) tty->driver_data;
-	struct usb_serial *serial = get_usb_serial (port, __FUNCTION__);
-	struct usb_serial_post_job *job;
-	unsigned long flags;
 	int rc;
 
 	if (!in_interrupt()) {
@@ -690,6 +714,18 @@
 		post_helper(NULL);
 
 		down(&port->sem);
+		/*
+		 * This happens when a line discipline asks how much room
+		 * we have, gets 64, then tries to perform two writes
+		 * for a byte each. First write takes whole URB, second
+		 * write hits this check.
+		 */
+		if (port->write_busy) {
+			up(&port->sem);
+			return serial_post_job(port, from_user, GFP_KERNEL,
+			    buf, count);
+		}
+
 		rc = __serial_write(port, from_user, buf, count);
 		up(&port->sem);
 		return rc;
@@ -705,12 +741,19 @@
 		return -EINVAL;
 	}
 
-	job = kmalloc(sizeof(struct usb_serial_post_job), GFP_ATOMIC);
-	if (job == NULL)
-		return -ENOMEM;
+	return serial_post_job(port, 0, GFP_ATOMIC, buf, count);
+}
 
-	job->port = port;
-	if ((job->len = count) >= POST_BSIZE) {
+static int serial_post_job(struct usb_serial_port *port, int from_user,
+    int gfp, const unsigned char *buf, int count)
+{
+	int done = 0, length;
+	int rc;
+
+	if (port == NULL)
+		return -EPIPE;
+
+	if (count >= 512) {
 		static int rate = 0;
 		/*
 		 * Data loss due to extreme circumstances.
@@ -718,13 +761,61 @@
 		 * Neener, neener! Actually, it's probably an echo loop anyway.
 		 * Only happens when getty starts talking to Visor.
 		 */
-		if (++rate % 1000 < 5)
-			err("too much data (%d)", count);
-		job->len = POST_BSIZE;
+		if (++rate % 1000 < 3) {
+			err("too much data (%d) from %s", count,
+			    from_user? "user": "kernel");
+		}
+		count = 512;
+	}
+
+	while (done < count) {
+		length = count - done;
+		if (length > POST_BSIZE)
+			length = POST_BSIZE;
+		if (length > port->bulk_out_size)
+			length = port->bulk_out_size;
+
+		rc = serial_post_one(port, from_user, gfp, buf + done, length);
+		if (rc <= 0) {
+			if (done != 0)
+				return done;
+			return rc;
+		}
+		done += rc;
+	}
+
+	return done;
+}
+
+static int serial_post_one(struct usb_serial_port *port, int from_user,
+    int gfp, const unsigned char *buf, int count)
+{
+	struct usb_serial *serial = get_usb_serial (port, __FUNCTION__);
+	struct usb_serial_post_job *job;
+	unsigned long flags;
+
+	dbg("%s - port %d user %d count %d", __FUNCTION__, port->number, from_user,
count);
+
+	job = kmalloc(sizeof(struct usb_serial_post_job), gfp);
+	if (job == NULL)
+		return -ENOMEM;
+
+	job->port = port;
+	if (count >= POST_BSIZE)
+		count = POST_BSIZE;
+	job->len = count;
+
+	if (from_user) {
+		if (copy_from_user(job->buff, buf, count)) {
+			kfree(job);
+			return -EFAULT;
+		}
+	} else {
+		memcpy(job->buff, buf, count);
 	}
-	memcpy(job->buff, buf, job->len);
 
 	spin_lock_irqsave(&post_lock, flags);
+	port->write_backlog += count;
 	list_add_tail(&job->link, &post_list);
 	serial->ref++;		/* Protect the port->sem from kfree() */
 	schedule_task(&post_task);
@@ -742,8 +833,13 @@
 	if (!serial)
 		return -ENODEV;
 
-	if (in_interrupt())
-		return POST_BSIZE;
+	if (in_interrupt()) {
+		retval = 0;
+		if (!port->write_busy && port->write_backlog == 0)
+			retval = port->bulk_out_size;
+		dbg("%s - returns %d", __FUNCTION__, retval);
+		return retval;
+	}
 
 	down (&port->sem);
 
@@ -776,10 +872,8 @@
 
 	down (&port->sem);
 
-	dbg("%s = port %d", __FUNCTION__, port->number);
-
 	if (!port->open_count) {
-		dbg("%s - port not open", __FUNCTION__);
+		dbg("%s - port %d: not open", __FUNCTION__, port->number);
 		goto exit;
 	}
 
@@ -1038,18 +1132,23 @@
 {
 	struct usb_serial *serial = port->serial;
 	int result;
-
-	dbg("%s - port %d", __FUNCTION__, port->number);
+	unsigned long flags;
 
 	if (count == 0) {
 		dbg("%s - write request of 0 bytes", __FUNCTION__);
 		return (0);
 	}
+	if (count < 0) {
+		err("%s - port %d: write request of %d bytes", __FUNCTION__,
+		    port->number, count);
+		return (0);
+	}
 
 	/* only do something if we have a bulk out endpoint */
 	if (serial->num_bulk_out) {
-		if (port->write_urb->status == -EINPROGRESS) {
-			dbg("%s - already writing", __FUNCTION__);
+		if (port->write_busy) {
+			/* Happens when two threads run port_helper. Watch. */
+			info("%s - already writing", __FUNCTION__);
 			return (0);
 		}
 
@@ -1058,12 +1157,10 @@
 		if (from_user) {
 			if (copy_from_user(port->write_urb->transfer_buffer, buf, count))
 				return -EFAULT;
-		}
-		else {
+		} else {
 			memcpy (port->write_urb->transfer_buffer, buf, count);
 		}
-
-		usb_serial_debug_data (__FILE__, __FUNCTION__, count,
port->write_urb->transfer_buffer);
+		dbg("%s - port %d [%d]", __FUNCTION__, port->number, count);
 
 		/* set up our urb */
 		usb_fill_bulk_urb (port->write_urb, serial->dev,
@@ -1075,10 +1172,18 @@
 				     generic_write_bulk_callback), port);
 
 		/* send the data out the bulk port */
+		port->write_busy = 1;
 		result = usb_submit_urb(port->write_urb);
-		if (result)
-			err("%s - failed submitting write urb, error %d", __FUNCTION__, result);
-		else
+		if (result) {
+			err("%s - port %d: failed submitting write urb (%d)",
+			     __FUNCTION__, port->number, result);
+			port->write_busy = 0;
+			spin_lock_irqsave(&post_lock, flags);
+			if (port->write_backlog != 0)
+				schedule_task(&post_task);
+			spin_unlock_irqrestore(&post_lock, flags);
+
+		} else
 			result = count;
 
 		return result;
@@ -1093,14 +1198,12 @@
 	struct usb_serial *serial = port->serial;
 	int room = 0;
 
-	dbg("%s - port %d", __FUNCTION__, port->number);
-	
 	if (serial->num_bulk_out) {
-		if (port->write_urb->status != -EINPROGRESS)
+		if (!port->write_busy && port->write_backlog == 0)
 			room = port->bulk_out_size;
 	}
 
-	dbg("%s - returns %d", __FUNCTION__, room);
+	dbg("%s - port %d, returns %d", __FUNCTION__, port->number, room);
 	return (room);
 }
 
@@ -1112,8 +1215,9 @@
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
 	if (serial->num_bulk_out) {
-		if (port->write_urb->status == -EINPROGRESS)
-			chars = port->write_urb->transfer_buffer_length;
+		if (port->write_busy)
+			chars += port->write_urb->transfer_buffer_length;
+		chars += port->write_backlog;	/* spin_lock... Baah */
 	}
 
 	dbg("%s - returns %d", __FUNCTION__, chars);
@@ -1177,14 +1281,16 @@
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
+	port->write_busy = 0;
+	wmb();
+
 	if (!serial) {
-		dbg("%s - bad serial pointer, exiting", __FUNCTION__);
+		err("%s - null serial pointer, exiting", __FUNCTION__);
 		return;
 	}
 
 	if (urb->status) {
 		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__,
urb->status);
-		return;
 	}
 
 	queue_task(&port->tqueue, &tq_immediate);
@@ -1210,12 +1316,18 @@
 	struct usb_serial_port *port = (struct usb_serial_port *)private;
 	struct usb_serial *serial = get_usb_serial (port, __FUNCTION__);
 	struct tty_struct *tty;
+	unsigned long flags;
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 	
 	if (!serial)
 		return;
 
+	spin_lock_irqsave(&post_lock, flags);
+	if (port->write_backlog != 0)
+		schedule_task(&post_task);
+	spin_unlock_irqrestore(&post_lock, flags);
+
 	tty = port->tty;
 	if (!tty)
 		return;
diff -urN -X dontdiff linux-2.4.22-1.2176/drivers/usb/serial/usb-serial.h
linux-2.4.22-1.2176-u1/drivers/usb/serial/usb-serial.h
--- linux-2.4.22-1.2176/drivers/usb/serial/usb-serial.h	2004-03-11 20:53:43.000000000
-0800
+++ linux-2.4.22-1.2176-u1/drivers/usb/serial/usb-serial.h	2004-03-23 11:07:12.000000000
-0800
@@ -111,6 +111,8 @@
 	int			bulk_out_size;
 	struct urb *		write_urb;
 	__u8			bulk_out_endpointAddress;
+	char			write_busy;	/* URB is active */
+	int			write_backlog;	/* Fifo used */
 
 	wait_queue_head_t	write_wait;
 	struct tq_struct	tqueue;

[Greg KH]

On Tue, Mar 23, 2004 at 02:31:53PM -0800, Pete Zaitcev wrote:
>  
>  If everything goes well, I will push this to Marcelo for 2.4.27. 
Wait, aren't I still the USB serial maintainer?  :)

>  I held back hoping for a backport, but now that I looked even more
>  at it, I think 2.6 gets it wrong, too. It will be a separate work. 
How does 2.6 get it wrong?

>  So let's just fix this.
>  
>  Code reviews are always appreciated. 
How about a patch against a clean 2.4 tree?  This is against the latest
fedora 2.4 kernel, right?

thanks,

greg k-h

[Pete Zaitcev]

On Tue, 23 Mar 2004 19:23:07 -0800
Greg KH <greg(a)kroah.com&gt; wrote:

>  On Tue, Mar 23, 2004 at 02:31:53PM -0800, Pete Zaitcev wrote:
>  > 
>  > If everything goes well, I will push this to Marcelo for 2.4.27.
>  
>  Wait, aren't I still the USB serial maintainer?  :) 
Oh duh, that's right. I just forgot :-)

>  > I held back hoping for a backport, but now that I looked even
> more
>  > at it, I think 2.6 gets it wrong, too. It will be a separate work.
>  
>  How does 2.6 get it wrong? 
That's how: I didn't agree with this paragraph:

>  > However, what about the port->open_count?
>  > It is manipulated without any locking, it seems. 
>  
>  Actually it can be gotten rid of entirely I think.  The reference count
>  of the object is now handled properly, so open_count is pretty much
>  pointless.  Now we are still relying on the fact that open() can't race
>  with disconnect() from the USB bus, which in real-life is probably ok, 
You cannot just get rid of it if you still want to call component
driver's ->open only once. Something has to count, and reference
counts do not match open counts, so you have to keep a counter for it.
An alternative would be to pass all upper level opens through to
component drivers, which would just push open counts down a level.
IIRC, we did it before, but migrated to the current scheme.
So, the open count stays. Since it stays, it has to be protected.

>  How about a patch against a clean 2.4 tree?  This is against the
> latest
>  fedora 2.4 kernel, right? 
I was going to send it out after 2.4.26, but I can send it now if you
want.

-- Pete

[Greg KH]

On Tue, Mar 23, 2004 at 07:54:37PM -0800, Pete Zaitcev wrote:
>  > > However, what about the port->open_count?
>  > > It is manipulated without any locking, it seems. 
>  > 
>  > Actually it can be gotten rid of entirely I think.  The reference count
>  > of the object is now handled properly, so open_count is pretty much
>  > pointless.  Now we are still relying on the fact that open() can't race
>  > with disconnect() from the USB bus, which in real-life is probably ok,
>  
>  You cannot just get rid of it if you still want to call component
>  driver's ->open only once. Something has to count, and reference
>  counts do not match open counts, so you have to keep a counter for it.
>  An alternative would be to pass all upper level opens through to
>  component drivers, which would just push open counts down a level.
>  IIRC, we did it before, but migrated to the current scheme.
>  So, the open count stays. Since it stays, it has to be protected. 
Ah, good point, you are correct.

>  > How about a patch against a clean 2.4 tree?  This is against the
> latest
>  > fedora 2.4 kernel, right?
>  
>  I was going to send it out after 2.4.26, but I can send it now if you
>  want. 
After 2.4.26 is fine, I'm in no hurry.

thanks,

greg k-h