Message-ID: <20091002173308.6158d734@lxorguk.ukuu.org.uk>
Date:	Fri, 2 Oct 2009 17:33:08 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Johan Hovold <jhovold@...il.com>
Cc:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Johan Hovold <jhovold@...il.com>,
	Michael Trimarchi <trimarchi@...dalf.sssup.it>,
	Oliver Neukum <oliver@...kum.org>, linux-usb@...r.kernel.org,
	Alan Cox <alan@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: ftdi_sio: Remove tty->low_latency.

> Alan, did you have time to look at it? Are there any reasons for wanting
> to keep low_latency in ftdi_sio when it was removed from all other
> drivers processing in interrupt context (without doing work queue
> re-implementations)?

Yes for latency handling (two layers of work queue is bad) but its the
right fix for stable.

For upstream how does this look as a tidy up
ftdi_sio: simplify driver

From: Alan Cox <alan@...ux.intel.com>

This does a lot of stuff that the modern buffering logic will cover itself
so remove the cruft.

- Remove the code handling throttle half way through a packet. We have 64K
  of slack and flow control is async anyway so stopping is the wrong thing
  to do
- Remove various commented out bits
- Without the partial packet stuff we can remove the async queue stuff and
  split it into sensible functions for URB processing and for queueing urbs
  for receipt
- Remove the unused rx_bytes count. We take locks for it, we jump through
  hoops for it and we never expose it.

Signed-off-by: Alan Cox <alan@...ux.intel.com>
---

 drivers/usb/serial/ftdi_sio.c |  199 +++++++++++------------------------------
 1 files changed, 51 insertions(+), 148 deletions(-)


diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 4f883b1..f796b07 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -78,10 +78,7 @@ struct ftdi_private {
 	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;
-	unsigned long rx_bytes;
 
 	__u16 interface;	/* FT2232C, FT2232H or FT4232H port interface
 				   (0 for FT232/245) */
@@ -763,7 +760,8 @@ 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 ftdi_private *priv,
+						struct tty_struct *tty);
 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);
@@ -1549,7 +1547,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. */
@@ -1700,9 +1697,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	spin_lock_irqsave(&priv->tx_lock, flags);
 	priv->tx_bytes = 0;
 	spin_unlock_irqrestore(&priv->tx_lock, flags);
-	spin_lock_irqsave(&priv->rx_lock, flags);
-	priv->rx_bytes = 0;
-	spin_unlock_irqrestore(&priv->rx_lock, flags);
 
 	if (tty)
 		tty->low_latency = (priv->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
@@ -1730,7 +1724,6 @@ static int ftdi_open(struct tty_struct *tty, struct usb_serial_port *port)
 	spin_unlock_irqrestore(&priv->rx_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,
@@ -1787,10 +1780,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);
@@ -2019,7 +2008,6 @@ static void ftdi_read_bulk_callback(struct urb *urb)
 	struct tty_struct *tty;
 	struct ftdi_private *priv;
 	unsigned long countread;
-	unsigned long flags;
 	int status = urb->status;
 
 	if (urb->number_of_packets > 0) {
@@ -2036,94 +2024,88 @@ static void ftdi_read_bulk_callback(struct urb *urb)
 	if (port->port.count <= 0)
 		return;
 
-	tty = tty_port_tty_get(&port->port);
-	if (!tty) {
-		dbg("%s - bad tty pointer - exiting", __func__);
+	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);
 		return;
 	}
 
 	priv = usb_get_serial_port_data(port);
 	if (!priv) {
 		dbg("%s - bad port private data pointer - exiting", __func__);
-		goto out;
+		return;
 	}
 
-	if (urb != port->read_urb)
-		dev_err(&port->dev, "%s - Not my urb!\n", __func__);
+	tty = tty_port_tty_get(&port->port);
+	if (!tty) {
+		dbg("%s - bad tty pointer - exiting", __func__);
+		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;
+
+	if (urb != port->read_urb) {
+		dev_err(&port->dev, "%s - Not my urb!\n", __func__);
+		return;
 	}
 
 	/* count data bytes, but not status bytes */
 	countread = urb->actual_length;
 	countread -= 2 * DIV_ROUND_UP(countread, priv->max_packet_size);
-	spin_lock_irqsave(&priv->rx_lock, flags);
-	priv->rx_bytes += countread;
-	spin_unlock_irqrestore(&priv->rx_lock, flags);
 
-	ftdi_process_read(&priv->rx_work.work);
-out:
+	ftdi_process_read(priv, tty);
 	tty_kref_put(tty);
 } /* 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);
+static void ftdi_repost_urb(struct ftdi_private *priv)
+{
+	struct usb_serial_port *port = priv->port;
+	int result;
+
+	/* if the port is closed or throttled stop trying to read */
+	if (port->port.count <= 0 || (priv->rx_flags & THROTTLED))
+		return;
+	/* 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);
+
+	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);
+}
+
+static void ftdi_process_read(struct ftdi_private *priv, struct tty_struct *tty)
+{
 	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;
 
 	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;
-	}
-
-	priv = usb_get_serial_port_data(port);
-	if (!priv) {
-		dbg("%s - bad port private data pointer - exiting", __func__);
-		goto out;
-	}
-
 	urb = port->read_urb;
-	if (!urb) {
-		dbg("%s - bad read_urb pointer - exiting", __func__);
-		goto out;
-	}
-
 	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]);
-	}
+	/* 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]);
 
 
 	/* TO DO -- check for hung up line and handle appropriately: */
@@ -2132,7 +2114,7 @@ static void ftdi_process_read(struct work_struct *work)
 	/* if CD is dropped and the line is not CLOCAL then we should hangup */
 
 	need_flip = 0;
-	for (packet_offset = priv->rx_processed;
+	for (packet_offset = 0;
 		packet_offset < urb->actual_length; packet_offset += priv->max_packet_size) {
 		int length;
 
@@ -2155,17 +2137,6 @@ static void ftdi_process_read(struct work_struct *work)
 			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;
-		}
-
 		/* Handle errors and break */
 		error_flag = TTY_NORMAL;
 		/* Although the device uses a bitmask and hence can have
@@ -2203,79 +2174,11 @@ static void ftdi_process_read(struct work_struct *work)
 			need_flip = 1;
 		}
 
-#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..." */
 
-	/* Low latency */
 	if (need_flip)
 		tty_flip_buffer_push(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;
-
-	/* 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);
-
-		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);
-	}
-out:
-	tty_kref_put(tty);
+	ftdi_repost_urb(priv);
 } /* ftdi_process_read */
 
 
@@ -2635,7 +2538,7 @@ static void ftdi_unthrottle(struct tty_struct *tty)
 	spin_unlock_irqrestore(&priv->rx_lock, flags);
 
 	if (actually_throttled)
-		schedule_delayed_work(&priv->rx_work, 0);
+		ftdi_repost_urb(priv);
 }
 
 static int __init ftdi_init(void)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists