Should .dtr_rts do tty_port_tty_get?

Hi,

I'm playing with a serial device and got a nice oops. Maybe after some weird stty's, I don't know. But it dies in uart_dtr_rts:

        .loc 1 1535 0
        movq    (%rbx), %r13    # port_1(D)->tty, D.26746
...
        .loc 1 1494 0
        testb   $2, 224(%r13)   #, D.26746_10->flags
                    ^^^^^^^^^
                      HERE

Because r13 (port->tty) is NULL. So the question is about the principle. Should it call tty_port_tty_get or is it a bug in the TTY layer and uart_dtr_rts should not be called with port->tty == NULL?

I'll attach a patch which does the former as a reply to this message.

BTW only serial_core needs tty in dtr_rts.

thanks,

js
suse labs

Under certain circumstances uart_dtr_rts might cause an oops. It dies because port->tty is NULL. To fix this, let's take a reference of port->tty by tty_port_tty_get. And if it is not there already, fail gracefully.

After all, indeed drop the reference.

---
 drivers/tty/serial/serial_core.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

index d6e7240..88a0472 100644
@@ -1531,8 +1531,12 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
 		 * If this is the first open to succeed,
 		 * adjust things to suit.
 		 */
-		if (!test_and_set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags))
-			uart_update_termios(port->tty, state);
+		if (!test_and_set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags)) {
+			struct tty_struct *tty = tty_port_tty_get(port);
+			if (tty)
+				uart_update_termios(tty, state);
+			tty_kref_put(tty);
+		}
 	}
 	else
 		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);

1.7.4.1

The uart helper layer assumes here (and a couple of other spots) that the IRQ handler for the tty takes the port lock.

I think the *right* fix is probably to pass port not port->tty into the helper in the first place but that seems to ripple into a lot of drivers.

(passing port->tty to things that then go tty->port is braindead and causes half the mess in the tty/serial code)

The oopsing path is through open BTW:
-> uart_open
-> tty_port_block_til_ready
-> tty_port_raise_dtr_rts
-> uart_dtr_rts

I seem to miss the point. uart_update_termios needs tty, not port.

thanks,

js
suse labs

Actually I think it needs shooting having looked more closely

It does 3 things

1. It copies a flag across as part of a console hack. That should be done elsewhere - eg in uart_startup

2. It sets the speed, which was already done by uart_startup

3. It goes poking around in the CBAUD flag before playing with DTR and RTS which will have no effect as we *already* set the flags in uart_dtr_rts

Am I missing something ?

Hmm, looking more closely, it's broken and should be converted to tty_port in full. I'll take a look if I can do something about that.

Perhaps.

Note that uart_startup is called with init_hw = 0 from uart_open. So the speed is not set yet. So we should set the speed in uart_startup unconditionally, I guess.

ACK

.dtr_rts did only dtr_rts when added by you. Then Arnd moved uart_update_termios into that in 3f582b8c11.

I'll post a patch as a short-term fix shortly.

thanks,

js
suse labs

In .dtr_rts we do:
uart_set_mctrl(uport, TIOCM_DTR | TIOCM_RTS)
and call uart_update_termios. It does:
uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS)
once again. As the only callsite of uart_update_termios is .dtr_rts, remove the uart_set_mctrl from uart_update_termios to not set it twice.

---
 drivers/tty/serial/serial_core.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

index 47657cf..3aae8ed 100644
@@ -1483,20 +1483,6 @@ static void uart_hangup(struct tty_struct *tty)
 static void uart_update_termios(struct tty_struct *tty,
 						struct uart_state *state)
 {
-	struct uart_port *port = state->uart_port;
-
-	/*
-	 * If the device failed to grab its irq resources,
-	 * or some other error occurred, don't try to talk
-	 * to the port hardware.
-	 */
-	if (!(tty->flags & (1 << TTY_IO_ERROR))) {
-		/*
-		 * And finally enable the RTS and DTR signals.
-		 */
-		if (tty->termios->c_cflag & CBAUD)
-			uart_set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
-	}
 }
 
 static int uart_carrier_raised(struct tty_port *port)

1.7.4.1

