[PATCH tty v1 0/8] synchronize UART_IER access against console write

[PATCH tty v1 0/8] synchronize UART_IER access against console write

[John Ogness]

Hi,

In preparation for making the 8250 serial driver the first driver to
support the upcoming atomic consoles [0], its console write()
callback (serial8250_console_write) was evaluated. For this callback
of the 8250 driver there are two critical writes to the UART_IER
register: once to disable all interrupts before transmitting a line
of text, and again after transmit to re-enable the previously enabled
interrupts. These two writes are performed under a single
synchronized section protected by the port lock.

I then checked all other access to UART_IER in the 8250 driver to see
if they always occurred under the port lock. If not, it would be
possible that the console write() callback could overwrite or restore
incorrect values to UART_IER. This is illustrated in the commit
message of the first patch.

Indeed several call sites were discovered where UART_IER is accessed
without the port lock. This series adds the missing locking in order
to ensure UART_IER access is always synchronized against the console
write() callback.

For call sites where UART_IER access was already performed under the
port lock, this series adds code comments and (when appropriate)
lockdep notation to help catch any future issues that may creep in.

Note that some of the new usage of port lock is not strictly
necessary, because (for example) the console is disabled before it
is suspended. However, these are not hot paths and by taking the port
lock it simplifies the synchronization semantics for UART_IER to
allow general lockdep usage.

Also note that none of these patches have been tagged for stable. The
possible stable candidates do include Fixes tags. But since the fixes
are not based on real-world reports, it probably is not necessary to
backport them.

John Ogness

[0] https://lore.kernel.org/lkml/20230302195618.156940-1-john.ogness@linutronix.de

John Ogness (8):
  serial: 8250: lock port in startup() callbacks
  serial: core: lock port for stop_rx() in uart_suspend_port()
  serial: 8250: lock port for stop_rx() in omap8250_irq()
  serial: core: lock port for start_rx() in uart_resume_port()
  serial: 8250: lock port for rx_dma() callback
  serial: 8250: lock port for omap8250_restore_regs()
  serial: 8250: lock port for UART_IER access in omap8250_irq()
  serial: 8250: synchronize and annotate UART_IER access

 drivers/tty/serial/8250/8250.h              |  6 ++
 drivers/tty/serial/8250/8250_aspeed_vuart.c |  3 +
 drivers/tty/serial/8250/8250_bcm7271.c      |  4 ++
 drivers/tty/serial/8250/8250_exar.c         |  4 ++
 drivers/tty/serial/8250/8250_mtk.c          |  9 +++
 drivers/tty/serial/8250/8250_omap.c         | 41 +++++++++++-
 drivers/tty/serial/8250/8250_port.c         | 71 ++++++++++++++++++++-
 drivers/tty/serial/serial_core.c            | 10 ++-
 8 files changed, 141 insertions(+), 7 deletions(-)


base-commit: d5b3d02d0b107345f2a6ecb5b06f98356f5c97ab
-- 
2.30.2

[PATCH tty v1 1/8] serial: 8250: lock port in startup() callbacks

[John Ogness]

uart_ops startup() callback is called without interrupts
disabled and without port->lock locked, relatively late during the
boot process (from the call path of console_on_rootfs()). If the
device is a console, it was already previously registered and could
be actively printing messages.

The console printing function serial8250_console_write() modifies
the interrupt register (UART_IER) under the port->lock with the
pattern: read, clear, restore.

Since some startup() callbacks are modifying UART_IER without the
port->lock locked, it is possible that the value intended to be
written by the startup() callback will get overwritten and be
lost.

CPU0                           CPU1
serial8250_console_write       omap_8250_startup
--------------------------     -----------------
spin_lock(port->lock)
oldval = read(UART_IER)
uart_console_write()
                               write(newval, UART_IER)
write(oldval, UART_IER)
spin_unlock(port->lock)

Add port->lock synchronization to the 8250 startup() callbacks
where they need to access UART_IER. This avoids racing with
serial8250_console_write().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_bcm7271.c |  4 ++++
 drivers/tty/serial/8250/8250_exar.c    |  4 ++++
 drivers/tty/serial/8250/8250_omap.c    |  3 +++
 drivers/tty/serial/8250/8250_port.c    | 18 ++++++++++++++++--
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c
index f801b1f5b46c..2b38bcc5112e 100644
--- a/drivers/tty/serial/8250/8250_bcm7271.c
+++ b/drivers/tty/serial/8250/8250_bcm7271.c
@@ -605,9 +605,13 @@ static int brcmuart_startup(struct uart_port *port)
 	/*
 	 * Disable the Receive Data Interrupt because the DMA engine
 	 * will handle this.
+	 *
+	 * Synchronize UART_IER access against the console.
 	 */
