[PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Alan Cox]

This sorts out the sdio uart handling of the tty layer. The existing code
has lots of races and other fun bugs. Beat it into the current tty format for
hotpluggable devices. The updates are intentionally modelled on Alan Stern's
USB serial approach which is defintiely "best practice" right now. Also clean
it up as we go and sort the circ stuff out.

I don't have hardware to test this so hopefully someone out there does,
otherwise given the nature of the bugs involved the driver will be progressed
to BROKEN and removal.
---

Alan Cox (6):
      sdio_uart: Style fixes
      sdio_uart: Use kfifo instead of the messy circ stuff
      sdio_uart: Fix termios handling
      sdio_uart: Switch to the open/close helpers
      sdio_uart: Move the open lock
      sdio_uart: refcount the tty objects


 drivers/mmc/card/sdio_uart.c |  333 ++++++++++++++++++++++--------------------
 1 files changed, 176 insertions(+), 157 deletions(-)

-- 
"and on the seventh day she exited append mode"

[PATCH 1/6] sdio_uart: refcount the tty objects

[Alan Cox]

The tty can go away underneath us, so we must refcount it. Do the naïve
implementation initially. We will worry about startup shortly.

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

 drivers/mmc/card/sdio_uart.c |   58 ++++++++++++++++++++++++++++++------------
 1 files changed, 41 insertions(+), 17 deletions(-)


diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index c2759db..8a4564a 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -172,8 +172,13 @@ static void sdio_uart_port_remove(struct sdio_uart_port *port)
 	sdio_claim_host(func);
 	port->func = NULL;
 	mutex_unlock(&port->func_lock);
-	if (port->opened)
-		tty_hangup(port->port.tty);
+	if (port->opened) {
+		struct tty_struct *tty = tty_port_tty_get(&port->port);
+		/* tty_hangup is async so is this safe as is ?? */
+		if (tty)
+			tty_hangup(tty);
+		tty_kref_put(tty);
+	}
 	mutex_unlock(&port->open_lock);
 	sdio_release_irq(func);
 	sdio_disable_func(func);
@@ -392,7 +397,7 @@ static void sdio_uart_stop_rx(struct sdio_uart_port *port)
 static void sdio_uart_receive_chars(struct sdio_uart_port *port,
 				    unsigned int *status)
 {
-	struct tty_struct *tty = port->port.tty;
+	struct tty_struct *tty = tty_port_tty_get(&port->port);
 	unsigned int ch, flag;
 	int max_count = 256;
 
@@ -429,25 +434,30 @@ static void sdio_uart_receive_chars(struct sdio_uart_port *port,
 		}
 
 		if ((*status & port->ignore_status_mask & ~UART_LSR_OE) == 0)
-			tty_insert_flip_char(tty, ch, flag);
+			if (tty)
+				tty_insert_flip_char(tty, ch, flag);
 
 		/*
 		 * Overrun is special.  Since it's reported immediately,
 		 * it doesn't affect the current character.
 		 */
 		if (*status & ~port->ignore_status_mask & UART_LSR_OE)
-			tty_insert_flip_char(tty, 0, TTY_OVERRUN);
+			if (tty)
+				tty_insert_flip_char(tty, 0, TTY_OVERRUN);
 
 		*status = sdio_in(port, UART_LSR);
 	} while ((*status & UART_LSR_DR) && (max_count-- > 0));
-	tty_flip_buffer_push(tty);
+	if (tty) {
+		tty_flip_buffer_push(tty);
+		tty_kref_put(tty);
+	}
 }
 
 static void sdio_uart_transmit_chars(struct sdio_uart_port *port)
 {
 	struct circ_buf *xmit = &port->xmit;
 	int count;
-	struct tty_struct *tty = port->port.tty;
+	struct tty_struct *tty;
 
 	if (port->x_char) {
 		sdio_out(port, UART_TX, port->x_char);
@@ -455,7 +465,10 @@ static void sdio_uart_transmit_chars(struct sdio_uart_port *port)
 		port->x_char = 0;
 		return;
 	}
-	if (circ_empty(xmit) || tty->stopped || tty->hw_stopped) {
+
+	tty = tty_port_tty_get(&port->port);
+
+	if (tty == NULL || circ_empty(xmit) || tty->stopped || tty->hw_stopped) {
 		sdio_uart_stop_tx(port);
 		return;
 	}
@@ -474,12 +487,13 @@ static void sdio_uart_transmit_chars(struct sdio_uart_port *port)
 
 	if (circ_empty(xmit))
 		sdio_uart_stop_tx(port);
+	tty_kref_put(tty);
 }
 
 static void sdio_uart_check_modem_status(struct sdio_uart_port *port)
 {
 	int status;
-	struct tty_struct *tty = port->port.tty;
+	struct tty_struct *tty;
 
 	status = sdio_in(port, UART_MSR);
 
@@ -494,7 +508,8 @@ static void sdio_uart_check_modem_status(struct sdio_uart_port *port)
 		port->icount.dcd++;
 	if (status & UART_MSR_DCTS) {
 		port->icount.cts++;
-		if (tty->termios->c_cflag & CRTSCTS) {
+		tty = tty_port_tty_get(&port->port);
+		if (tty && (tty->termios->c_cflag & CRTSCTS)) {
 			int cts = (status & UART_MSR_CTS);
 			if (tty->hw_stopped) {
 				if (cts) {
@@ -509,6 +524,7 @@ static void sdio_uart_check_modem_status(struct sdio_uart_port *port)
 				}
 			}
 		}
+		tty_kref_put(tty);
 	}
 }
 
@@ -548,8 +564,10 @@ static void sdio_uart_irq(struct sdio_func *func)
 static int sdio_uart_startup(struct sdio_uart_port *port)
 {
 	unsigned long page;
-	int ret;
-	struct tty_struct *tty = port->port.tty;
+	int ret = -ENOMEM;
+	struct tty_struct *tty = tty_port_tty_get(&port->port);
+
+	/* FIXME: What if it is NULL ?? */
 
 	/*
 	 * Set the TTY IO error marker - we will only clear this
@@ -560,7 +578,7 @@ static int sdio_uart_startup(struct sdio_uart_port *port)
 	/* Initialise and allocate the transmit buffer. */
 	page = __get_free_page(GFP_KERNEL);
 	if (!page)
-		return -ENOMEM;
+		goto err0;
 	port->xmit.buf = (unsigned char *)page;
 	circ_clear(&port->xmit);
 
@@ -614,6 +632,7 @@ static int sdio_uart_startup(struct sdio_uart_port *port)
 	sdio_uart_irq(port->func);
 
 	sdio_uart_release_func(port);
+	tty_kref_put(tty);
 	return 0;
 
 err3:
@@ -622,12 +641,15 @@ err2:
 	sdio_uart_release_func(port);
 err1:
 	free_page((unsigned long)port->xmit.buf);
+err0:
+	tty_kref_put(tty);
 	return ret;
 }
 
 static void sdio_uart_shutdown(struct sdio_uart_port *port)
 {
 	int ret;
+	struct tty_struct *tty;
 
 	ret = sdio_uart_claim_func(port);
 	if (ret)
@@ -637,9 +659,11 @@ static void sdio_uart_shutdown(struct sdio_uart_port *port)
 
 	/* TODO: wait here for TX FIFO to drain */
 
+	tty = tty_port_tty_get(&port->port);
 	/* Turn off DTR and RTS early. */
-	if (port->port.tty->termios->c_cflag & HUPCL)
+	if (tty && (tty->termios->c_cflag & HUPCL))
 		sdio_uart_clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
+	tty_kref_put(tty);
 
 	/* Disable interrupts from this port */
 	sdio_release_irq(port->func);
@@ -688,11 +712,11 @@ static int sdio_uart_open(struct tty_struct *tty, struct file *filp)
 
 	if (!port->opened) {
 		tty->driver_data = port;
-		port->port.tty = tty;
+		tty_port_tty_set(&port->port, tty);
 		ret = sdio_uart_startup(port);
 		if (ret) {
 			tty->driver_data = NULL;
-			port->port.tty = NULL;
+			tty_port_tty_set(&port->port, NULL);
 			mutex_unlock(&port->open_lock);
 			sdio_uart_port_put(port);
 			return ret;
@@ -727,7 +751,7 @@ static void sdio_uart_close(struct tty_struct *tty, struct file * filp)
 		tty->closing = 1;
 		sdio_uart_shutdown(port);
 		tty_ldisc_flush(tty);
-		port->port.tty = NULL;
+		tty_port_tty_set(&port->port, NULL);
 		tty->driver_data = NULL;
 		tty->closing = 0;
 	}

[PATCH 2/6] sdio_uart: Move the open lock

[Alan Cox]

When we move to the tty_port logic the port mutex will protect open v close
v hangup. Move to this first in the existing open code so we have a bisection
point.

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

 drivers/mmc/card/sdio_uart.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)


diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index 8a4564a..97432c0 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -78,7 +78,6 @@ struct sdio_uart_port {
 	struct tty_struct	*tty;
 	unsigned int		index;
 	unsigned int		opened;
-	struct mutex		open_lock;
 	struct sdio_func	*func;
 	struct mutex		func_lock;
 	struct task_struct	*in_sdio_uart_irq;
@@ -103,7 +102,6 @@ static int sdio_uart_add_port(struct sdio_uart_port *port)
 	int index, ret = -EBUSY;
 
 	kref_init(&port->kref);
-	mutex_init(&port->open_lock);
 	mutex_init(&port->func_lock);
 	spin_lock_init(&port->write_lock);
 
@@ -166,7 +164,7 @@ static void sdio_uart_port_remove(struct sdio_uart_port *port)
 	 * give up on that port ASAP.
 	 * Beware: the lock ordering is critical.
 	 */
-	mutex_lock(&port->open_lock);
+	mutex_lock(&port->port.mutex);
 	mutex_lock(&port->func_lock);
 	func = port->func;
 	sdio_claim_host(func);
@@ -179,7 +177,7 @@ static void sdio_uart_port_remove(struct sdio_uart_port *port)
 			tty_hangup(tty);
 		tty_kref_put(tty);
 	}
-	mutex_unlock(&port->open_lock);
+	mutex_unlock(&port->port.mutex);
 	sdio_release_irq(func);
 	sdio_disable_func(func);
 	sdio_release_host(func);
@@ -698,14 +696,14 @@ static int sdio_uart_open(struct tty_struct *tty, struct file *filp)
 	if (!port)
 		return -ENODEV;
 
-	mutex_lock(&port->open_lock);
+	mutex_lock(&port->port.mutex);
 
 	/*
 	 * Make sure not to mess up with a dead port
 	 * which has not been closed yet.
 	 */
 	if (tty->driver_data && tty->driver_data != port) {
-		mutex_unlock(&port->open_lock);
+		mutex_unlock(&port->port.mutex);
 		sdio_uart_port_put(port);
 		return -EBUSY;
 	}
@@ -717,13 +715,13 @@ static int sdio_uart_open(struct tty_struct *tty, struct file *filp)
 		if (ret) {
 			tty->driver_data = NULL;
 			tty_port_tty_set(&port->port, NULL);
-			mutex_unlock(&port->open_lock);
+			mutex_unlock(&port->port.mutex);
 			sdio_uart_port_put(port);
 			return ret;
 		}
 	}
 	port->opened++;
-	mutex_unlock(&port->open_lock);
+	mutex_unlock(&port->port.mutex);
 	return 0;
 }
 
@@ -734,7 +732,7 @@ static void sdio_uart_close(struct tty_struct *tty, struct file * filp)
 	if (!port)
 		return;
 
-	mutex_lock(&port->open_lock);
+	mutex_lock(&port->port.mutex);
 	BUG_ON(!port->opened);
 
 	/*
@@ -743,7 +741,7 @@ static void sdio_uart_close(struct tty_struct *tty, struct file * filp)
 	 * is larger than port->count.
 	 */
 	if (tty->count > port->opened) {
-		mutex_unlock(&port->open_lock);
+		mutex_unlock(&port->port.mutex);
 		return;
 	}
 
@@ -755,7 +753,7 @@ static void sdio_uart_close(struct tty_struct *tty, struct file * filp)
 		tty->driver_data = NULL;
 		tty->closing = 0;
 	}
