serial: 8250: fix racy uartclk update

Commit 868f3ee6e452 ("serial: 8250: Add 8250 port clock update method") added a hack to support SoCs where the UART reference clock can change behind the back of the driver but failed to add the proper locking.

First, make sure to take a reference to the tty struct to avoid dereferencing a NULL pointer if the clock change races with a hangup.

Second, the termios semaphore must be held during the update to prevent a racing termios change.

Fixes 868f3ee6e452 ("serial: 8250: Add 8250 port clock update method")

Fixes c8dff3aa8241 ("serial: 8250: Skip uninitialized TTY port baud rate update")

Cc [email protected] # 5.9

Cc Serge Semin <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

drivers/tty/serial/8250/8250_port.c +17 -4

index 66374704747e..e4dd82fd7c2a 100644
@@ -2696,21 +2696,32 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port,
 void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
+	struct tty_port *tport = &port->state->port;
 	unsigned int baud, quot, frac = 0;
 	struct ktermios *termios;
+	struct tty_struct *tty;
 	unsigned long flags;
 
-	mutex_lock(&port->state->port.mutex);
+	tty = tty_port_tty_get(tport);
+	if (!tty) {
+		mutex_lock(&tport->mutex);
+		port->uartclk = uartclk;
+		mutex_unlock(&tport->mutex);
+		return;
+	}
+
+	down_write(&tty->termios_rwsem);
+	mutex_lock(&tport->mutex);
 
 	if (port->uartclk == uartclk)
 		goto out_lock;
 
 	port->uartclk = uartclk;
 
-	if (!tty_port_initialized(&port->state->port))
+	if (!tty_port_initialized(tport))
 		goto out_lock;
 
-	termios = &port->state->port.tty->termios;
+	termios = &tty->termios;
 
 	baud = serial8250_get_baud_rate(port, termios, NULL);
 	quot = serial8250_get_divisor(port, baud, &frac);

@@ -2727,7 +2738,9 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 	serial8250_rpm_put(up);
 
 out_lock:
-	mutex_unlock(&port->state->port.mutex);
+	mutex_unlock(&tport->mutex);
+	up_write(&tty->termios_rwsem);
+	tty_kref_put(tty);
 }
 EXPORT_SYMBOL_GPL(serial8250_update_uartclk);
 

Signature · 1 line

2.32.0

Rename a couple of oddly named labels that are used to unlock before returning after what they do (rather than after the context they are used in) to improve readability.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/tty/serial/8250/8250_port.c +5 -5

index e4dd82fd7c2a..5775cbff8f6e 100644
@@ -1338,7 +1338,7 @@ static void autoconfig(struct uart_8250_port *up)
 	up->tx_loadsz = uart_config[port->type].tx_loadsz;
 
 	if (port->type == PORT_UNKNOWN)
