* [PATCH] serial: core: Keep mctrl register state and cached copy in sync
@ 2022-01-02 17:52 Lukas Wunner
2022-01-07 8:23 ` Jiri Slaby
0 siblings, 1 reply; 3+ messages in thread
From: Lukas Wunner @ 2022-01-02 17:52 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Russell King, Lino Sanfilippo, Philipp Rosenberger, Jochen Mades,
Su Bao Cheng, Jan Kiszka, Chao Zeng, linux-serial
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] serial: core: Keep mctrl register state and cached copy in sync
2022-01-02 17:52 [PATCH] serial: core: Keep mctrl register state and cached copy in sync Lukas Wunner
@ 2022-01-07 8:23 ` Jiri Slaby
2022-01-07 8:51 ` Lukas Wunner
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Slaby @ 2022-01-07 8:23 UTC (permalink / raw)
To: Lukas Wunner, Greg Kroah-Hartman
Cc: Russell King, Lino Sanfilippo, Philipp Rosenberger, Jochen Mades,
Su Bao Cheng, Jan Kiszka, Chao Zeng, linux-serial
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] serial: core: Keep mctrl register state and cached copy in sync
2022-01-07 8:23 ` Jiri Slaby
@ 2022-01-07 8:51 ` Lukas Wunner
0 siblings, 0 replies; 3+ messages in thread
From: Lukas Wunner @ 2022-01-07 8:51 UTC (permalink / raw)
To: Jiri Slaby
Cc: Greg Kroah-Hartman, Russell King, Lino Sanfilippo,
Philipp Rosenberger, Jochen Mades, Su Bao Cheng, Jan Kiszka,
Chao Zeng, linux-serial
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-07 8:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-02 17:52 [PATCH] serial: core: Keep mctrl register state and cached copy in sync Lukas Wunner
2022-01-07 8:23 ` Jiri Slaby
2022-01-07 8:51 ` Lukas Wunner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.