+	spin_lock_irq(&port->lock);
 	up->ier &= ~UART_IER_RDI;
 	serial_port_out(port, UART_IER, up->ier);
+	spin_unlock_irq(&port->lock);
 
 	priv->tx_running = false;
 	priv->dma.rx_dma = NULL;
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 64770c62bbec..ae66ef9d863c 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -194,8 +194,12 @@ static int xr17v35x_startup(struct uart_port *port)
 	/*
 	 * Make sure all interrups are masked until initialization is
 	 * complete and the FIFOs are cleared
+	 *
+	 * Synchronize UART_IER access against the console.
 	 */
+	spin_lock_irq(&port->lock);
 	serial_port_out(port, UART_IER, 0);
+	spin_unlock_irq(&port->lock);
 
 	return serial8250_do_startup(port);
 }
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 5c093dfcee1d..fbca0692aa51 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -710,8 +710,11 @@ static int omap_8250_startup(struct uart_port *port)
 		up->dma = NULL;
 	}
 
+	/* Synchronize UART_IER access against the console. */
+	spin_lock_irq(&port->lock);
 	up->ier = UART_IER_RLSI | UART_IER_RDI;
 	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irq(&port->lock);
 
 #ifdef CONFIG_PM
 	up->capabilities |= UART_CAP_RPM;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 0cef9bfd0471..b3971302d8e5 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2149,7 +2149,12 @@ int serial8250_do_startup(struct uart_port *port)
 
 	serial8250_rpm_get(up);
 	if (port->type == PORT_16C950) {
-		/* Wake up and initialize UART */
+		/*
+		 * Wake up and initialize UART
+		 *
+		 * Synchronize UART_IER access against the console.
+		 */
+		spin_lock_irqsave(&port->lock, flags);
 		up->acr = 0;
 		serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
 		serial_port_out(port, UART_EFR, UART_EFR_ECB);
@@ -2159,12 +2164,19 @@ int serial8250_do_startup(struct uart_port *port)
 		serial_port_out(port, UART_LCR, UART_LCR_CONF_MODE_B);
 		serial_port_out(port, UART_EFR, UART_EFR_ECB);
 		serial_port_out(port, UART_LCR, 0);
+		spin_unlock_irqrestore(&port->lock, flags);
 	}
 
 	if (port->type == PORT_DA830) {
-		/* Reset the port */
+		/*
+		 * Reset the port
+		 *
+		 * Synchronize UART_IER access against the console.
+		 */
+		spin_lock_irqsave(&port->lock, flags);
 		serial_port_out(port, UART_IER, 0);
 		serial_port_out(port, UART_DA830_PWREMU_MGMT, 0);
+		spin_unlock_irqrestore(&port->lock, flags);
 		mdelay(10);
 
 		/* Enable Tx, Rx and free run mode */
@@ -2275,6 +2287,8 @@ int serial8250_do_startup(struct uart_port *port)
 		 * this interrupt whenever the transmitter is idle and
 		 * the interrupt is enabled.  Delays are necessary to
 		 * allow register changes to become visible.
+		 *
+		 * Synchronize UART_IER access against the console.
 		 */
 		spin_lock_irqsave(&port->lock, flags);
 
-- 
2.30.2

[PATCH tty v1 2/8] serial: core: lock port for stop_rx() in uart_suspend_port()

[John Ogness]

The uarts_ops stop_rx() callback expects that the port->lock is
taken and interrupts are disabled.

Fixes: c9d2325cdb92 ("serial: core: Do stop_rx in suspend path for console if console_suspend is disabled")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/serial_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 4b98d13555c0..37ad53616372 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2333,8 +2333,11 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 	 * able to Re-start_rx later.
 	 */
 	if (!console_suspend_enabled && uart_console(uport)) {
-		if (uport->ops->start_rx)
+		if (uport->ops->start_rx) {
+			spin_lock_irq(&uport->lock);
 			uport->ops->stop_rx(uport);
+			spin_unlock_irq(&uport->lock);
+		}
 		goto unlock;
 	}
 
-- 
2.30.2

RE: [PATCH tty v1 2/8] serial: core: lock port for stop_rx() in uart_suspend_port()

[Vijaya Krishna Nivarthi (Temp) (QUIC)]

++Doug

Hi,


> -----Original Message-----
> From: John Ogness <john.ogness@linutronix.de>
> Sent: Thursday, May 25, 2023 3:02 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Petr Mladek <pmladek@suse.com>; Thomas Gleixner
> <tglx@linutronix.de>; linux-kernel@vger.kernel.org; Jiri Slaby
> <jirislaby@kernel.org>; Vijaya Krishna Nivarthi (Temp) (QUIC)
> <quic_vnivarth@quicinc.com>; linux-serial@vger.kernel.org
> Subject: [PATCH tty v1 2/8] serial: core: lock port for stop_rx() in
> uart_suspend_port()
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> The uarts_ops stop_rx() callback expects that the port->lock is taken and
> interrupts are disabled.
> 
> Fixes: c9d2325cdb92 ("serial: core: Do stop_rx in suspend path for console if
> console_suspend is disabled")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/serial_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 4b98d13555c0..37ad53616372 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2333,8 +2333,11 @@ int uart_suspend_port(struct uart_driver *drv,
> struct uart_port *uport)
>          * able to Re-start_rx later.
>          */
>         if (!console_suspend_enabled && uart_console(uport)) {
> -               if (uport->ops->start_rx)
> +               if (uport->ops->start_rx) {
> +                       spin_lock_irq(&uport->lock);
>                         uport->ops->stop_rx(uport);
> +                       spin_unlock_irq(&uport->lock);
> +               }


Looks correct to me.
Thank you for the fix.

-Vijay/


>                 goto unlock;
>         }
> 
> --
> 2.30.2

Re: [PATCH tty v1 2/8] serial: core: lock port for stop_rx() in uart_suspend_port()

[Doug Anderson]

Hi,

On Thu, May 25, 2023 at 2:35 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> The uarts_ops stop_rx() callback expects that the port->lock is
> taken and interrupts are disabled.
>
> Fixes: c9d2325cdb92 ("serial: core: Do stop_rx in suspend path for console if console_suspend is disabled")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/serial_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH tty v1 3/8] serial: 8250: lock port for stop_rx() in omap8250_irq()