-	mutex_unlock(&port->open_lock);
+	mutex_unlock(&port->port.mutex);
 	sdio_uart_port_put(port);
 }
 

[PATCH 3/6] sdio_uart: Switch to the open/close helpers

[Alan Cox]

Gets us proper tty semantics, removes some code and fixes up a few corner
case races (hangup during open etc)

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

 drivers/mmc/card/sdio_uart.c |  190 +++++++++++++++++++++++++-----------------
 1 files changed, 114 insertions(+), 76 deletions(-)


diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index 97432c0..fd00453 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -559,13 +559,46 @@ static void sdio_uart_irq(struct sdio_func *func)
 	port->in_sdio_uart_irq = NULL;
 }
 
-static int sdio_uart_startup(struct sdio_uart_port *port)
+/**
+ *	uart_dtr_rts		-	 port helper to set uart signals
+ *	@tport: tty port to be updated
+ *	@onoff: set to turn on DTR/RTS
+ *
+ *	Called by the tty port helpers when the modem signals need to be
+ *	adjusted during an open, close and hangup.
+ */
+
+static void uart_dtr_rts(struct tty_port *tport, int onoff)
 {
-	unsigned long page;
-	int ret = -ENOMEM;
-	struct tty_struct *tty = tty_port_tty_get(&port->port);
+	struct sdio_uart_port *port =
+			container_of(tport, struct sdio_uart_port, port);
+	if (onoff == 0)
+		sdio_uart_clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
+	else
+		sdio_uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
+}
 
-	/* FIXME: What if it is NULL ?? */
+/**
+ *	sdio_uart_activate	-	start up hardware
+ *	@tport: tty port to activate
+ *	@tty: tty bound to this port
+ *
+ *	Activate a tty port. The port locking guarantees us this will be
+ *	run exactly once per set of opens, and if successful will see the
+ *	shutdown method run exactly once to match. Start up and shutdown are
+ *	protected from each other by the internal locking and will not run
+ *	at the same time even during a hangup event.
+ *
+ *	If we successfully start up the port we take an extra kref as we
+ *	will keep it around until shutdown when the kref is dropped.
+ */
+
+static int sdio_uart_activate(struct tty_port *tport, struct tty_struct *tty)
+{
+	struct sdio_uart_port *port =
+			container_of(tport, struct sdio_uart_port, port);
+	unsigned long page;
+	int ret;
 
 	/*
 	 * Set the TTY IO error marker - we will only clear this
@@ -576,7 +609,7 @@ static int sdio_uart_startup(struct sdio_uart_port *port)
 	/* Initialise and allocate the transmit buffer. */
 	page = __get_free_page(GFP_KERNEL);
 	if (!page)
-		goto err0;
+		return -ENOMEM;
 	port->xmit.buf = (unsigned char *)page;
 	circ_clear(&port->xmit);
 
@@ -630,7 +663,6 @@ static int sdio_uart_startup(struct sdio_uart_port *port)
 	sdio_uart_irq(port->func);
 
 	sdio_uart_release_func(port);
-	tty_kref_put(tty);
 	return 0;
 
 err3:
@@ -639,15 +671,25 @@ err2:
 	sdio_uart_release_func(port);
 err1:
 	free_page((unsigned long)port->xmit.buf);
-err0:
-	tty_kref_put(tty);
 	return ret;
 }
 
-static void sdio_uart_shutdown(struct sdio_uart_port *port)
+	
+/**
+ *	sdio_uart_shutdown	-	stop hardware
+ *	@tport: tty port to shut down
+ *
+ *	Deactivate a tty port. The port locking guarantees us this will be
+ *	run only if a successful matching activate already ran. The two are
+ *	protected from each other by the internal locking and will not run
+ *	at the same time even during a hangup event.
+ */
+
+static void sdio_uart_shutdown(struct tty_port *tport)
 {
+	struct sdio_uart_port *port =
+			container_of(tport, struct sdio_uart_port, port);
 	int ret;
-	struct tty_struct *tty;
 
 	ret = sdio_uart_claim_func(port);
 	if (ret)
@@ -655,14 +697,6 @@ static void sdio_uart_shutdown(struct sdio_uart_port *port)
 
 	sdio_uart_stop_rx(port);
 
-	/* TODO: wait here for TX FIFO to drain */
-
-	tty = tty_port_tty_get(&port->port);
-	/* Turn off DTR and RTS early. */
-	if (tty && (tty->termios->c_cflag & HUPCL))
-		sdio_uart_clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
-	tty_kref_put(tty);
-
 	/* Disable interrupts from this port */
 	sdio_release_irq(port->func);
 	port->ier = 0;
@@ -687,74 +721,68 @@ skip:
 	free_page((unsigned long)port->xmit.buf);
 }
 
