USB: ftdi_sio: re-implement read processing
authorJohan Hovold <jhovold@gmail.com>
Wed, 7 Oct 2009 18:05:07 +0000 (20:05 +0200)
committerGreg Kroah-Hartman <gregkh@suse.de>
Fri, 9 Oct 2009 20:52:05 +0000 (13:52 -0700)
- Re-structure read processing.
 - Kill obsolete work queue and always push to tty in completion handler.
 - Use tty_insert_flip_string instead of per character push when
   possible.
 - Fix stalled-read regression in 2.6.31 by using urb status to
   determine when port is closed rather than port count.
 - Fix race with open/close by checking ASYNCB_INITIALIZED in
   unthrottle.
 - Kill private rx_flag and lock and use throttle flags in
   usb_serial_port instead.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
Cc: stable <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
drivers/usb/serial/ftdi_sio.c

index 75c84d9..9c60d6d 100644 (file)
@@ -76,12 +76,7 @@ struct ftdi_private {
        unsigned long last_dtr_rts;     /* saved modem control outputs */
        wait_queue_head_t delta_msr_wait; /* Used for TIOCMIWAIT */
        char prev_status, diff_status;        /* Used for TIOCMIWAIT */
-       __u8 rx_flags;          /* receive state flags (throttling) */
-       spinlock_t rx_lock;     /* spinlock for receive state */
-       struct delayed_work rx_work;
        struct usb_serial_port *port;
-       int rx_processed;
-
        __u16 interface;        /* FT2232C, FT2232H or FT4232H port interface
                                   (0 for FT232/245) */
 
@@ -736,10 +731,6 @@ static const char *ftdi_chip_name[] = {
 /* Constants for read urb and write urb */
 #define BUFSZ 512
 
-/* rx_flags */
-#define THROTTLED              0x01
-#define ACTUALLY_THROTTLED     0x02
-
 /* Used for TIOCMIWAIT */
 #define FTDI_STATUS_B0_MASK    (FTDI_RS0_CTS | FTDI_RS0_DSR | FTDI_RS0_RI | FTDI_RS0_RLSD)
 #define FTDI_STATUS_B1_MASK    (FTDI_RS_BI)
@@ -762,7 +753,7 @@ static int  ftdi_write_room(struct tty_struct *tty);
 static int  ftdi_chars_in_buffer(struct tty_struct *tty);
 static void ftdi_write_bulk_callback(struct urb *urb);
 static void ftdi_read_bulk_callback(struct urb *urb);
-static void ftdi_process_read(struct work_struct *work);
+static void ftdi_process_read(struct usb_serial_port *port);
 static void ftdi_set_termios(struct tty_struct *tty,
                        struct usb_serial_port *port, struct ktermios *old);
 static int  ftdi_tiocmget(struct tty_struct *tty, struct file *file);
@@ -1525,7 +1516,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
        }
 
        kref_init(&priv->kref);
-       spin_lock_init(&priv->rx_lock);
        spin_lock_init(&priv->tx_lock);
        init_waitqueue_head(&priv->delta_msr_wait);
        /* This will push the characters through immediately rather
@@ -1547,7 +1537,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
                port->read_urb->transfer_buffer_length = BUFSZ;
        }
 
-       INIT_DELAYED_WORK(&priv->rx_work, ftdi_process_read);
        priv->port = port;
 
        /* Free port's existing write urb and transfer buffer. */
@@ -1684,6 +1673,26 @@ static int ftdi_sio_port_remove(struct usb_serial_port *port)
        return 0;
 }
 