[John Ogness]

The uarts_ops stop_rx() callback expects that the port->lock is
taken and interrupts are disabled.

Fixes: 1fe0e1fa3209 ("serial: 8250_omap: Handle optional overrun-throttle-ms property")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index fbca0692aa51..c17d98161d5e 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -658,7 +658,9 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 
 		up->ier = port->serial_in(port, UART_IER);
 		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
+			spin_lock(&port->lock);
 			port->ops->stop_rx(port);
+			spin_unlock(&port->lock);
 		} else {
 			/* Keep restarting the timer until
 			 * the input overrun subsides.
-- 
2.30.2

Re: [PATCH tty v1 3/8] serial: 8250: lock port for stop_rx() in omap8250_irq()

[Tony Lindgren]

* John Ogness <john.ogness@linutronix.de> [230525 09:34]:
> The uarts_ops stop_rx() callback expects that the port->lock is
> taken and interrupts are disabled.
> 
> Fixes: 1fe0e1fa3209 ("serial: 8250_omap: Handle optional overrun-throttle-ms property")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Looks good to me:

Reviewed-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/tty/serial/8250/8250_omap.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index fbca0692aa51..c17d98161d5e 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -658,7 +658,9 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
>  
>  		up->ier = port->serial_in(port, UART_IER);
>  		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
> +			spin_lock(&port->lock);
>  			port->ops->stop_rx(port);
> +			spin_unlock(&port->lock);
>  		} else {
>  			/* Keep restarting the timer until
>  			 * the input overrun subsides.
> -- 
> 2.30.2
> 

[PATCH tty v1 4/8] serial: core: lock port for start_rx() in uart_resume_port()

[John Ogness]

The only user of the start_rx() callback (qcom_geni) directly calls
its own stop_rx() callback. Since stop_rx() requires that the
port->lock is taken and interrupts are disabled, the start_rx()
callback has the same requirement.

Fixes: cfab87c2c271 ("serial: core: Introduce callback for start_rx and do stop_rx in suspend only if this callback implementation is present.")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/serial_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 37ad53616372..f856c7fae2fd 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2430,8 +2430,11 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 		if (console_suspend_enabled)
 			uart_change_pm(state, UART_PM_STATE_ON);
 		uport->ops->set_termios(uport, &termios, NULL);
-		if (!console_suspend_enabled && uport->ops->start_rx)
+		if (!console_suspend_enabled && uport->ops->start_rx) {
+			spin_lock_irq(&uport->lock);
 			uport->ops->start_rx(uport);
+			spin_unlock_irq(&uport->lock);
+		}
 		if (console_suspend_enabled)
 			console_start(uport->cons);
 	}
-- 
2.30.2

Re: [PATCH tty v1 4/8] serial: core: lock port for start_rx() in uart_resume_port()

[Doug Anderson]

Hi,

On Thu, May 25, 2023 at 2:34 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> The only user of the start_rx() callback (qcom_geni) directly calls
> its own stop_rx() callback. Since stop_rx() requires that the
> port->lock is taken and interrupts are disabled, the start_rx()
> callback has the same requirement.
>
> Fixes: cfab87c2c271 ("serial: core: Introduce callback for start_rx and do stop_rx in suspend only if this callback implementation is present.")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/serial_core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 37ad53616372..f856c7fae2fd 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2430,8 +2430,11 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>                 if (console_suspend_enabled)
>                         uart_change_pm(state, UART_PM_STATE_ON);
>                 uport->ops->set_termios(uport, &termios, NULL);
> -               if (!console_suspend_enabled && uport->ops->start_rx)
> +               if (!console_suspend_enabled && uport->ops->start_rx) {
> +                       spin_lock_irq(&uport->lock);
>                         uport->ops->start_rx(uport);
> +                       spin_unlock_irq(&uport->lock);
> +               }

Seems right, but shouldn't you also fix the call to stop_rx() that the
same commit cfab87c2c271 ("serial: core: Introduce callback for
start_rx and do stop_rx in suspend only if this callback
implementation is present.") added? That one is also missing the lock,
right?

-Doug

Re: [PATCH tty v1 4/8] serial: core: lock port for start_rx() in uart_resume_port()

[Doug Anderson]

Hi,

On Thu, May 25, 2023 at 9:07 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, May 25, 2023 at 2:34 AM John Ogness <john.ogness@linutronix.de> wrote:
> >
> > The only user of the start_rx() callback (qcom_geni) directly calls
> > its own stop_rx() callback. Since stop_rx() requires that the
> > port->lock is taken and interrupts are disabled, the start_rx()
> > callback has the same requirement.
> >
> > Fixes: cfab87c2c271 ("serial: core: Introduce callback for start_rx and do stop_rx in suspend only if this callback implementation is present.")
> > Signed-off-by: John Ogness <john.ogness@linutronix.de>
> > ---
> >  drivers/tty/serial/serial_core.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 37ad53616372..f856c7fae2fd 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -2430,8 +2430,11 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
> >                 if (console_suspend_enabled)
> >                         uart_change_pm(state, UART_PM_STATE_ON);
> >                 uport->ops->set_termios(uport, &termios, NULL);
> > -               if (!console_suspend_enabled && uport->ops->start_rx)
> > +               if (!console_suspend_enabled && uport->ops->start_rx) {
> > +                       spin_lock_irq(&uport->lock);
> >                         uport->ops->start_rx(uport);
> > +                       spin_unlock_irq(&uport->lock);
> > +               }
>
> Seems right, but shouldn't you also fix the call to stop_rx() that the
> same commit cfab87c2c271 ("serial: core: Introduce callback for
> start_rx and do stop_rx in suspend only if this callback
> implementation is present.") added? That one is also missing the lock,
> right?

Ah, I see. You did that in a separate patch and I wasn't CCed. I guess
I would have just put the two in one patch, but I don't feel that
strongly.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH tty v1 4/8] serial: core: lock port for start_rx() in uart_resume_port()

[John Ogness]

On 2023-05-25, Doug Anderson <dianders@chromium.org> wrote:
>> Seems right, but shouldn't you also fix the call to stop_rx() that
>> the same commit cfab87c2c271 ("serial: core: Introduce callback for
>> start_rx and do stop_rx in suspend only if this callback
>> implementation is present.") added? That one is also missing the
>> lock, right?
>
> Ah, I see. You did that in a separate patch and I wasn't CCed. I guess
> I would have just put the two in one patch, but I don't feel that
> strongly.

Actually stop_rx() was introduced in a different commit. The commit you
reference just changed it a bit. My other patch uses a different Fixes
tag.

Also, I was concerned about packing too much new spin locking in a
single commit in the hopes it will help with any bisecting issues.

> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks!

John

[PATCH tty v1 5/8] serial: 8250: lock port for rx_dma() callback

[John Ogness]

The rx_dma() callback (omap_8250_rx_dma) accesses UART_IER. This
register is modified twice by each console write
(serial8250_console_write()) under the port lock. However, not
all calls to the rx_dma() callback are under the port lock.

Add the missing port locking around rx_dma() callback calls. Add
lockdep notation to the omap_8250_rx_dma().

Note that this is not fixing a real problem because:

1. Currently DMA is forced off for 8250_omap consoles.

2. The serial devices are resumed before console printing is enabled.

However, adding this locking allows for clean locking semantics
for the rx_dma() callback so that lockdep can be used to identify
possible problems in the future. It also simplifies synchronization
of UART_IER in general by not needing to rely on implementation
details such as 1 and 2.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index c17d98161d5e..3cb9cfa62331 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -728,8 +728,11 @@ static int omap_8250_startup(struct uart_port *port)
 		priv->wer |= OMAP_UART_TX_WAKEUP_EN;
 	serial_out(up, UART_OMAP_WER, priv->wer);
 
-	if (up->dma && !(priv->habit & UART_HAS_EFR2))
+	if (up->dma && !(priv->habit & UART_HAS_EFR2)) {
+		spin_lock_irq(&port->lock);
 		up->dma->rx_dma(up);
+		spin_unlock_irq(&port->lock);
+	}
 
 	enable_irq(up->port.irq);
 
@@ -1003,6 +1006,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p)
 	unsigned long			flags;
 	u32				reg;
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&p->port.lock);
+
 	if (priv->rx_dma_broken)
 		return -EINVAL;
 
@@ -1736,8 +1742,11 @@ static int omap8250_runtime_resume(struct device *dev)
 	if (up && omap8250_lost_context(up))
 		omap8250_restore_regs(up);
 
-	if (up && up->dma && up->dma->rxchan && !(priv->habit & UART_HAS_EFR2))
+	if (up && up->dma && up->dma->rxchan && !(priv->habit & UART_HAS_EFR2)) {
+		spin_lock_irq(&up->port.lock);
 		omap_8250_rx_dma(up);
+		spin_unlock_irq(&up->port.lock);
+	}
 
 	priv->latency = priv->calc_latency;
 	schedule_work(&priv->qos_work);
-- 
2.30.2

[PATCH tty v1 6/8] serial: 8250: lock port for omap8250_restore_regs()

[John Ogness]

omap8250_restore_regs() accesses UART_IER. This register is
modified twice by each console write (serial8250_console_write())
under the port lock. However, not all calls to omap8250_restore_regs()
are under the port lock.

Add the missing port locking around omap8250_restore_regs() calls. Add
lockdep notation to the omap8250_restore_regs().

Note that this is not fixing a real problem because the serial devices
are resumed before console printing is enabled.

However, adding this locking allows for clean locking semantics
for omap8250_restore_regs() so that lockdep can be used to identify
possible problems in the future. It also simplifies synchronization
of UART_IER in general by not needing to rely on such implementation
details.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 3cb9cfa62331..34939462fd69 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -309,6 +309,9 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 	struct uart_8250_dma	*dma = up->dma;
 	u8 mcr = serial8250_in_MCR(up);
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	if (dma && dma->tx_running) {
 		/*
 		 * TCSANOW requests the change to occur immediately however if
@@ -1739,8 +1742,11 @@ static int omap8250_runtime_resume(struct device *dev)
 	if (priv->line >= 0)
 		up = serial8250_get_port(priv->line);
 
-	if (up && omap8250_lost_context(up))
+	if (up && omap8250_lost_context(up)) {
+		spin_lock_irq(&up->port.lock);
 		omap8250_restore_regs(up);
+		spin_unlock_irq(&up->port.lock);
+	}
 
 	if (up && up->dma && up->dma->rxchan && !(priv->habit & UART_HAS_EFR2)) {
 		spin_lock_irq(&up->port.lock);
-- 
2.30.2

[PATCH tty v1 7/8] serial: 8250: lock port for UART_IER access in omap8250_irq()

[John Ogness]

omap8250_irq() accesses UART_IER. This register is modified twice
by each console write (serial8250_console_write()) under the port
lock. omap8250_irq() must also take the port lock to guanentee
synchronized access to UART_IER.

Since the port lock is already being taken for the stop_rx() callback
and since it is safe to call cancel_delayed_work() while holding the
port lock, simply extend the port lock region to include UART_IER
access.

Fixes: 1fe0e1fa3209 ("serial: 8250_omap: Handle optional overrun-throttle-ms property")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 34939462fd69..3225c95fde1d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -659,17 +659,18 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	if ((lsr & UART_LSR_OE) && up->overrun_backoff_time_ms > 0) {
 		unsigned long delay;
 
+		/* Synchronize UART_IER access against the console. */
+		spin_lock(&port->lock);
 		up->ier = port->serial_in(port, UART_IER);
 		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
-			spin_lock(&port->lock);
 			port->ops->stop_rx(port);
-			spin_unlock(&port->lock);
 		} else {
 			/* Keep restarting the timer until
 			 * the input overrun subsides.
 			 */
 			cancel_delayed_work(&up->overrun_backoff);
 		}
