[PATCH 0/3] serial: 8250: fix racy uartclk update

[PATCH 0/3] serial: 8250: fix racy uartclk update

[Johan Hovold]

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(-)

-- 
2.32.0

[PATCH 1/3] serial: 8250: fix racy uartclk update

[Johan Hovold]

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: stable@vger.kernel.org      # 5.9
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/8250/8250_port.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 66374704747e..e4dd82fd7c2a 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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);
 
-- 
2.32.0

[PATCH 2/3] serial: 8250: rename unlock labels

[Johan Hovold]

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 <johan@kernel.org>
---
 drivers/tty/serial/8250/8250_port.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e4dd82fd7c2a..5775cbff8f6e 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -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);
-- 
2.32.0

[PATCH 3/3] serial: 8250_dw: drop bogus uartclk optimisation

[Johan Hovold]

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 <johan@kernel.org>
---
 drivers/tty/serial/8250/8250_dw.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a3a0154da567..52e03dd0eb3b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -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);
 
-- 
2.32.0

Re: [PATCH 1/3] serial: 8250: fix racy uartclk update

[Andy Shevchenko]

On Fri, Oct 15, 2021 at 01:14:20PM +0200, Johan Hovold wrote:
> 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.

Nice catch!
Thanks, Johan, for fixing this!
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 868f3ee6e452 ("serial: 8250: Add 8250 port clock update method")
> Fixes: c8dff3aa8241 ("serial: 8250: Skip uninitialized TTY port baud rate update")
> Cc: stable@vger.kernel.org      # 5.9
> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 66374704747e..e4dd82fd7c2a 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -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);
>  
> -- 
> 2.32.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] serial: 8250: rename unlock labels

[Andy Shevchenko]

On Fri, Oct 15, 2021 at 01:14:21PM +0200, Johan Hovold wrote:
> 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.

I'm on the same page with the label naming schema, hence
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e4dd82fd7c2a..5775cbff8f6e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -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);
> -- 
> 2.32.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] serial: 8250_dw: drop bogus uartclk optimisation

[Andy Shevchenko]

On Fri, Oct 15, 2021 at 01:14:22PM +0200, Johan Hovold wrote:
> 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.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index a3a0154da567..52e03dd0eb3b 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -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);
>  
> -- 
> 2.32.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] serial: 8250: fix racy uartclk update

[Serge Semin]

On Fri, Oct 15, 2021 at 01:14:19PM +0200, Johan Hovold wrote:
> 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

> 
> 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(-)
> 
> -- 
> 2.32.0
> 

Re: [PATCH 0/3] serial: 8250: fix racy uartclk update

[Serge Semin]

Hello Johan

On Fri, Oct 15, 2021 at 10:10:00PM +0300, Serge Semin wrote:
> On Fri, Oct 15, 2021 at 01:14:19PM +0200, Johan Hovold wrote:
> > 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

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 <fancer.lancer@gmail.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>

-Sergey

> 
> > 
> > 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(-)
> > 
> > -- 
> > 2.32.0
> > 

Re: [PATCH 2/3] serial: 8250: rename unlock labels

[Serge Semin]

On Fri, Oct 15, 2021 at 01:14:21PM +0200, Johan Hovold wrote:
> 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.

Initially I had the same thought in mind when was adding the update
clock method because normally I add the GOTO-target-related suffix for
the cleanup path. But in this case I decided to stick with the locally
selected naming scheme, since it also made sense in the meaning: "goto
Out_of_the_Lock". So in case of the autoconfig() method leaving the
"out_lock" label name was reasonable because the code program counter
jumps out of the locked section of the function while the "goto out"
jumps to out of the method itself.

-Serge

> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/tty/serial/8250/8250_port.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e4dd82fd7c2a..5775cbff8f6e 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -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);
> -- 
> 2.32.0
> 

Re: [PATCH 0/3] serial: 8250: fix racy uartclk update

[Johan Hovold]

On Sat, Oct 16, 2021 at 06:25:21PM +0300, Serge Semin wrote:

> 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 <fancer.lancer@gmail.com>
> Tested-by: Serge Semin <fancer.lancer@gmail.com>

Thanks for testing.

Johan