+static int ftdi_submit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+{
+       struct urb *urb = port->read_urb;
+       struct usb_serial *serial = port->serial;
+       int result;
+
+       usb_fill_bulk_urb(urb, serial->dev,
+                          usb_rcvbulkpipe(serial->dev,
+                                       port->bulk_in_endpointAddress),
+                          urb->transfer_buffer,
+                          urb->transfer_buffer_length,
+                          ftdi_read_bulk_callback, port);
+       result = usb_submit_urb(urb, mem_flags);
+       if (result)
+               dev_err(&port->dev,
+                       "%s - failed submitting read urb, error %d\n",
+                                                       __func__, result);
+       return result;
+}
+
 static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
 { /* ftdi_open */
        struct usb_device *dev = port->serial->dev;
@@ -1717,23 +1726,14 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
                ftdi_set_termios(tty, port, tty->termios);
 
        /* Not throttled */
-       spin_lock_irqsave(&priv->rx_lock, flags);
-       priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
-       spin_unlock_irqrestore(&priv->rx_lock, flags);
+       spin_lock_irqsave(&port->lock, flags);
+       port->throttled = 0;
+       port->throttle_req = 0;
+       spin_unlock_irqrestore(&port->lock, flags);
 
        /* Start reading from the device */
-       priv->rx_processed = 0;
-       usb_fill_bulk_urb(port->read_urb, dev,
-                       usb_rcvbulkpipe(dev, port->bulk_in_endpointAddress),
-                       port->read_urb->transfer_buffer,
-                               port->read_urb->transfer_buffer_length,
-                       ftdi_read_bulk_callback, port);
-       result = usb_submit_urb(port->read_urb, GFP_KERNEL);
-       if (result)
-               dev_err(&port->dev,
-                       "%s - failed submitting read urb, error %d\n",
-                       __func__, result);
-       else
+       result = ftdi_submit_read_urb(port, GFP_KERNEL);
+       if (!result)
                kref_get(&priv->kref);
 
        return result;
@@ -1779,10 +1779,6 @@ static void ftdi_close(struct usb_serial_port *port)
 
        dbg("%s", __func__);
 
-
-       /* cancel any scheduled reading */
-       cancel_delayed_work_sync(&priv->rx_work);
-
        /* shutdown our bulk read */
        usb_kill_urb(port->read_urb);
        kref_put(&priv->kref, ftdi_sio_priv_release);
@@ -2005,236 +2001,121 @@ static int ftdi_chars_in_buffer(struct tty_struct *tty)
        return buffered;
 }
 