+		spin_unlock(&port->lock);
 
 		delay = msecs_to_jiffies(up->overrun_backoff_time_ms);
 		schedule_delayed_work(&up->overrun_backoff, delay);
-- 
2.30.2

Re: [PATCH tty v1 7/8] serial: 8250: lock port for UART_IER access in omap8250_irq()

[Tony Lindgren]

* John Ogness <john.ogness@linutronix.de> [230525 09:34]:
> omap8250_irq() accesses UART_IER. This register is modified twice
> by each console write (serial8250_console_write()) under the port
> lock. omap8250_irq() must also take the port lock to guanentee
> synchronized access to UART_IER.
> 
> Since the port lock is already being taken for the stop_rx() callback
> and since it is safe to call cancel_delayed_work() while holding the
> port lock, simply extend the port lock region to include UART_IER
> access.
> 
> Fixes: 1fe0e1fa3209 ("serial: 8250_omap: Handle optional overrun-throttle-ms property")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/tty/serial/8250/8250_omap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 34939462fd69..3225c95fde1d 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -659,17 +659,18 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
>  	if ((lsr & UART_LSR_OE) && up->overrun_backoff_time_ms > 0) {
>  		unsigned long delay;
>  
> +		/* Synchronize UART_IER access against the console. */
> +		spin_lock(&port->lock);
>  		up->ier = port->serial_in(port, UART_IER);
>  		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
> -			spin_lock(&port->lock);
>  			port->ops->stop_rx(port);
> -			spin_unlock(&port->lock);
>  		} else {
>  			/* Keep restarting the timer until
>  			 * the input overrun subsides.
>  			 */
>  			cancel_delayed_work(&up->overrun_backoff);
>  		}
> +		spin_unlock(&port->lock);
>  
>  		delay = msecs_to_jiffies(up->overrun_backoff_time_ms);
>  		schedule_delayed_work(&up->overrun_backoff, delay);
> -- 
> 2.30.2
> 

