[PATCH] serial: core: Keep mctrl register state and cached copy in sync

[PATCH] serial: core: Keep mctrl register state and cached copy in sync

[Lukas Wunner]

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>
---
 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 259f28ec6c8b..dc40c4155356 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2389,7 +2389,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.33.0

Re: [PATCH] serial: core: Keep mctrl register state and cached copy in sync

[Jiri Slaby]

On 02. 01. 22, 18:52, Lukas Wunner wrote:
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2389,7 +2389,8 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>   		 * We probably don't need a spinlock around this, but

One line above, it states:
--- keep the DTR setting that is set in uart_set_options()

>   		 */
>   		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);

So I don't think this is correct -- either the comment is wrong now or 
the code...

thanks,
-- 
js
suse labs

Re: [PATCH] serial: core: Keep mctrl register state and cached copy in sync

[Lukas Wunner]

On Fri, Jan 07, 2022 at 09:23:17AM +0100, Jiri Slaby wrote:
> On 02. 01. 22, 18:52, Lukas Wunner wrote:
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -2389,7 +2389,8 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> >   		 * We probably don't need a spinlock around this, but
> 
> One line above, it states:
> --- keep the DTR setting that is set in uart_set_options()

Yes:

		 * Ensure that the modem control lines are de-activated.
		 * keep the DTR setting that is set in uart_set_options()

In other words, clear all bits in port->mctrl except DTR (if it's been set
before by uart_set_options()).  That's what the code below is supposed to
achieve:

> >   		 */
> >   		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);
> 
> So I don't think this is correct -- either the comment is wrong now or the
> code...

Why do you think so?  I don't quite follow.

Thanks,

Lukas