From cc01f17d5cb8ac604108515735aeca72e17944c1 Mon Sep 17 00:00:00 2001 From: Johan Hovold Date: Wed, 7 Oct 2009 20:05:07 +0200 Subject: USB: ftdi_sio: re-implement read processing - 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 Cc: stable Signed-off-by: Greg Kroah-Hartman --- drivers/usb/serial/ftdi_sio.c | 383 +++++++++++++++--------------------------- 1 file changed, 131 insertions(+), 252 deletions(-) (limited to 'drivers') diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c index 75c84d9b080..9c60d6d4908 100644 --- a/drivers/usb/serial/ftdi_sio.c +++ b/drivers/usb/serial/ftdi_sio.c @@ -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) -- cgit v1.2.3