[PATCH tty v1 8/8] serial: 8250: synchronize and annotate UART_IER access

[John Ogness]

The UART_IER register is modified twice by each console write
(serial8250_console_write()) under the port lock. Any driver code that
accesses UART_IER must do so with the port locked in order to ensure
consistent values, even when for read accesses.

Add locking, lockdep notation, and/or comments everywhere UART_IER is
accessed. The added locking is not fixing a real problem because it
occurs where the console is not active. However, adding the locking
to these non-critical paths greatly simplifies UART_IER access
tracking by establishing a general policy that all UART_IER access
is performed with the port locked.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h              |  6 +++
 drivers/tty/serial/8250/8250_aspeed_vuart.c |  3 ++
 drivers/tty/serial/8250/8250_mtk.c          |  9 ++++
 drivers/tty/serial/8250/8250_omap.c         | 14 ++++++
 drivers/tty/serial/8250/8250_port.c         | 53 +++++++++++++++++++++
 5 files changed, 85 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 5418708f4631..471c6bc5f78f 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -179,6 +179,9 @@ static inline void serial_dl_write(struct uart_8250_port *up, u32 value)
 
 static inline bool serial8250_set_THRI(struct uart_8250_port *up)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	if (up->ier & UART_IER_THRI)
 		return false;
 	up->ier |= UART_IER_THRI;