-static int sdio_uart_open(struct tty_struct *tty, struct file *filp)
+/**
+ *	sdio_uart_install	-	install method
+ *	@driver: the driver in use (sdio_uart in our case)
+ *	@tty: the tty being bound
+ *
+ *	Look up and bind the tty and the driver together. Initialize
+ *	any needed private data (in our case the termios)
+ */
+
+static int sdio_uart_install(struct tty_driver *driver, struct tty_struct *tty)
 {
-	struct sdio_uart_port *port;
-	int ret;
+	int idx = tty->index;
+	struct sdio_uart_port *port = sdio_uart_port_get(idx);
+	int ret = tty_init_termios(tty);
+
+	if (ret == 0) {
+		tty_driver_kref_get(driver);
+		tty->count++;
+		/* This is the ref sdio_uart_port get provided */
+		tty->driver_data = port;
+		driver->ttys[idx] = tty;
+	} else
+		sdio_uart_port_put(port);
+	return ret;
+		
+}
 
-	port = sdio_uart_port_get(tty->index);
-	if (!port)
-		return -ENODEV;
+/**
+ *	sdio_uart_cleanup	-	called on the last tty kref drop
+ *	@tty: the tty being destroyed
+ *
+ *	Called asynchronously when the last reference to the tty is dropped.
+ *	We cannot destroy the tty->driver_data port kref until this point
+ */
 
-	mutex_lock(&port->port.mutex);
+static void sdio_uart_cleanup(struct tty_struct *tty)
+{
+	struct sdio_uart_port *port = tty->driver_data;
+	tty->driver_data = NULL;	/* Bug trap */
+	sdio_uart_port_put(port);
+}
 
-	/*
-	 * Make sure not to mess up with a dead port
-	 * which has not been closed yet.
-	 */
-	if (tty->driver_data && tty->driver_data != port) {
-		mutex_unlock(&port->port.mutex);
-		sdio_uart_port_put(port);
-		return -EBUSY;
-	}
+/*
+ *	Open/close/hangup is now entirely boilerplate
+ */
 
-	if (!port->opened) {
-		tty->driver_data = port;
-		tty_port_tty_set(&port->port, tty);
-		ret = sdio_uart_startup(port);
-		if (ret) {
-			tty->driver_data = NULL;
-			tty_port_tty_set(&port->port, NULL);
-			mutex_unlock(&port->port.mutex);
-			sdio_uart_port_put(port);
-			return ret;
-		}
-	}
-	port->opened++;
-	mutex_unlock(&port->port.mutex);
-	return 0;
+static int sdio_uart_open(struct tty_struct *tty, struct file *filp)
+{
+	struct sdio_uart_port *port = tty->driver_data;
+	return tty_port_open(&port->port, tty, filp);
 }
 
 static void sdio_uart_close(struct tty_struct *tty, struct file * filp)
 {
 	struct sdio_uart_port *port = tty->driver_data;
+	tty_port_close(&port->port, tty, filp);
+}
 
