[PATCH AUTOSEL 4.4 27/29] serial: pl010: Drop CR register reset on set_termios

[PATCH AUTOSEL 4.4 27/29] serial: pl010: Drop CR register reset on set_termios

[Sasha Levin]

From: Lukas Wunner <lukas@wunner.de>

[ Upstream commit 08a0c6dff91c965e39905cf200d22db989203ccb ]

pl010_set_termios() briefly resets the CR register to zero.

Where does this register write come from?

The PL010 driver's IRQ handler ambauart_int() originally modified the CR
register without holding the port spinlock.  ambauart_set_termios() also
modified that register.  To prevent concurrent read-modify-writes by the
IRQ handler and to prevent transmission while changing baudrate,
ambauart_set_termios() had to disable interrupts.  That is achieved by
writing zero to the CR register.

However in 2004 the PL010 driver was amended to acquire the port
spinlock in the IRQ handler, obviating the need to disable interrupts in
->set_termios():
https://git.kernel.org/history/history/c/157c0342e591

That rendered the CR register write obsolete.  Drop it.

Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Link: https://lore.kernel.org/r/fcaff16e5b1abb4cc3da5a2879ac13f278b99ed0.1641128728.git.lukas@wunner.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/tty/serial/amba-pl010.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
index 5d41d5b92619a..7f4ba92739663 100644
--- a/drivers/tty/serial/amba-pl010.c
+++ b/drivers/tty/serial/amba-pl010.c
@@ -465,14 +465,11 @@ pl010_set_termios(struct uart_port *port, struct ktermios *termios,
 	if ((termios->c_cflag & CREAD) == 0)
 		uap->port.ignore_status_mask |= UART_DUMMY_RSR_RX;
 
-	/* first, disable everything */
 	old_cr = readb(uap->port.membase + UART010_CR) & ~UART010_CR_MSIE;
 
 	if (UART_ENABLE_MS(port, termios->c_cflag))
 		old_cr |= UART010_CR_MSIE;
 
-	writel(0, uap->port.membase + UART010_CR);
-
 	/* Set baud rate */
 	quot -= 1;
 	writel((quot & 0xf00) >> 8, uap->port.membase + UART010_LCRM);
-- 
2.34.1

Re: [PATCH AUTOSEL 4.4 27/29] serial: pl010: Drop CR register reset on set_termios

[Lukas Wunner]

On Mon, Jan 17, 2022 at 10:08:20PM -0500, Sasha Levin wrote:
> From: Lukas Wunner <lukas@wunner.de>
> 
> [ Upstream commit 08a0c6dff91c965e39905cf200d22db989203ccb ]
> 
> pl010_set_termios() briefly resets the CR register to zero.
> 
> Where does this register write come from?
> 
> The PL010 driver's IRQ handler ambauart_int() originally modified the CR
> register without holding the port spinlock.  ambauart_set_termios() also
> modified that register.  To prevent concurrent read-modify-writes by the
> IRQ handler and to prevent transmission while changing baudrate,
> ambauart_set_termios() had to disable interrupts.  That is achieved by
> writing zero to the CR register.
> 
> However in 2004 the PL010 driver was amended to acquire the port
> spinlock in the IRQ handler, obviating the need to disable interrupts in
> ->set_termios():
> https://git.kernel.org/history/history/c/157c0342e591
> 
> That rendered the CR register write obsolete.  Drop it.

I'd recommend against backporting this particular patch for pl010
as it's merely a cleanup that eases future work on the driver,
but doesn't actually fix anything.

You've also auto-selected a patch for the pl011 driver with the
same subject.  That other patch *does* actually fix an rs485
Transmit Enable glitch, so backporting it makes sense.

Thanks,

Lukas

> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Link: https://lore.kernel.org/r/fcaff16e5b1abb4cc3da5a2879ac13f278b99ed0.1641128728.git.lukas@wunner.de
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/tty/serial/amba-pl010.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c
> index 5d41d5b92619a..7f4ba92739663 100644
> --- a/drivers/tty/serial/amba-pl010.c
> +++ b/drivers/tty/serial/amba-pl010.c
> @@ -465,14 +465,11 @@ pl010_set_termios(struct uart_port *port, struct ktermios *termios,
>  	if ((termios->c_cflag & CREAD) == 0)
>  		uap->port.ignore_status_mask |= UART_DUMMY_RSR_RX;
>  
> -	/* first, disable everything */
>  	old_cr = readb(uap->port.membase + UART010_CR) & ~UART010_CR_MSIE;
>  
>  	if (UART_ENABLE_MS(port, termios->c_cflag))
>  		old_cr |= UART010_CR_MSIE;
>  
> -	writel(0, uap->port.membase + UART010_CR);
> -
>  	/* Set baud rate */
>  	quot -= 1;
>  	writel((quot & 0xf00) >> 8, uap->port.membase + UART010_LCRM);
> -- 
> 2.34.1

[PATCH AUTOSEL 4.4 28/29] serial: core: Keep mctrl register state and cached copy in sync

[Sasha Levin]

From: Lukas Wunner <lukas@wunner.de>

[ Upstream commit 93a770b7e16772530196674ffc79bb13fa927dc6 ]

struct uart_port contains a cached copy of the Modem Control signals.
It is used to skip register writes in uart_update_mctrl() if the new
signal state equals the old signal state.  It also avoids a register
read to obtain the current state of output signals.

When a uart_port is registered, uart_configure_port() changes signal
state but neglects to keep the cached copy in sync.  That may cause
a subsequent register write to be incorrectly skipped.  Fix it before
it trips somebody up.

This behavior has been present ever since the serial core was introduced
in 2002:
https://git.kernel.org/history/history/c/33c0d1b0c3eb

So far it was never an issue because the cached copy is initialized to 0
by kzalloc() and when uart_configure_port() is executed, at most DTR has
been set by uart_set_options() or sunsu_console_setup().  Therefore,
a stable designation seems unnecessary.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Link: https://lore.kernel.org/r/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/tty/serial/serial_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 013fb874c64e2..8142135a2eec4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2247,7 +2247,8 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		 * We probably don't need a spinlock around this, but
 		 */
 		spin_lock_irqsave(&port->lock, flags);
-		port->ops->set_mctrl(port, port->mctrl & TIOCM_DTR);
+		port->mctrl &= TIOCM_DTR;
+		port->ops->set_mctrl(port, port->mctrl);
 		spin_unlock_irqrestore(&port->lock, flags);
 
 		/*
-- 
2.34.1

Re: [PATCH AUTOSEL 4.4 28/29] serial: core: Keep mctrl register state and cached copy in sync

[Greg Kroah-Hartman]

On Mon, Jan 17, 2022 at 10:08:21PM -0500, Sasha Levin wrote:
> From: Lukas Wunner <lukas@wunner.de>
> 
> [ Upstream commit 93a770b7e16772530196674ffc79bb13fa927dc6 ]
> 
> struct uart_port contains a cached copy of the Modem Control signals.
> It is used to skip register writes in uart_update_mctrl() if the new
> signal state equals the old signal state.  It also avoids a register
> read to obtain the current state of output signals.
> 
> When a uart_port is registered, uart_configure_port() changes signal
> state but neglects to keep the cached copy in sync.  That may cause
> a subsequent register write to be incorrectly skipped.  Fix it before
> it trips somebody up.
> 
> This behavior has been present ever since the serial core was introduced
> in 2002:
> https://git.kernel.org/history/history/c/33c0d1b0c3eb
> 
> So far it was never an issue because the cached copy is initialized to 0
> by kzalloc() and when uart_configure_port() is executed, at most DTR has
> been set by uart_set_options() or sunsu_console_setup().  Therefore,
> a stable designation seems unnecessary.

As per the text here, this is not needed in any stable trees, so can you
please drop it from all of your autosel queues now?

thanks,

greg k-h