@@ -188,6 +191,9 @@ static inline bool serial8250_set_THRI(struct uart_8250_port *up)
 
 static inline bool serial8250_clear_THRI(struct uart_8250_port *up)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	if (!(up->ier & UART_IER_THRI))
 		return false;
 	up->ier &= ~UART_IER_THRI;
diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 9d2a7856784f..4a9e71b2dbbc 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -275,6 +275,9 @@ static void __aspeed_vuart_set_throttle(struct uart_8250_port *up,
 {
 	unsigned char irqs = UART_IER_RLSI | UART_IER_RDI;
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	up->ier &= ~irqs;
 	if (!throttle)
 		up->ier |= irqs;
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index fb1d5ec0940e..aa8e98164d68 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -222,11 +222,17 @@ static void mtk8250_shutdown(struct uart_port *port)
 
 static void mtk8250_disable_intrs(struct uart_8250_port *up, int mask)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	serial_out(up, UART_IER, serial_in(up, UART_IER) & (~mask));
 }
 
 static void mtk8250_enable_intrs(struct uart_8250_port *up, int mask)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	serial_out(up, UART_IER, serial_in(up, UART_IER) | mask);
 }
 
@@ -235,6 +241,9 @@ static void mtk8250_set_flow_ctrl(struct uart_8250_port *up, int mode)
 	struct uart_port *port = &up->port;
 	int lcr = serial_in(up, UART_LCR);
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&port->lock);
+
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	serial_out(up, MTK_UART_EFR, UART_EFR_ECB);
 	serial_out(up, UART_LCR, lcr);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 3225c95fde1d..0498b9b0e4e9 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -533,6 +533,10 @@ static void omap_8250_pm(struct uart_port *port, unsigned int state,
 	u8 efr;
 
 	pm_runtime_get_sync(port->dev);
+
+	/* Synchronize UART_IER access against the console. */
+	spin_lock_irq(&port->lock);
+
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	efr = serial_in(up, UART_EFR);
 	serial_out(up, UART_EFR, efr | UART_EFR_ECB);
@@ -543,6 +547,8 @@ static void omap_8250_pm(struct uart_port *port, unsigned int state,
 	serial_out(up, UART_EFR, efr);
 	serial_out(up, UART_LCR, 0);
 
+	spin_unlock_irq(&port->lock);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 }
@@ -760,8 +766,11 @@ static void omap_8250_shutdown(struct uart_port *port)
 	if (priv->habit & UART_HAS_EFR2)
 		serial_out(up, UART_OMAP_EFR2, 0x0);
 
+	/* Synchronize UART_IER access against the console. */
+	spin_lock_irq(&port->lock);
 	up->ier = 0;
 	serial_out(up, UART_IER, 0);
+	spin_unlock_irq(&port->lock);
 	disable_irq_nosync(up->port.irq);
 	dev_pm_clear_wake_irq(port->dev);
 
@@ -803,6 +812,7 @@ static void omap_8250_unthrottle(struct uart_port *port)
 
 	pm_runtime_get_sync(port->dev);
 
+	/* Synchronize UART_IER access against the console. */
 	spin_lock_irqsave(&port->lock, flags);
 	priv->throttled = false;
 	if (up->dma)
@@ -953,6 +963,7 @@ static void __dma_rx_complete(void *param)
 	struct dma_tx_state     state;
 	unsigned long flags;
 
+	/* Synchronize UART_IER access against the console. */
 	spin_lock_irqsave(&p->port.lock, flags);
 
 	/*
@@ -1227,6 +1238,9 @@ static u16 omap_8250_handle_rx_dma(struct uart_8250_port *up, u8 iir, u16 status
 static void am654_8250_handle_rx_dma(struct uart_8250_port *up, u8 iir,
 				     u16 status)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	/*
 	 * Queue a new transfer if FIFO has data.
 	 */
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index b3971302d8e5..4b7bbd8b3305 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -539,6 +539,9 @@ EXPORT_SYMBOL_GPL(serial8250_rpm_put);
  */
 static int serial8250_em485_init(struct uart_8250_port *p)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&p->port.lock);