-	if (!port)
-		return;
-
-	mutex_lock(&port->port.mutex);
-	BUG_ON(!port->opened);
-
-	/*
-	 * This is messy.  The tty layer calls us even when open()
-	 * returned an error.  Ignore this close request if tty->count
-	 * is larger than port->count.
-	 */
-	if (tty->count > port->opened) {
-		mutex_unlock(&port->port.mutex);
-		return;
-	}
-
-	if (--port->opened == 0) {
-		tty->closing = 1;
-		sdio_uart_shutdown(port);
-		tty_ldisc_flush(tty);
-		tty_port_tty_set(&port->port, NULL);
-		tty->driver_data = NULL;
-		tty->closing = 0;
-	}
-	mutex_unlock(&port->port.mutex);
-	sdio_uart_port_put(port);
+static void sdio_uart_hangup(struct tty_struct *tty)
+{
+	struct sdio_uart_port *port = tty->driver_data;
+	tty_port_hangup(&port->port);
 }
 
 static int sdio_uart_write(struct tty_struct * tty, const unsigned char *buf,
@@ -1020,6 +1048,12 @@ static const struct file_operations sdio_uart_proc_fops = {
 	.release	= single_release,
 };
 
+static const struct tty_port_operations sdio_uart_port_ops = {
+	.dtr_rts = uart_dtr_rts,
+	.shutdown = sdio_uart_shutdown,
+	.activate = sdio_uart_activate,
+};
+	
 static const struct tty_operations sdio_uart_ops = {
 	.open			= sdio_uart_open,
 	.close			= sdio_uart_close,
@@ -1030,9 +1064,12 @@ static const struct tty_operations sdio_uart_ops = {
 	.throttle		= sdio_uart_throttle,
 	.unthrottle		= sdio_uart_unthrottle,
 	.set_termios		= sdio_uart_set_termios,
+	.hangup			= sdio_uart_hangup,
 	.break_ctl		= sdio_uart_break_ctl,
 	.tiocmget		= sdio_uart_tiocmget,
 	.tiocmset		= sdio_uart_tiocmset,
+	.install		= sdio_uart_install,
+	.cleanup		= sdio_uart_cleanup,
 	.proc_fops		= &sdio_uart_proc_fops,
 };
 
@@ -1095,6 +1132,7 @@ static int sdio_uart_probe(struct sdio_func *func,
 	port->func = func;
 	sdio_set_drvdata(func, port);
 	tty_port_init(&port->port);
+	port->port.ops = &sdio_uart_port_ops;
 
 	ret = sdio_uart_add_port(port);
 	if (ret) {

[PATCH 4/6] sdio_uart: Fix termios handling

[Alan Cox]

Switching between two non standard baud rates fails because of the cflag
test. Do as we did elsewhere and just kill the "optimisation".

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

 drivers/mmc/card/sdio_uart.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)


diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index fd00453..e611bc6 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -902,12 +902,6 @@ static void sdio_uart_set_termios(struct tty_struct *tty, struct ktermios *old_t
 	struct sdio_uart_port *port = tty->driver_data;
 	unsigned int cflag = tty->termios->c_cflag;
 
-#define RELEVANT_IFLAG(iflag)	((iflag) & (IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK))
-
-	if ((cflag ^ old_termios->c_cflag) == 0 &&
-	    RELEVANT_IFLAG(tty->termios->c_iflag ^ old_termios->c_iflag) == 0)
-		return;
-
 	if (sdio_uart_claim_func(port) != 0)
 		return;
 

[PATCH 5/6] sdio_uart: Use kfifo instead of the messy circ stuff

[Alan Cox]

Probably all the tty code should switch to this, especially when the new
lockless kfifo is merged.

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

 drivers/mmc/card/sdio_uart.c |   86 +++++++++++++-----------------------------
 1 files changed, 27 insertions(+), 59 deletions(-)


diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index e611bc6..c4fc27d 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -32,7 +32,7 @@
 #include <linux/mutex.h>
 #include <linux/seq_file.h>
 #include <linux/serial_reg.h>
-#include <linux/circ_buf.h>
+#include <linux/kfifo.h>
 #include <linux/gfp.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
@@ -46,18 +46,9 @@
 #define UART_NR		8	/* Number of UARTs this driver can handle */
 
 
-#define UART_XMIT_SIZE	PAGE_SIZE
+#define FIFO_SIZE	PAGE_SIZE
 #define WAKEUP_CHARS	256
 
-#define circ_empty(circ)	((circ)->head == (circ)->tail)
-#define circ_clear(circ)	((circ)->head = (circ)->tail = 0)
-
-#define circ_chars_pending(circ) \
-		(CIRC_CNT((circ)->head, (circ)->tail, UART_XMIT_SIZE))
-
-#define circ_chars_free(circ) \
-		(CIRC_SPACE((circ)->head, (circ)->tail, UART_XMIT_SIZE))
-
 
 struct uart_icount {
 	__u32	cts;
@@ -82,7 +73,7 @@ struct sdio_uart_port {
 	struct mutex		func_lock;
 	struct task_struct	*in_sdio_uart_irq;
 	unsigned int		regs_offset;
-	struct circ_buf		xmit;
+	struct kfifo		*xmit_fifo;
 	spinlock_t		write_lock;
 	struct uart_icount	icount;
 	unsigned int		uartclk;
@@ -104,6 +95,9 @@ static int sdio_uart_add_port(struct sdio_uart_port *port)
 	kref_init(&port->kref);
 	mutex_init(&port->func_lock);
 	spin_lock_init(&port->write_lock);
+	port->xmit_fifo = kfifo_alloc(FIFO_SIZE, GFP_KERNEL, &port->write_lock);
+	if (port->xmit_fifo == NULL)
+		return -ENOMEM;
 
 	spin_lock(&sdio_uart_table_lock);
 	for (index = 0; index < UART_NR; index++) {
@@ -453,9 +447,11 @@ static void sdio_uart_receive_chars(struct sdio_uart_port *port,
 
 static void sdio_uart_transmit_chars(struct sdio_uart_port *port)
 {
-	struct circ_buf *xmit = &port->xmit;
+	struct kfifo *xmit = port->xmit_fifo;
 	int count;
 	struct tty_struct *tty;
+	u8 iobuf[16];
+	int len;
 
 	if (port->x_char) {
 		sdio_out(port, UART_TX, port->x_char);
@@ -466,24 +462,21 @@ static void sdio_uart_transmit_chars(struct sdio_uart_port *port)
 
 	tty = tty_port_tty_get(&port->port);
 
-	if (tty == NULL || circ_empty(xmit) || tty->stopped || tty->hw_stopped) {
+	if (tty == NULL || !kfifo_len(xmit) || tty->stopped || tty->hw_stopped) {
 		sdio_uart_stop_tx(port);
 		return;
 	}
 
-	count = 16;
-	do {
-		sdio_out(port, UART_TX, xmit->buf[xmit->tail]);
-		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+	len = kfifo_get(xmit, iobuf, 16);
+	for (count = 0; count < len; count++) {
+		sdio_out(port, UART_TX, iobuf[count]);
 		port->icount.tx++;
-		if (circ_empty(xmit))
-			break;
-	} while (--count > 0);
+	}
 
-	if (circ_chars_pending(xmit) < WAKEUP_CHARS)
+	if (len < WAKEUP_CHARS)
 		tty_wakeup(tty);
 
-	if (circ_empty(xmit))
+	if (len == 0)
 		sdio_uart_stop_tx(port);
 	tty_kref_put(tty);
 }
@@ -606,22 +599,17 @@ static int sdio_uart_activate(struct tty_port *tport, struct tty_struct *tty)
 	 */
 	set_bit(TTY_IO_ERROR, &tty->flags);
 
-	/* Initialise and allocate the transmit buffer. */
-	page = __get_free_page(GFP_KERNEL);
-	if (!page)
-		return -ENOMEM;
-	port->xmit.buf = (unsigned char *)page;
-	circ_clear(&port->xmit);
+	kfifo_reset(port->xmit_fifo);
 
 	ret = sdio_uart_claim_func(port);
 	if (ret)
-		goto err1;
+		return ret;
 	ret = sdio_enable_func(port->func);
 	if (ret)
-		goto err2;
+		goto err1;
 	ret = sdio_claim_irq(port->func, sdio_uart_irq);
 	if (ret)
-		goto err3;
+		goto err2;
 
 	/*
 	 * Clear the FIFO buffers and disable them.
@@ -665,12 +653,10 @@ static int sdio_uart_activate(struct tty_port *tport, struct tty_struct *tty)
 	sdio_uart_release_func(port);
 	return 0;
 
-err3:
-	sdio_disable_func(port->func);
 err2:
-	sdio_uart_release_func(port);
+	sdio_disable_func(port->func);
 err1:
-	free_page((unsigned long)port->xmit.buf);
+	sdio_uart_release_func(port);
 	return ret;
 }
 
@@ -693,7 +679,7 @@ static void sdio_uart_shutdown(struct tty_port *tport)
 
 	ret = sdio_uart_claim_func(port);
 	if (ret)
-		goto skip;
+		return ret;
 
 	sdio_uart_stop_rx(port);
 
@@ -715,10 +701,6 @@ static void sdio_uart_shutdown(struct tty_port *tport)
 	sdio_disable_func(port->func);
 
 	sdio_uart_release_func(port);
-
-skip:
-	/* Free the transmit buffer page. */
-	free_page((unsigned long)port->xmit.buf);
 }
 
 /**
@@ -789,26 +771,12 @@ static int sdio_uart_write(struct tty_struct * tty, const unsigned char *buf,
 			   int count)
 {
 	struct sdio_uart_port *port = tty->driver_data;
-	struct circ_buf *circ = &port->xmit;
-	int c, ret = 0;
+	int ret;
 
 	if (!port->func)
 		return -ENODEV;
 
-	spin_lock(&port->write_lock);
-	while (1) {
-		c = CIRC_SPACE_TO_END(circ->head, circ->tail, UART_XMIT_SIZE);
-		if (count < c)
-			c = count;
-		if (c <= 0)
-			break;
-		memcpy(circ->buf + circ->head, buf, c);
-		circ->head = (circ->head + c) & (UART_XMIT_SIZE - 1);
-		buf += c;
-		count -= c;
-		ret += c;
-	}
-	spin_unlock(&port->write_lock);
+	ret = kfifo_put(port->xmit_fifo, buf, count);
 
 	if ( !(port->ier & UART_IER_THRI)) {
 		int err = sdio_uart_claim_func(port);
@@ -826,13 +794,13 @@ static int sdio_uart_write(struct tty_struct * tty, const unsigned char *buf,
 static int sdio_uart_write_room(struct tty_struct *tty)
 {
 	struct sdio_uart_port *port = tty->driver_data;
-	return port ? circ_chars_free(&port->xmit) : 0;
+	return FIFO_SIZE - kfifo_len(port->xmit_fifo);
 }
 
 static int sdio_uart_chars_in_buffer(struct tty_struct *tty)
 {
 	struct sdio_uart_port *port = tty->driver_data;
-	return port ? circ_chars_pending(&port->xmit) : 0;
+	return kfifo_len(port->xmit_fifo);
 }
 
 static void sdio_uart_send_xchar(struct tty_struct *tty, char ch)

[PATCH 6/6] sdio_uart: Style fixes

[Alan Cox]

Running the current code through checkpatch shows a few bits of noise
mostly but not entirely from before the changes.

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

 drivers/mmc/card/sdio_uart.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)


diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index c4fc27d..ff93f47 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -462,7 +462,8 @@ static void sdio_uart_transmit_chars(struct sdio_uart_port *port)
 
 	tty = tty_port_tty_get(&port->port);
 
-	if (tty == NULL || !kfifo_len(xmit) || tty->stopped || tty->hw_stopped) {
+	if (tty == NULL || !kfifo_len(xmit)
+				|| tty->stopped || tty->hw_stopped) {
 		sdio_uart_stop_tx(port);
 		return;
 	}
@@ -590,7 +591,6 @@ static int sdio_uart_activate(struct tty_port *tport, struct tty_struct *tty)
 {
 	struct sdio_uart_port *port =
 			container_of(tport, struct sdio_uart_port, port);
-	unsigned long page;
 	int ret;
 
 	/*
@@ -633,7 +633,7 @@ static int sdio_uart_activate(struct tty_port *tport, struct tty_struct *tty)
 	 */
 	sdio_out(port, UART_LCR, UART_LCR_WLEN8);
 
-	port->ier = UART_IER_RLSI | UART_IER_RDI | UART_IER_RTOIE | UART_IER_UUE;
+	port->ier = UART_IER_RLSI|UART_IER_RDI|UART_IER_RTOIE|UART_IER_UUE;
 	port->mctrl = TIOCM_OUT2;
 
 	sdio_uart_change_speed(port, tty->termios, NULL);
@@ -660,7 +660,6 @@ err1:
 	return ret;
 }
 
-	
 /**
  *	sdio_uart_shutdown	-	stop hardware
  *	@tport: tty port to shut down
@@ -675,11 +674,8 @@ static void sdio_uart_shutdown(struct tty_port *tport)
 {
 	struct sdio_uart_port *port =
 			container_of(tport, struct sdio_uart_port, port);
-	int ret;
-
-	ret = sdio_uart_claim_func(port);
-	if (ret)
-		return ret;
+	if (sdio_uart_claim_func(port))
+		return;
 
 	sdio_uart_stop_rx(port);
 
@@ -727,7 +723,6 @@ static int sdio_uart_install(struct tty_driver *driver, struct tty_struct *tty)
 	} else
 		sdio_uart_port_put(port);
 	return ret;
-		
 }
 
 /**
@@ -767,7 +762,7 @@ static void sdio_uart_hangup(struct tty_struct *tty)
 	tty_port_hangup(&port->port);
 }
 
-static int sdio_uart_write(struct tty_struct * tty, const unsigned char *buf,
+static int sdio_uart_write(struct tty_struct *tty, const unsigned char *buf,
 			   int count)
 {
 	struct sdio_uart_port *port = tty->driver_data;
@@ -778,7 +773,7 @@ static int sdio_uart_write(struct tty_struct * tty, const unsigned char *buf,
 
 	ret = kfifo_put(port->xmit_fifo, buf, count);
 
-	if ( !(port->ier & UART_IER_THRI)) {
+	if (!(port->ier & UART_IER_THRI)) {
 		int err = sdio_uart_claim_func(port);
 		if (!err) {
 			sdio_uart_start_tx(port);
@@ -865,7 +860,8 @@ static void sdio_uart_unthrottle(struct tty_struct *tty)
 	sdio_uart_release_func(port);
 }
 
-static void sdio_uart_set_termios(struct tty_struct *tty, struct ktermios *old_termios)
+static void sdio_uart_set_termios(struct tty_struct *tty,
+						struct ktermios *old_termios)
 {
 	struct sdio_uart_port *port = tty->driver_data;
 	unsigned int cflag = tty->termios->c_cflag;
@@ -944,7 +940,7 @@ static int sdio_uart_tiocmset(struct tty_struct *tty, struct file *file,
 	int result;
 
 	result = sdio_uart_claim_func(port);
-	if(!result) {
+	if (!result) {
 		sdio_uart_update_mctrl(port, set, clear);
 		sdio_uart_release_func(port);
 	}
@@ -962,7 +958,7 @@ static int sdio_uart_proc_show(struct seq_file *m, void *v)
 		struct sdio_uart_port *port = sdio_uart_port_get(i);
 		if (port) {
 			seq_printf(m, "%d: uart:SDIO", i);
-			if(capable(CAP_SYS_ADMIN)) {
+			if (capable(CAP_SYS_ADMIN)) {
 				seq_printf(m, " tx:%d rx:%d",
 					      port->icount.tx, port->icount.rx);
 				if (port->icount.frame)
@@ -1015,7 +1011,7 @@ static const struct tty_port_operations sdio_uart_port_ops = {
 	.shutdown = sdio_uart_shutdown,
 	.activate = sdio_uart_activate,
 };
-	
+
 static const struct tty_operations sdio_uart_ops = {
 	.open			= sdio_uart_open,
 	.close			= sdio_uart_close,
@@ -1068,7 +1064,7 @@ static int sdio_uart_probe(struct sdio_func *func,
 		}
 		if (!tpl) {
 			printk(KERN_WARNING
-			       "%s: can't find tuple 0x91 subtuple 0 (SUBTPL_SIOREG) for GPS class\n",
+       "%s: can't find tuple 0x91 subtuple 0 (SUBTPL_SIOREG) for GPS class\n",
 			       sdio_func_id(func));
 			kfree(port);
 			return -EINVAL;
@@ -1101,7 +1097,8 @@ static int sdio_uart_probe(struct sdio_func *func,
 		kfree(port);
 	} else {
 		struct device *dev;
-		dev = tty_register_device(sdio_uart_tty_driver, port->index, &func->dev);
+		dev = tty_register_device(sdio_uart_tty_driver,
+						port->index, &func->dev);
 		if (IS_ERR(dev)) {
 			sdio_uart_port_remove(port);
 			ret = PTR_ERR(dev);

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Nicolas Pitre]

On Mon, 2 Nov 2009, Alan Cox wrote:

> This sorts out the sdio uart handling of the tty layer. The existing code
> has lots of races and other fun bugs. Beat it into the current tty format for
> hotpluggable devices. The updates are intentionally modelled on Alan Stern's
> USB serial approach which is defintiely "best practice" right now. Also clean
> it up as we go and sort the circ stuff out.

Thanks.

> I don't have hardware to test this so hopefully someone out there does,
> otherwise given the nature of the bugs involved the driver will be progressed
> to BROKEN and removal.

I still have the hardware.  Even tested it recently with a new SDIO host 
controller and it "worked just fine".  Hardware is a GPS receiver so 
admitedly nothing that would push this driver into corner cases.

However I have problems applying your patches.  Most of them require 
fuzzy patching to apply, and one of them even doesn't apply that way.  
What is your base tree?


Nicolas

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Alan Cox]

> However I have problems applying your patches.  Most of them require 
> fuzzy patching to apply, and one of them even doesn't apply that way.  
> What is your base tree?

Linux-next, but there is a patch I posted afterwards that goes before the
sdio_uart patches. Want me to send you a set with all the patches
including the other one ?

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Nicolas Pitre]

On Mon, 2 Nov 2009, Alan Cox wrote:

> > However I have problems applying your patches.  Most of them require 
> > fuzzy patching to apply, and one of them even doesn't apply that way.  
> > What is your base tree?
> 
> Linux-next, but there is a patch I posted afterwards that goes before the
> sdio_uart patches.

Yep, picked it as well.

> Want me to send you a set with all the patches
> including the other one ?

It all applied fine now on top of linux-next, with these warnings:

Applying: sdio_uart: Switch to the open/close helpers
/home/nico/kernels/linux-2.6/.git/rebase-apply/patch:90: trailing whitespace.
/home/nico/kernels/linux-2.6/.git/rebase-apply/patch:156: trailing whitespace.
/home/nico/kernels/linux-2.6/.git/rebase-apply/patch:260: trailing whitespace.
warning: 3 lines add whitespace errors.

Now... testing reveals that the very first patch "sdio_uart: use 
tty_port" causes a segmentation fault in sdio_uart_open():

Unable to handle kernel NULL pointer dereference at virtual address 00000084
pgd = dfb44000 [00000084] *pgd=1fb99031, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT
last sysfs file: 
/sys/devices/platform/mvsdio/mmc_host/mmc0/mmc0:f111/uevent
Modules linked in:
CPU: 0    Not tainted  (2.6.32-rc5-next-20091102-00001-gb36eae9 #10)
PC is at sdio_uart_open+0x204/0x2cc
[...]

This is fixed by this patch:

diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index c2759db..671fe5e 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -608,7 +608,7 @@ static int sdio_uart_startup(struct sdio_uart_port *port)
 		if (!(sdio_uart_get_mctrl(port) & TIOCM_CTS))
 			tty->hw_stopped = 1;
 
-	clear_bit(TTY_IO_ERROR, &port->tty->flags);
+	clear_bit(TTY_IO_ERROR, &tty->flags);
 
 	/* Kick the IRQ handler once while we're still holding the host lock */
 	sdio_uart_irq(port->func);

With this folded in, the card does work with the full series applied.  
However the kernel is now crashing when the card is pulled out while 
some process is reading from the device.  This used to behave well 
before. I don't have time to investigate that one right now though.


Nicolas

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Alan Cox]

> However the kernel is now crashing when the card is pulled out while 
> some process is reading from the device.  This used to behave well 
> before. I don't have time to investigate that one right now though.

That usually means I've goofed on the ref counting. I'll add your patch
and review the code closely.

Thanks
Alan

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Nicolas Pitre]

On Mon, 2 Nov 2009, Nicolas Pitre wrote:

> With this folded in, the card does work with the full series applied.  
> However the kernel is now crashing when the card is pulled out while 
> some process is reading from the device.  This used to behave well 
> before. I don't have time to investigate that one right now though.

OK, here more data points:

My test is to insert GPS card and do a 'cat /dev/ttySDIO0' and watch 
NMEA data scrolling by.

With "sdio_uart: Move the open lock" applied, I can pull the card out 
and the cat process terminates.  However inserting the card back and 
repeating the test again always gives me:

	cat: /dev/ttySDIO0: Input/output error

until I reboot.  Same thing happens if I terminates cat with ^C and 
start it again.

With "sdio_uart: Switch to the open/close helpers" applied, things got 
even worse.  Interrupting cat with ^C still gives the same result as 
above.  But yanking the card out when cat is still running doesn't 
terminate cat unless I ^C it at which point the kernel crashes with a 
BUG().  The backtrace is:

[<c0029c88>] (__bug+0x18/0x24) from [<c02665d0>] (sdio_writeb+0x1c/0x4c)
[<c02665d0>] (sdio_writeb+0x1c/0x4c) from [<c01b4098>] (tty_port_lower_dtr_rts+0x1c/0x20)
[<c01b4098>] (tty_port_lower_dtr_rts+0x1c/0x20) from [<c01b412c>] (tty_port_close_end+0x2c/0x130)
[<c01b412c>] (tty_port_close_end+0x2c/0x130) from [<c01b49a8>] (tty_port_close+0x2c/0x3c)
[<c01b49a8>] (tty_port_close+0x2c/0x3c) from [<c01ae0c4>] (tty_release_dev+0x17c/0x478)
[<c01ae0c4>] (tty_release_dev+0x17c/0x478) from [<c01ae3e8>] (tty_release+0x28/0x4c)
[<c01ae3e8>] (tty_release+0x28/0x4c) from [<c00b5bf0>] (__fput+0x118/0x210)
[<c00b5bf0>] (__fput+0x118/0x210) from [<c00b28b4>] (filp_close+0x70/0x7c)
[<c00b28b4>] (filp_close+0x70/0x7c) from [<c003d0e0>] (put_files_struct+0x80/0xd4)
[<c003d0e0>] (put_files_struct+0x80/0xd4) from [<c003eac0>] (do_exit+0x220/0x714)
[<c003eac0>] (do_exit+0x220/0x714) from [<c003f074>] (do_group_exit+0xc0/0xf4)
[<c003f074>] (do_group_exit+0xc0/0xf4) from [<c004d8d0>] (get_signal_to_deliver+0x3c8/0x428)
[...] (impressive nesting culled)

Only after backing out those 2 patches as wellas "sdio_uart: refcount 
the tty objects" is the correct behavior restored.


Nicolas



[<c004d8d0>] (get_signal_to_deliver+0x3c8/0x428) from [<c0028b34>] (do_notify_resume+0x5c/0x5d0)
[<c0028b34>] (do_notify_resume+0x5c/0x5d0) from [<c0026a88>] (work_pending+0x1c/0x20)





> 
> 
> Nicolas
> 

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Alan Cox]

> With this folded in, the card does work with the full series applied.  
> However the kernel is now crashing when the card is pulled out while 
> some process is reading from the device.  This used to behave well 
> before. I don't have time to investigate that one right now though.

Going through it I found one assumption in the tty_port code that wanted
fixing. We would release the port and then try to change the modem lines.

The dtr_rts method didn't claim the function which in conjunction with
that made it crash.

Also the hangup checked your old port->opened which now never got set.

I am scratching my head over some of the other stuff I found however. In
particular port->func can be set to NULL when the device is removed.

The claim method takes the mutex, checks if it is NULL and acts
accordingly but it releases the mutex, which makes it useless as the code
then uses port->func. If I move the release of the mutex to the
release_func method then that fixes almost all cases.

The one I'm stuck on is this


		CPU1				CPU2


	sdio_uart_irq
					sdio_uart_port_remove
					port->func = NULL;
	sdio_in
	BUG_ON


I'm not sure what the required rules on the irq disable/remove are meant
to be here ?

Alan

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[David Vrabel]

Alan Cox wrote:
> 
> The claim method takes the mutex, checks if it is NULL and acts
> accordingly but it releases the mutex, which makes it useless as the code
> then uses port->func. If I move the release of the mutex to the
> release_func method then that fixes almost all cases.
> 
> The one I'm stuck on is this
> 
> 
> 		CPU1				CPU2
> 
> 
> 	sdio_uart_irq
> 					sdio_uart_port_remove
> 					port->func = NULL;
> 	sdio_in
> 	BUG_ON

This is actually happening?  sdio_claim_host()/sdio_release_host() act
like a mutex so sdio_uart_port_remove() will wait in sdio_claim_host()
until sdio_uart_irq() returns (SDIO interrupt handlers are called with
the host claimed).

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: [PATCH 0/6] Clean up the sdio_uart driver and fix the tty code

[Alan Cox]

> > 	sdio_uart_irq
> > 					sdio_uart_port_remove
> > 					port->func = NULL;
> > 	sdio_in
> > 	BUG_ON
> 
> This is actually happening?  sdio_claim_host()/sdio_release_host() act

Found by inspection

> like a mutex so sdio_uart_port_remove() will wait in sdio_claim_host()
> until sdio_uart_irq() returns (SDIO interrupt handlers are called with
> the host claimed).

Ok that was a detail I was missing. That part of the locking now makes
sense.

Ok so I think I have it fixed up barring stuff which is "feature add" -
such as implementing TIOCMIWAIT and blocking on no carrier.