Now, uart_update_termios is empty, so it's time to remove it. We no longer need a live tty in .dtr_rts. So this should prune all the bugs where tty is zeroed in port->tty during tty_port_block_til_ready.

There is one thing to note. We don't set ASYNC_NORMAL_ACTIVE now. It's because this is done already in tty_port_block_til_ready.

---
 drivers/tty/serial/serial_core.c |   25 +------------------------
 1 files changed, 1 insertions(+), 24 deletions(-)

index 3aae8ed..d4bd465 100644
@@ -1470,21 +1470,6 @@ static void uart_hangup(struct tty_struct *tty)
 	mutex_unlock(&port->mutex);
 }
 
-/**
- *	uart_update_termios	-	update the terminal hw settings
- *	@tty: tty associated with UART
- *	@state: UART to update
- *
- *	Copy across the serial console cflag setting into the termios settings
- *	for the initial open of the port.  This allows continuity between the
- *	kernel settings, and the settings init adopts when it opens the port
- *	for the first time.
- */
-static void uart_update_termios(struct tty_struct *tty,
-						struct uart_state *state)
-{
-}
-
 static int uart_carrier_raised(struct tty_port *port)
 {
 	struct uart_state *state = container_of(port, struct uart_state, port);

@@ -1504,16 +1489,8 @@ static void uart_dtr_rts(struct tty_port *port, int onoff)
 	struct uart_state *state = container_of(port, struct uart_state, port);
 	struct uart_port *uport = state->uart_port;
 
-	if (onoff) {
+	if (onoff)
 		uart_set_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
-
-		/*
-		 * If this is the first open to succeed,
-		 * adjust things to suit.
-		 */
-		if (!test_and_set_bit(ASYNCB_NORMAL_ACTIVE, &port->flags))
-			uart_update_termios(port->tty, state);
-	}
 	else
 		uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
 }

1.7.4.1

We should not fiddle with speed and cflags in .dtr_rts hook. Actually we might not have tty at that moment already.

So move the console cflag copy and speed setup into uart_startup. Actually the speed setup is already there, but we need to call it unconditionally (uart_startup is called from uart_open with hw_init = 0).

This means we move uart_change_speed before dtr/rts setup in .dtr_rts. But this should not matter as the setup should be called after uart_change_speed anyway.

Before:                             After:
dtr/rts setup (dtr_rts)             uart_change_speed (startup)
uart_change_speed (update_termios)  dtr/rts setup (dtr_rts)
dtr/rts setup (update_termios)      dtr/rts setup (update_termios)

The second setup will dismiss with the next patch.

---
 drivers/tty/serial/serial_core.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

index d6e7240..47657cf 100644
@@ -172,12 +172,16 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state, int in
 
 	retval = uport->ops->startup(uport);
 	if (retval == 0) {
-		if (init_hw) {
-			/*
-			 * Initialise the hardware port settings.
-			 */
-			uart_change_speed(tty, state, NULL);
+		if (uart_console(uport) && uport->cons->cflag) {
+			tty->termios->c_cflag = uport->cons->cflag;
+			uport->cons->cflag = 0;
+		}
+		/*
+		 * Initialise the hardware port settings.
+		 */
+		uart_change_speed(tty, state, NULL);
 
+		if (init_hw) {
 			/*
 			 * Setup the RTS and DTR signals once the
 			 * port is open and ready to respond.

@@ -1481,11 +1485,6 @@ static void uart_update_termios(struct tty_struct *tty,
 {
 	struct uart_port *port = state->uart_port;
 
-	if (uart_console(port) && port->cons->cflag) {
-		tty->termios->c_cflag = port->cons->cflag;
-		port->cons->cflag = 0;
-	}
-
 	/*
 	 * If the device failed to grab its irq resources,
 	 * or some other error occurred, don't try to talk

@@ -1493,11 +1492,6 @@ static void uart_update_termios(struct tty_struct *tty,
 	 */
 	if (!(tty->flags & (1 << TTY_IO_ERROR))) {
 		/*
-		 * Make termios settings take effect.
-		 */
-		uart_change_speed(tty, state, NULL);
-
-		/*
 		 * And finally enable the RTS and DTR signals.
 		 */
 		if (tty->termios->c_cflag & CBAUD)

1.7.4.1