+
 	if (p->em485)
 		goto deassert_rts;
 
@@ -676,6 +679,8 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 	serial8250_rpm_get(p);
 
 	if (p->capabilities & UART_CAP_SLEEP) {
+		/* Synchronize UART_IER access against the console. */
+		spin_lock_irq(&p->port.lock);
 		if (p->capabilities & UART_CAP_EFR) {
 			lcr = serial_in(p, UART_LCR);
 			efr = serial_in(p, UART_EFR);
@@ -689,6 +694,7 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 			serial_out(p, UART_EFR, efr);
 			serial_out(p, UART_LCR, lcr);
 		}
+		spin_unlock_irq(&p->port.lock);
 	}
 
 	serial8250_rpm_put(p);
@@ -696,6 +702,9 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 
 static void serial8250_clear_IER(struct uart_8250_port *up)
 {
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	if (up->capabilities & UART_CAP_UUE)
 		serial_out(up, UART_IER, UART_IER_UUE);
 	else
@@ -968,6 +977,9 @@ static void autoconfig_16550a(struct uart_8250_port *up)
 	unsigned char status1, status2;
 	unsigned int iersave;
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&up->port.lock);
+
 	up->port.type = PORT_16550A;
 	up->capabilities |= UART_CAP_FIFO;
 