-		goto out_lock;
+		goto out_unlock;
 
 	/*
 	 * Reset the UART.

@@ -1355,7 +1355,7 @@ static void autoconfig(struct uart_8250_port *up)
 	else
 		serial_out(up, UART_IER, 0);
 
-out_lock:
+out_unlock:
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	/*

@@ -2714,12 +2714,12 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 	mutex_lock(&tport->mutex);
 
 	if (port->uartclk == uartclk)
-		goto out_lock;
+		goto out_unlock;
 
 	port->uartclk = uartclk;
 
 	if (!tty_port_initialized(tport))
-		goto out_lock;
+		goto out_unlock;
 
 	termios = &tty->termios;
 

@@ -2737,7 +2737,7 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 	spin_unlock_irqrestore(&port->lock, flags);
 	serial8250_rpm_put(up);
 
-out_lock:
+out_unlock:
 	mutex_unlock(&tport->mutex);
 	up_write(&tty->termios_rwsem);
 	tty_kref_put(tty);

Signature · 1 line

2.32.0

Rename a couple of oddly named labels that are used to unlock before returning after what they do (rather than after the context they are used in) to improve readability.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/tty/serial/8250/8250_port.c +5 -5

index e4dd82fd7c2a..5775cbff8f6e 100644
@@ -1338,7 +1338,7 @@ static void autoconfig(struct uart_8250_port *up)
 	up->tx_loadsz = uart_config[port->type].tx_loadsz;
 
 	if (port->type == PORT_UNKNOWN)
-		goto out_lock;
+		goto out_unlock;
 
 	/*
 	 * Reset the UART.

@@ -1355,7 +1355,7 @@ static void autoconfig(struct uart_8250_port *up)
 	else
 		serial_out(up, UART_IER, 0);
 
-out_lock:
+out_unlock:
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	/*

@@ -2714,12 +2714,12 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 	mutex_lock(&tport->mutex);
 
 	if (port->uartclk == uartclk)
-		goto out_lock;
+		goto out_unlock;
 
 	port->uartclk = uartclk;
 
 	if (!tty_port_initialized(tport))
-		goto out_lock;
+		goto out_unlock;
 
 	termios = &tty->termios;
 

@@ -2737,7 +2737,7 @@ void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk)
 	spin_unlock_irqrestore(&port->lock, flags);
 	serial8250_rpm_put(up);
 
-out_lock:
+out_unlock:
 	mutex_unlock(&tport->mutex);
 	up_write(&tty->termios_rwsem);
 	tty_kref_put(tty);

Signature · 1 line

2.32.0

The driver was updating the port uartclk before setting the new rate in an attempt to avoid having the clock notifier redundantly update the divisors.

The set_termios() callback is however called under the termios semaphore and tty-port mutex so the worker scheduled by the clock notifier will block in serial8250_update_uartclk() until the uartclk and divisors have been updated anyway.

Drop the unnecessary swaps and incorrect comment and simply update the uartclk field if the clock-rate change was successful.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serial/8250/8250_dw.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/tty/serial/8250/8250_dw.c +4 -7

index a3a0154da567..52e03dd0eb3b 100644
@@ -338,15 +338,12 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	rate = clk_round_rate(d->clk, newrate);
 	if (rate > 0) {
 		/*
-		 * Premilinary set the uartclk to the new clock rate so the
-		 * clock update event handler caused by the clk_set_rate()
-		 * calling wouldn't actually update the UART divisor since
-		 * we about to do this anyway.
+		 * Note that any clock-notifer worker will block in
+		 * serial8250_update_uartclk() until we are done.
 		 */
-		swap(p->uartclk, rate);
 		ret = clk_set_rate(d->clk, newrate);
-		if (ret)
-			swap(p->uartclk, rate);
+		if (!ret)
+			p->uartclk = rate;
 	}
 	clk_prepare_enable(d->clk);
 

Signature · 1 line

2.32.0

Here's a fix for the asynchronous uartclk-update hack which was added last year and a couple of related clean ups.

Johan

Johan Hovold (3):
serial: 8250: fix racy uartclk update
serial: 8250: rename unlock labels
serial: 8250_dw: drop bogus uartclk optimisation

 drivers/tty/serial/8250/8250_dw.c   | 11 ++++------
 drivers/tty/serial/8250/8250_port.c | 31 ++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 16 deletions(-)

Signature · 1 line

2.32.0

Quoted 2 lines · Johan Hovold

Here's a fix for the asynchronous uartclk-update hack which was added last year and a couple of related clean ups.

Yeah, that was very unfortunate patchset. Thank you very much for the fix. I'll test it out tomorrow.

-Sergey

Quoted 17 lines

Johan

Johan Hovold (3):
serial: 8250: fix racy uartclk update
serial: 8250: rename unlock labels
serial: 8250_dw: drop bogus uartclk optimisation

 drivers/tty/serial/8250/8250_dw.c   | 11 ++++------
 drivers/tty/serial/8250/8250_port.c | 31 ++++++++++++++++++++---------
 2 files changed, 26 insertions(+), 16 deletions(-)

Signature · 1 line

2.32.0

The series has been tested on Baikal-T1 SoC with two UART ports feed with a common reference clock. It's working well. Thanks again for fixing the problem. For the whole series

Reviewed-by Serge Semin <[email protected]>

Tested-by Serge Semin <[email protected]>