-static void ftdi_read_bulk_callback(struct urb *urb)
+static int ftdi_process_packet(struct tty_struct *tty,
+               struct usb_serial_port *port, struct ftdi_private *priv,
+               char *packet, int len)
 {
-       struct usb_serial_port *port = urb->context;
-       struct ftdi_private *priv;
-       int status = urb->status;
-
-       dbg("%s - port %d", __func__, port->number);
-
-       if (port->port.count <= 0)
-               return;
-
-       if (status) {
-               /* This will happen at close every time so it is a dbg not an
-                  err */
-               dbg("(this is ok on close) nonzero read bulk status received: %d", status);
-               goto out;
-       }
-
-       priv = usb_get_serial_port_data(port);
-       ftdi_process_read(&priv->rx_work.work);
-} /* ftdi_read_bulk_callback */
-
-
-static void ftdi_process_read(struct work_struct *work)
-{ /* ftdi_process_read */
-       struct ftdi_private *priv =
-               container_of(work, struct ftdi_private, rx_work.work);
-       struct usb_serial_port *port = priv->port;
-       struct urb *urb;
-       struct tty_struct *tty;
-       char error_flag;
-       unsigned char *data;
-
        int i;
-       int result;
-       int need_flip;
-       int packet_offset;
-       unsigned long flags;
+       char status;
+       char flag;
+       char *ch;
 
        dbg("%s - port %d", __func__, port->number);
 
-       if (port->port.count <= 0)
-               return;
-
-       tty = tty_port_tty_get(&port->port);
-       if (!tty) {
-               dbg("%s - bad tty pointer - exiting", __func__);
-               return;
+       if (len < 2) {
+               dbg("malformed packet");
+               return 0;
        }
 
-       priv = usb_get_serial_port_data(port);
-       if (!priv) {
-               dbg("%s - bad port private data pointer - exiting", __func__);
-               goto out;
+       /* Compare new line status to the old one, signal if different/
+          N.B. packet may be processed more than once, but differences
+          are only processed once.  */
+       status = packet[0] & FTDI_STATUS_B0_MASK;
+       if (status != priv->prev_status) {
+               priv->diff_status |= status ^ priv->prev_status;
+               wake_up_interruptible(&priv->delta_msr_wait);
+               priv->prev_status = status;
        }
 
-       urb = port->read_urb;
-       if (!urb) {
-               dbg("%s - bad read_urb pointer - exiting", __func__);
-               goto out;
+       /*
+        * Although the device uses a bitmask and hence can have multiple
+        * errors on a packet - the order here sets the priority the error is
+        * returned to the tty layer.
+        */
+       flag = TTY_NORMAL;
+       if (packet[1] & FTDI_RS_OE) {
+               flag = TTY_OVERRUN;
+               dbg("OVERRRUN error");
        }
-
-       data = urb->transfer_buffer;
-
-       if (priv->rx_processed) {
-               dbg("%s - already processed: %d bytes, %d remain", __func__,
-                               priv->rx_processed,
-                               urb->actual_length - priv->rx_processed);
-       } else {
-               /* The first two bytes of every read packet are status */
-               if (urb->actual_length > 2)
-                       usb_serial_debug_data(debug, &port->dev, __func__,
-                                               urb->actual_length, data);
-               else
-                       dbg("Status only: %03oo %03oo", data[0], data[1]);
+       if (packet[1] & FTDI_RS_BI) {
+               flag = TTY_BREAK;
+               dbg("BREAK received");
+               usb_serial_handle_break(port);
+       }
+       if (packet[1] & FTDI_RS_PE) {
+               flag = TTY_PARITY;
+               dbg("PARITY error");
+       }
+       if (packet[1] & FTDI_RS_FE) {
+               flag = TTY_FRAME;
+               dbg("FRAMING error");
        }
 
-
-       /* TO DO -- check for hung up line and handle appropriately: */
-       /*   send hangup  */
-       /* See acm.c - you do a tty_hangup  - eg tty_hangup(tty) */
-       /* if CD is dropped and the line is not CLOCAL then we should hangup */
-
-       need_flip = 0;
-       for (packet_offset = priv->rx_processed;
-               packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
-               int length;
-
-               /* Compare new line status to the old one, signal if different/
-                  N.B. packet may be processed more than once, but differences
-                  are only processed once.  */
-               char new_status = data[packet_offset + 0] &
-                                               FTDI_STATUS_B0_MASK;
-               if (new_status != priv->prev_status) {
-                       priv->diff_status |=
-                               new_status ^ priv->prev_status;
-                       wake_up_interruptible(&priv->delta_msr_wait);
-                       priv->prev_status = new_status;
-               }
-
-               length = min_t(u32, priv->max_packet_size, urb->actual_length-packet_offset)-2;
-               if (length < 0) {
-                       dev_err(&port->dev, "%s - bad packet length: %d\n",
-                               __func__, length+2);
-                       length = 0;
-               }
-
-               if (priv->rx_flags & THROTTLED) {
-                       dbg("%s - throttled", __func__);
-                       break;
-               }
-               if (tty_buffer_request_room(tty, length) < length) {
-                       /* break out & wait for throttling/unthrottling to
-                          happen */
-                       dbg("%s - receive room low", __func__);
-                       break;
+       len -= 2;
+       if (!len)
+               return 0;       /* status only */
+       ch = packet + 2;
+
+       if (!(port->console && port->sysrq) && flag == TTY_NORMAL)
+               tty_insert_flip_string(tty, ch, len);
+       else {
+               for (i = 0; i < len; i++, ch++) {
+                       if (!usb_serial_handle_sysrq_char(tty, port, *ch))
+                               tty_insert_flip_char(tty, *ch, flag);
                }
+       }
+       return len;
+}
 
-               /* Handle errors and break */
-               error_flag = TTY_NORMAL;
-               /* Although the device uses a bitmask and hence can have
-                  multiple errors on a packet - the order here sets the
-                  priority the error is returned to the tty layer  */
+static void ftdi_process_read(struct usb_serial_port *port)
+{
+       struct urb *urb = port->read_urb;
+       struct tty_struct *tty;
+       struct ftdi_private *priv = usb_get_serial_port_data(port);
+       char *data = (char *)urb->transfer_buffer;
+       int i;
+       int len;
+       int count = 0;
 
-               if (data[packet_offset+1] & FTDI_RS_OE) {
-                       error_flag = TTY_OVERRUN;
-                       dbg("OVERRRUN error");
-               }
-               if (data[packet_offset+1] & FTDI_RS_BI) {
-                       error_flag = TTY_BREAK;
-                       dbg("BREAK received");
-                       usb_serial_handle_break(port);
-               }
-               if (data[packet_offset+1] & FTDI_RS_PE) {
-                       error_flag = TTY_PARITY;
-                       dbg("PARITY error");
-               }
-               if (data[packet_offset+1] & FTDI_RS_FE) {
-                       error_flag = TTY_FRAME;
-                       dbg("FRAMING error");
-               }
-               if (length > 0) {
-                       for (i = 2; i < length+2; i++) {
-                               /* Note that the error flag is duplicated for
-                                  every character received since we don't know
-                                  which character it applied to */
-                               if (!usb_serial_handle_sysrq_char(tty, port,
-                                               data[packet_offset + i]))
-                                       tty_insert_flip_char(tty,
-                                               data[packet_offset + i],
-                                               error_flag);
-                       }
-                       need_flip = 1;
-               }
+       tty = tty_port_tty_get(&port->port);
+       if (!tty)
+               return;
 
-#ifdef NOT_CORRECT_BUT_KEEPING_IT_FOR_NOW
-               /* if a parity error is detected you get status packets forever
-                  until a character is sent without a parity error.
-                  This doesn't work well since the application receives a
-                  never ending stream of bad data - even though new data
-                  hasn't been sent. Therefore I (bill) have taken this out.
-                  However - this might make sense for framing errors and so on
-                  so I am leaving the code in for now.
-               */
-               else {
-                       if (error_flag != TTY_NORMAL) {
-                               dbg("error_flag is not normal");
-                               /* In this case it is just status - if that is
-                                  an error send a bad character */
-                               if (tty->flip.count >= TTY_FLIPBUF_SIZE)
-                                       tty_flip_buffer_push(tty);
-                               tty_insert_flip_char(tty, 0xff, error_flag);
-                               need_flip = 1;
-                       }
-               }
-#endif
-       } /* "for(packet_offset=0..." */
+       for (i = 0; i < urb->actual_length; i += priv->max_packet_size) {
+               len = min_t(int, urb->actual_length - i, priv->max_packet_size);
+               count += ftdi_process_packet(tty, port, priv, &data[i], len);
+       }
 
-       /* Low latency */
-       if (need_flip)
+       if (count)
                tty_flip_buffer_push(tty);
+       tty_kref_put(tty);
+}
 
-       if (packet_offset < urb->actual_length) {
-               /* not completely processed - record progress */
-               priv->rx_processed = packet_offset;
-               dbg("%s - incomplete, %d bytes processed, %d remain",
-                               __func__, packet_offset,
-                               urb->actual_length - packet_offset);
-               /* check if we were throttled while processing */
-               spin_lock_irqsave(&priv->rx_lock, flags);
-               if (priv->rx_flags & THROTTLED) {
-                       priv->rx_flags |= ACTUALLY_THROTTLED;
-                       spin_unlock_irqrestore(&priv->rx_lock, flags);
-                       dbg("%s - deferring remainder until unthrottled",
-                                       __func__);
-                       goto out;
-               }
-               spin_unlock_irqrestore(&priv->rx_lock, flags);
-               /* if the port is closed stop trying to read */
-               if (port->port.count > 0)
-                       /* delay processing of remainder */
-                       schedule_delayed_work(&priv->rx_work, 1);
-               else
-                       dbg("%s - port is closed", __func__);
-               goto out;
-       }
-
-       /* urb is completely processed */
-       priv->rx_processed = 0;
+static void ftdi_read_bulk_callback(struct urb *urb)
+{
+       struct usb_serial_port *port = urb->context;
+       unsigned long flags;
 
-       /* if the port is closed stop trying to read */
-       if (port->port.count > 0) {
-               /* Continue trying to always read  */
-               usb_fill_bulk_urb(port->read_urb, port->serial->dev,
-                       usb_rcvbulkpipe(port->serial->dev,
-                                       port->bulk_in_endpointAddress),
-                       port->read_urb->transfer_buffer,
-                       port->read_urb->transfer_buffer_length,
-                       ftdi_read_bulk_callback, port);
+       dbg("%s - port %d", __func__, port->number);
 
-               result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
-               if (result)
-                       dev_err(&port->dev,
-                               "%s - failed resubmitting read urb, error %d\n",
-                               __func__, result);
+       if (urb->status) {
+               dbg("%s - nonzero read bulk status received: %d",
+                                               __func__, urb->status);
+               return;
        }
-out:
-       tty_kref_put(tty);
-} /* ftdi_process_read */
 
+       usb_serial_debug_data(debug, &port->dev, __func__,
+                               urb->actual_length, urb->transfer_buffer);
+       ftdi_process_read(port);
+
+       spin_lock_irqsave(&port->lock, flags);
+       port->throttled = port->throttle_req;
+       if (!port->throttled) {
+               spin_unlock_irqrestore(&port->lock, flags);
+               ftdi_submit_read_urb(port, GFP_ATOMIC);
+       } else
+               spin_unlock_irqrestore(&port->lock, flags);
+}
 
 static void ftdi_break_ctl(struct tty_struct *tty, int break_state)
 {
@@ -2566,33 +2447,31 @@ static int ftdi_ioctl(struct tty_struct *tty, struct file *file,
 static void ftdi_throttle(struct tty_struct *tty)
 {
        struct usb_serial_port *port = tty->driver_data;
-       struct ftdi_private *priv = usb_get_serial_port_data(port);
        unsigned long flags;
 
        dbg("%s - port %d", __func__, port->number);
 
-       spin_lock_irqsave(&priv->rx_lock, flags);
-       priv->rx_flags |= THROTTLED;
-       spin_unlock_irqrestore(&priv->rx_lock, flags);
+       spin_lock_irqsave(&port->lock, flags);
+       port->throttle_req = 1;
+       spin_unlock_irqrestore(&port->lock, flags);
 }
 
-
-static void ftdi_unthrottle(struct tty_struct *tty)
+void ftdi_unthrottle(struct tty_struct *tty)
 {
        struct usb_serial_port *port = tty->driver_data;
-       struct ftdi_private *priv = usb_get_serial_port_data(port);
-       int actually_throttled;
+       int was_throttled;
        unsigned long flags;
 
        dbg("%s - port %d", __func__, port->number);
 
-       spin_lock_irqsave(&priv->rx_lock, flags);
-       actually_throttled = priv->rx_flags & ACTUALLY_THROTTLED;
-       priv->rx_flags &= ~(THROTTLED | ACTUALLY_THROTTLED);
-       spin_unlock_irqrestore(&priv->rx_lock, flags);
+       spin_lock_irqsave(&port->lock, flags);
+       was_throttled = port->throttled;
+       port->throttled = port->throttle_req = 0;
+       spin_unlock_irqrestore(&port->lock, flags);
 
-       if (actually_throttled)
-               schedule_delayed_work(&priv->rx_work, 0);
+       /* Resubmit urb if throttled and open. */
+       if (was_throttled && test_bit(ASYNCB_INITIALIZED, &port->port.flags))
+               ftdi_submit_read_urb(port, GFP_KERNEL);
 }
 
 static int __init ftdi_init(void)