@@ -1151,6 +1163,8 @@ static void autoconfig(struct uart_8250_port *up)
 	/*
 	 * We really do need global IRQs disabled here - we're going to
 	 * be frobbing the chips IRQ enable register to see if it exists.
+	 *
+	 * Synchronize UART_IER access against the console.
 	 */
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -1323,7 +1337,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	/* forget possible initially masked and pending IRQ */
 	probe_irq_off(probe_irq_on());
 	save_mcr = serial8250_in_MCR(up);
+	/* Synchronize UART_IER access against the console. */
+	spin_lock_irq(&port->lock);
 	save_ier = serial_in(up, UART_IER);
+	spin_unlock_irq(&port->lock);
 	serial8250_out_MCR(up, UART_MCR_OUT1 | UART_MCR_OUT2);
 
 	irqs = probe_irq_on();
@@ -1335,7 +1352,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
 		serial8250_out_MCR(up,
 			UART_MCR_DTR | UART_MCR_RTS | UART_MCR_OUT2);
 	}
+	/* Synchronize UART_IER access against the console. */
+	spin_lock_irq(&port->lock);
 	serial_out(up, UART_IER, UART_IER_ALL_INTR);
+	spin_unlock_irq(&port->lock);
 	serial_in(up, UART_LSR);
 	serial_in(up, UART_RX);
 	serial_in(up, UART_IIR);
@@ -1345,7 +1365,10 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	irq = probe_irq_off(irqs);
 
 	serial8250_out_MCR(up, save_mcr);
+	/* Synchronize UART_IER access against the console. */
+	spin_lock_irq(&port->lock);
 	serial_out(up, UART_IER, save_ier);
+	spin_unlock_irq(&port->lock);
 
 	if (port->flags & UPF_FOURPORT)
 		outb_p(save_ICP, ICP);
@@ -1360,6 +1383,9 @@ static void serial8250_stop_rx(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&port->lock);
+
 	serial8250_rpm_get(up);
 
 	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
@@ -1379,6 +1405,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&p->port.lock);
+
 	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		mcr |= UART_MCR_RTS;
 	else
@@ -1428,6 +1457,9 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
 {
 	struct uart_8250_em485 *em485 = p->em485;
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&p->port.lock);
+
 	stop_delay += (u64)p->port.rs485.delay_rts_after_send * NSEC_PER_MSEC;
 
 	/*
@@ -1607,6 +1639,9 @@ static void serial8250_start_tx(struct uart_port *port)
 	struct uart_8250_port *up = up_to_u8250p(port);
 	struct uart_8250_em485 *em485 = up->em485;
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&port->lock);
+
 	if (!port->x_char && uart_circ_empty(&port->state->xmit))
 		return;
 
@@ -1634,6 +1669,9 @@ static void serial8250_disable_ms(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&port->lock);
+
 	/* no MSR capabilities */
 	if (up->bugs & UART_BUG_NOMSR)
 		return;
@@ -1648,6 +1686,9 @@ static void serial8250_enable_ms(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&port->lock);
+
 	/* no MSR capabilities */
 	if (up->bugs & UART_BUG_NOMSR)
 		return;
@@ -2104,6 +2145,14 @@ static void serial8250_put_poll_char(struct uart_port *port,
 	unsigned int ier;
 	struct uart_8250_port *up = up_to_u8250p(port);
 
+	/*
+	 * Normally the port is locked to synchronize UART_IER access
+	 * against the console. However, this function is only used by
+	 * KDB/KGDB, where it may not be possible to acquire the port
+	 * lock because all other CPUs are quiesced. The quiescence
+	 * should allow safe lockless usage here.
+	 */
+
 	serial8250_rpm_get(up);
 	/*
 	 *	First save the IER then disable the interrupts
@@ -2439,6 +2488,8 @@ void serial8250_do_shutdown(struct uart_port *port)
 	serial8250_rpm_get(up);
 	/*
 	 * Disable interrupts from this port
+	 *
+	 * Synchronize UART_IER access against the console.
 	 */
 	spin_lock_irqsave(&port->lock, flags);
 	up->ier = 0;
@@ -2738,6 +2789,8 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	/*
 	 * Ok, we're now changing the port state.  Do it with
 	 * interrupts disabled.
+	 *
+	 * Synchronize UART_IER access against the console.
 	 */
 	serial8250_rpm_get(up);
 	spin_lock_irqsave(&port->lock, flags);
-- 
2.30.2