diff mbox series

[next,v1,1/2] serial: 8250: Switch to nbcon console

Message ID 20240905134719.142554-2-john.ogness@linutronix.de
State New
Headers show
Series convert 8250 to nbcon | expand

Commit Message

John Ogness Sept. 5, 2024, 1:47 p.m. UTC
Implement the necessary callbacks to switch the 8250 console driver
to perform as an nbcon console.

Add implementations for the nbcon console callbacks (write_atomic,
write_thread, device_lock, device_unlock) and add CON_NBCON to the
initial flags.

The legacy code is kept in order to easily switch back to legacy mode
by defining USE_SERIAL_8250_LEGACY_CONSOLE.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c |  42 +++++++-
 drivers/tty/serial/8250/8250_port.c | 148 +++++++++++++++++++++++++++-
 include/linux/serial_8250.h         |   6 ++
 3 files changed, 193 insertions(+), 3 deletions(-)

Comments

Greg Kroah-Hartman Sept. 6, 2024, 10:10 a.m. UTC | #1
On Thu, Sep 05, 2024 at 03:53:18PM +0206, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks (write_atomic,
> write_thread, device_lock, device_unlock) and add CON_NBCON to the
> initial flags.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.

define it where?

And ick, having #ifdef like this is rough to maintain, why is it needed?
If this is working well, let's just switch over to the new stuff and not
look back!

thanks,

greg k-h
Petr Mladek Sept. 6, 2024, 12:37 p.m. UTC | #2
On Thu 2024-09-05 15:53:18, John Ogness wrote:
> Implement the necessary callbacks to switch the 8250 console driver
> to perform as an nbcon console.
> 
> Add implementations for the nbcon console callbacks (write_atomic,
> write_thread, device_lock, device_unlock) and add CON_NBCON to the
> initial flags.
> 
> The legacy code is kept in order to easily switch back to legacy mode
> by defining USE_SERIAL_8250_LEGACY_CONSOLE.
> 
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -388,6 +388,7 @@ void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
>  
>  #ifdef CONFIG_SERIAL_8250_CONSOLE
>  
> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE

Just for record. I agree that it is better to simply remove the
obsolete legacy code.

Or maybe, we would need to keep it for the rs485 consoles, see below.

>  static void univ8250_console_write(struct console *co, const char *s,
>  				   unsigned int count)
>  {
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -546,6 +546,13 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  	if (!p->em485)
>  		return -ENOMEM;
>  
> +#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
> +	if (uart_console(&p->port)) {
> +		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
> +		p->port.cons->write_atomic = NULL;
> +	}

Wait! This makes the rs485 consoles much less usable for debugging.
They might have troubles to see the emergency and panic messages.
Or do I miss anything, please?

Is this acceptable? Why?
Why is this limitation exactly needed?

Is is because the following code is not safe enough for the write_atomic
variant when it is guarded only by the nbcon context ownership?

void serial8250_console_write_thread(struct uart_8250_port *up,
				     struct nbcon_write_context *wctxt)
{
[...]
	if (em485) {
		if (em485->tx_stopped)
			up->rs485_start_tx(up);
		mdelay(port->rs485.delay_rts_before_send);
	}
[...]
	if (em485) {
		mdelay(port->rs485.delay_rts_after_send);
		if (em485->tx_stopped)
			up->rs485_stop_tx(up);
	}
[...]

Would it break even the nbcon console context it taken over the safe
way? Or only by "unsafe" takeover?

IMHO, we should risk the "unsafe" takeover. We still would be
in a better situation than the legacy code which ignores
the port->lock during panic() all the time (after bust_

> +#endif
> +
>  	hrtimer_init(&p->em485->stop_tx_timer, CLOCK_MONOTONIC,
>  		     HRTIMER_MODE_REL);
>  	hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
> @@ -3269,6 +3285,11 @@ static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
>  
>  	wait_for_xmitr(up, UART_LSR_THRE);
>  	serial_port_out(port, UART_TX, ch);
> +
> +	if (ch == '\n')
> +		up->console_newline_needed = false;
> +	else
> +		up->console_newline_needed = true;

I might be just dumb but this code confused me. I missed that the
variable was actually set after printing the character. I inverted
the logic in my head and it did not make sense.

I vote for adding a comment. Or better make the code more
straightforward by renaming the variable and inverting the logic:

	if (ch == '\n')
		up->console_line_ended = true;
	else
		up->console_line_ended = false;

>  }
>  
>  /*
> @@ -3421,6 +3443,125 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
>  	if (locked)
>  		uart_port_unlock_irqrestore(port, flags);
>  }
> +#else
> +void serial8250_console_write_thread(struct uart_8250_port *up,
> +				     struct nbcon_write_context *wctxt)
> +{
> +	struct uart_8250_em485 *em485 = up->em485;
> +	struct uart_port *port = &up->port;
> +	unsigned int ier;
> +
> +	touch_nmi_watchdog();

This should not be needed in the write_thread() variant because
it allows to schedule after emitting one record.

> +	if (!nbcon_enter_unsafe(wctxt))
> +		return;
> +

The rest looks good.

Best Regards,
Petr
John Ogness Sept. 6, 2024, 1:35 p.m. UTC | #3
On 2024-09-06, Petr Mladek <pmladek@suse.com> wrote:
>> +#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
>
> Just for record. I agree that it is better to simply remove the
> obsolete legacy code.

Agreed. I will be removing it for v2.

>> +#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
>> +	if (uart_console(&p->port)) {
>> +		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
>> +		p->port.cons->write_atomic = NULL;
>> +	}
>
> Wait! This makes the rs485 consoles much less usable for debugging.
> They might have troubles to see the emergency and panic messages.
> Or do I miss anything, please?
>
> Is this acceptable? Why?

It is not acceptable. I am looking into making the atomic part work for
RS485 as well. My main problem is testing since I will need to get my
hands or real RS485 hardware.

>>  	wait_for_xmitr(up, UART_LSR_THRE);
>>  	serial_port_out(port, UART_TX, ch);
>> +
>> +	if (ch == '\n')
>> +		up->console_newline_needed = false;
>> +	else
>> +		up->console_newline_needed = true;
>
> I might be just dumb but this code confused me. I missed that the
> variable was actually set after printing the character. I inverted
> the logic in my head and it did not make sense.
>
> I vote for adding a comment. Or better make the code more
> straightforward by renaming the variable and inverting the logic:
>
> 	if (ch == '\n')
> 		up->console_line_ended = true;
> 	else
> 		up->console_line_ended = false;

OK. I will add a comment, rename the variable, and invert the logic.

>> +void serial8250_console_write_thread(struct uart_8250_port *up,
>> +				     struct nbcon_write_context *wctxt)
>> +{
>> +	struct uart_8250_em485 *em485 = up->em485;
>> +	struct uart_port *port = &up->port;
>> +	unsigned int ier;
>> +
>> +	touch_nmi_watchdog();
>
> This should not be needed in the write_thread() variant because
> it allows to schedule after emitting one record.

Agreed.

Thanks.

John
John Ogness Sept. 6, 2024, 4:38 p.m. UTC | #4
On 2024-09-06, John Ogness <john.ogness@linutronix.de> wrote:
>> Wait! This makes the rs485 consoles much less usable for debugging.
>> They might have troubles to see the emergency and panic messages.
>>
>> Is this acceptable? Why?
>
> It is not acceptable. I am looking into making the atomic part work for
> RS485 as well.

So there are 2 things _not_ supported by the write_atomic() callback:

1. RS485 mode. This is due to the need to start up TX for the
write, which can lead to:

up->rs485_start_tx()
  serial8250_em485_start_tx()
    serial8250_stop_rx()
      serial8250_rpm_get()
        pm_runtime_get_sync()
          __pm_runtime_resume()
            spin_lock_irqsave()

Taking a spin lock is not safe from NMI and thus disqualifies this call
chain for write_atomic().

If UART_CAP_RPM is not set, the pm_runtime_get_sync() is avoided. So I
could only disable atomic RS485 if UART_CAP_RPM is set. But the OMAP
variant of the 8250 does set this capability.

2. Modem control. This is due to waiting for inputs, which can lead to:

serial8250_modem_status()
  wake_up_interruptible()

Performing wakes is not safe from scheduler or NMI and thus disqualifies
this call chain for write_atomic().

It would probably be acceptable to move serial8250_modem_status() into
an irq_work.

I would be grateful for any insights on how best to handle these 2
issues if we want full write_atomic() support for all 8250 variants.

John
Thomas Gleixner Sept. 7, 2024, 8:39 p.m. UTC | #5
On Fri, Sep 06 2024 at 18:44, John Ogness wrote:
> So there are 2 things _not_ supported by the write_atomic() callback:
>
> 1. RS485 mode. This is due to the need to start up TX for the
> write, which can lead to:
>
> up->rs485_start_tx()
>   serial8250_em485_start_tx()
>     serial8250_stop_rx()
>       serial8250_rpm_get()
>         pm_runtime_get_sync()
>           __pm_runtime_resume()
>             spin_lock_irqsave()
>
> Taking a spin lock is not safe from NMI and thus disqualifies this call
> chain for write_atomic().

Correct. __pm_runtime_resume() can sleep as well :)

> If UART_CAP_RPM is not set, the pm_runtime_get_sync() is avoided. So I
> could only disable atomic RS485 if UART_CAP_RPM is set. But the OMAP
> variant of the 8250 does set this capability.

Sure, but none of this makes sense. What's so special about that em485
muck that serial8250_stop_rx() needs to do that PM dance?

It writes the IER register, which serial8250_console_write() just wrote
to in serial8250_clear_IER() without doing this PM dance. So for the
console write path this stop part is not required at all.  That said,
serial8250_em485_stop_tx() doesn't have this PM dance either.

I'm 100% that this is just a problem of blindly sharing this with the
regular uart code and not because there is a requirement. See what
serial8250_console_setup() does at the end:

        if (port->dev)
                pm_runtime_get_sync(port->dev);

The corresponding put() is in serial8250_console_exit(). So there is
absolutely zero reason for power management in the console write
functions. It's the usual voodoo programming which nobody noticed
because it did not immediately blow up in their face.

There is another minor issue in that em485 muck. One code path arms a
hrtimer, which does not work from NMI like contexts, but that is only
taken when the transmitter is not empty, so probably a non-issue
because the console write code waits for it to be drained.

There are also a few lockdep_assert_held_once(port->lock) in that code
which will trigger when called from the nbcon write functions. They are
already broken today when oops_in_progress is set and the trylock of
port::lock fails...

So splitting this up into a clean and lean set for the console write
functions will make all these horrors just go away. The current sharing
is just fragile as hell and makes no sense at all.

> 2. Modem control. This is due to waiting for inputs, which can lead to:
>
> serial8250_modem_status()
>   wake_up_interruptible()
>
> Performing wakes is not safe from scheduler or NMI and thus disqualifies
> this call chain for write_atomic().
>
> It would probably be acceptable to move serial8250_modem_status() into
> an irq_work.

Yes, but serial8250_modem_status() has more problems than that:

See uart_handle_dcd_change() and uart_handle_cts_change(). They call
into the tty layer and do their own wakeups.

So no, serial8250_modem_status() cannot be invoked there at all.

You have to defer this whole status dance to irq work and this really
needs to be done inside the write_atomic() callback. Otherwise a status
change could get lost, which is bad in non-panic situations.

That needs a bit of thought vs. port->msr_saved_flags, because in a
hostile takeover situation that needs to take into account that the
interrupted context might be fiddling with msr_saved_flags too, which
might on resume overwrite the write_atomic() modifications due to RMW.
Shouldn't be hard.

That brings me to that USE_SERIAL_8250_LEGACY_CONSOLE #ifdeffery, which
started this conversation.

The nbcon conversion does not make things worse than they are today. Any
problem which happens in the atomic_write() callback has existed before
already. So just get rid of the legacy code and be done with it.

At some point you have to bite the bullet and deal with the fallout when
it's reported. Remember, perfect is the enemy of good and you will never
reach perfect.

Thanks,

        tglx
Andy Shevchenko Sept. 9, 2024, 9:50 a.m. UTC | #6
On Fri, Sep 06, 2024 at 06:44:41PM +0206, John Ogness wrote:
> On 2024-09-06, John Ogness <john.ogness@linutronix.de> wrote:
> >> Wait! This makes the rs485 consoles much less usable for debugging.
> >> They might have troubles to see the emergency and panic messages.
> >>
> >> Is this acceptable? Why?
> >
> > It is not acceptable. I am looking into making the atomic part work for
> > RS485 as well.
> 
> So there are 2 things _not_ supported by the write_atomic() callback:
> 
> 1. RS485 mode. This is due to the need to start up TX for the
> write, which can lead to:
> 
> up->rs485_start_tx()
>   serial8250_em485_start_tx()
>     serial8250_stop_rx()
>       serial8250_rpm_get()
>         pm_runtime_get_sync()
>           __pm_runtime_resume()
>             spin_lock_irqsave()
> 
> Taking a spin lock is not safe from NMI and thus disqualifies this call
> chain for write_atomic().
> 
> If UART_CAP_RPM is not set, the pm_runtime_get_sync() is avoided. So I
> could only disable atomic RS485 if UART_CAP_RPM is set. But the OMAP
> variant of the 8250 does set this capability.

Please, don't add a new code which relies on UART_CAP_RPM.
The idea is to enable runtime PM for all users who provides respective
callbacks. Rather, you should ask runtime PM for this information.

> 2. Modem control. This is due to waiting for inputs, which can lead to:
> 
> serial8250_modem_status()
>   wake_up_interruptible()
> 
> Performing wakes is not safe from scheduler or NMI and thus disqualifies
> this call chain for write_atomic().
> 
> It would probably be acceptable to move serial8250_modem_status() into
> an irq_work.
> 
> I would be grateful for any insights on how best to handle these 2
> issues if we want full write_atomic() support for all 8250 variants.
Andy Shevchenko Sept. 9, 2024, 9:53 a.m. UTC | #7
On Sat, Sep 07, 2024 at 10:39:00PM +0200, Thomas Gleixner wrote:
> On Fri, Sep 06 2024 at 18:44, John Ogness wrote:

...

> I'm 100% that this is just a problem of blindly sharing this with the
> regular uart code and not because there is a requirement. See what
> serial8250_console_setup() does at the end:
> 
>         if (port->dev)
>                 pm_runtime_get_sync(port->dev);
> 
> The corresponding put() is in serial8250_console_exit(). So there is
> absolutely zero reason for power management in the console write
> functions. It's the usual voodoo programming which nobody noticed
> because it did not immediately blow up in their face.

It might be historical, but yes, the above is for a reason.
And if somebody needs a kernel console to be shutdown, they should remove
it from the active consoles.
Thomas Gleixner Sept. 9, 2024, 12:13 p.m. UTC | #8
On Mon, Sep 09 2024 at 12:53, Andy Shevchenko wrote:
> On Sat, Sep 07, 2024 at 10:39:00PM +0200, Thomas Gleixner wrote:
>> On Fri, Sep 06 2024 at 18:44, John Ogness wrote:
>
> ...
>
>> I'm 100% that this is just a problem of blindly sharing this with the
>> regular uart code and not because there is a requirement. See what
>> serial8250_console_setup() does at the end:
>> 
>>         if (port->dev)
>>                 pm_runtime_get_sync(port->dev);
>> 
>> The corresponding put() is in serial8250_console_exit(). So there is
>> absolutely zero reason for power management in the console write
>> functions. It's the usual voodoo programming which nobody noticed
>> because it did not immediately blow up in their face.
>
> It might be historical, but yes, the above is for a reason.
> And if somebody needs a kernel console to be shutdown, they should remove
> it from the active consoles.

Correct, because you cant do PM from arbitrary contexts.

Ergo the code which does PM in the context of the console write function
is bogus.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 29e4b83e0376..d7079931dd7e 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -388,6 +388,7 @@  void __init serial8250_register_ports(struct uart_driver *drv, struct device *de
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 
+#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
 static void univ8250_console_write(struct console *co, const char *s,
 				   unsigned int count)
 {
@@ -395,6 +396,37 @@  static void univ8250_console_write(struct console *co, const char *s,
 
 	serial8250_console_write(up, s, count);
 }
+#else
+static void univ8250_console_write_atomic(struct console *co,
+					  struct nbcon_write_context *wctxt)
+{
+	struct uart_8250_port *up = &serial8250_ports[co->index];
+
+	serial8250_console_write_atomic(up, wctxt);
+}
+
+static void univ8250_console_write_thread(struct console *co,
+					  struct nbcon_write_context *wctxt)
+{
+	struct uart_8250_port *up = &serial8250_ports[co->index];
+
+	serial8250_console_write_thread(up, wctxt);
+}
+
+static void univ8250_console_device_lock(struct console *con, unsigned long *flags)
+{
+	struct uart_port *up = &serial8250_ports[con->index].port;
+
+	__uart_port_lock_irqsave(up, flags);
+}
+
+static void univ8250_console_device_unlock(struct console *con, unsigned long flags)
+{
+	struct uart_port *up = &serial8250_ports[con->index].port;
+
+	__uart_port_unlock_irqrestore(up, flags);
+}
+#endif /* USE_SERIAL_8250_LEGACY_CONSOLE */
 
 static int univ8250_console_setup(struct console *co, char *options)
 {
@@ -494,12 +526,20 @@  static int univ8250_console_match(struct console *co, char *name, int idx,
 
 static struct console univ8250_console = {
 	.name		= "ttyS",
+#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
 	.write		= univ8250_console_write,
+	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
+#else
+	.write_atomic	= univ8250_console_write_atomic,
+	.write_thread	= univ8250_console_write_thread,
+	.device_lock	= univ8250_console_device_lock,
+	.device_unlock	= univ8250_console_device_unlock,
+	.flags		= CON_PRINTBUFFER | CON_ANYTIME | CON_NBCON,
+#endif
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
 	.exit		= univ8250_console_exit,
 	.match		= univ8250_console_match,
-	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
 	.index		= -1,
 	.data		= &serial8250_reg,
 };
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 3509af7dc52b..ce841c62900d 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -546,6 +546,13 @@  static int serial8250_em485_init(struct uart_8250_port *p)
 	if (!p->em485)
 		return -ENOMEM;
 
+#ifndef USE_SERIAL_8250_LEGACY_CONSOLE
+	if (uart_console(&p->port)) {
+		dev_warn(p->port.dev, "no atomic printing for rs485 consoles\n");
+		p->port.cons->write_atomic = NULL;
+	}
+#endif
+
 	hrtimer_init(&p->em485->stop_tx_timer, CLOCK_MONOTONIC,
 		     HRTIMER_MODE_REL);
 	hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
@@ -691,7 +698,11 @@  static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 	serial8250_rpm_put(p);
 }
 
-static void serial8250_clear_IER(struct uart_8250_port *up)
+/*
+ * Only to be used by write_atomic() and the legacy write(), which do not
+ * require port lock.
+ */
+static void __serial8250_clear_IER(struct uart_8250_port *up)
 {
 	if (up->capabilities & UART_CAP_UUE)
 		serial_out(up, UART_IER, UART_IER_UUE);
@@ -699,6 +710,11 @@  static void serial8250_clear_IER(struct uart_8250_port *up)
 		serial_out(up, UART_IER, 0);
 }
 
+static inline void serial8250_clear_IER(struct uart_8250_port *up)
+{
+	__serial8250_clear_IER(up);
+}
+
 #ifdef CONFIG_SERIAL_8250_RSA
 /*
  * Attempts to turn on the RSA FIFO.  Returns zero on failure.
@@ -3269,6 +3285,11 @@  static void serial8250_console_putchar(struct uart_port *port, unsigned char ch)
 
 	wait_for_xmitr(up, UART_LSR_THRE);
 	serial_port_out(port, UART_TX, ch);
+
+	if (ch == '\n')
+		up->console_newline_needed = false;
+	else
+		up->console_newline_needed = true;
 }
 
 /*
@@ -3297,6 +3318,7 @@  static void serial8250_console_restore(struct uart_8250_port *up)
 	serial8250_out_MCR(up, up->mcr | UART_MCR_DTR | UART_MCR_RTS);
 }
 
+#ifdef USE_SERIAL_8250_LEGACY_CONSOLE
 /*
  * Print a string to the serial port using the device FIFO
  *
@@ -3355,7 +3377,7 @@  void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	 *	First save the IER then disable the interrupts
 	 */
 	ier = serial_port_in(port, UART_IER);
-	serial8250_clear_IER(up);
+	__serial8250_clear_IER(up);
 
 	/* check scratch reg to see if port powered off during system sleep */
 	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
@@ -3421,6 +3443,125 @@  void serial8250_console_write(struct uart_8250_port *up, const char *s,
 	if (locked)
 		uart_port_unlock_irqrestore(port, flags);
 }
+#else
+void serial8250_console_write_thread(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt)
+{
+	struct uart_8250_em485 *em485 = up->em485;
+	struct uart_port *port = &up->port;
+	unsigned int ier;
+
+	touch_nmi_watchdog();
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	/* First save IER then disable the interrupts. */
+	ier = serial_port_in(port, UART_IER);
+	serial8250_clear_IER(up);
+
+	/* Check scratch reg if port powered off during system sleep. */
+	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
+		serial8250_console_restore(up);
+		up->canary = 0;
+	}
+
+	if (em485) {
+		if (em485->tx_stopped)
+			up->rs485_start_tx(up);
+		mdelay(port->rs485.delay_rts_before_send);
+	}
+
+	if (nbcon_exit_unsafe(wctxt)) {
+		int len = READ_ONCE(wctxt->len);
+		int i;
+
+		/*
+		 * Write out the message. Toggle unsafe for each byte in order
+		 * to give another (higher priority) context the opportunity
+		 * for a friendly takeover. If such a takeover occurs, this
+		 * must abort writing since wctxt->outbuf and wctxt->len are
+		 * no longer valid.
+		 */
+		for (i = 0; i < len; i++) {
+			if (!nbcon_enter_unsafe(wctxt))
+				break;
+
+			uart_console_write(port, wctxt->outbuf + i, 1, serial8250_console_putchar);
+
+			if (!nbcon_exit_unsafe(wctxt))
+				break;
+		}
+	}
+
+	/*
+	 * If ownership was lost, this context must reacquire ownership in
+	 * order to perform final actions (such as re-enabling interrupts).
+	 */
+	while (!nbcon_enter_unsafe(wctxt))
+		nbcon_reacquire_nobuf(wctxt);
+
+	/* Finally, wait for transmitter to become empty and restore IER. */
+	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	if (em485) {
+		mdelay(port->rs485.delay_rts_after_send);
+		if (em485->tx_stopped)
+			up->rs485_stop_tx(up);
+	}
+	serial_port_out(port, UART_IER, ier);
+
+	/*
+	 * The receive handling will happen properly because the receive ready
+	 * bit will still be set; it is not cleared on read.  However, modem
+	 * control will not, we must call it if we have saved something in the
+	 * saved flags while processing with interrupts off.
+	 */
+	if (up->msr_saved_flags)
+		serial8250_modem_status(up);
+
+	nbcon_exit_unsafe(wctxt);
+}
+
+void serial8250_console_write_atomic(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt)
+{
+	struct uart_port *port = &up->port;
+	unsigned int ier;
+
+	/* Atomic console not supported for rs485 mode. */
+	if (WARN_ON_ONCE(up->em485))
+		return;
+
+	touch_nmi_watchdog();
+
+	if (!nbcon_enter_unsafe(wctxt))
+		return;
+
+	/*
+	 * First save IER then disable the interrupts. The special variant to
+	 * clear IER is used because atomic printing may occur without holding
+	 * the port lock.
+	 */
+	ier = serial_port_in(port, UART_IER);
+	__serial8250_clear_IER(up);
+
+	/* Check scratch reg if port powered off during system sleep. */
+	if (up->canary && (up->canary != serial_port_in(port, UART_SCR))) {
+		serial8250_console_restore(up);
+		up->canary = 0;
+	}
+
+	if (up->console_newline_needed)
+		uart_console_write(port, "\n", 1, serial8250_console_putchar);
+	uart_console_write(port, wctxt->outbuf, wctxt->len, serial8250_console_putchar);
+
+	/* Finally, wait for transmitter to become empty and restore IER. */
+	wait_for_xmitr(up, UART_LSR_BOTH_EMPTY);
+	serial_port_out(port, UART_IER, ier);
+
+	nbcon_exit_unsafe(wctxt);
+}
+#endif /* USE_SERIAL_8250_LEGACY_CONSOLE */
 
 static unsigned int probe_baud(struct uart_port *port)
 {
@@ -3439,6 +3580,7 @@  static unsigned int probe_baud(struct uart_port *port)
 
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 {
+	struct uart_8250_port *up = up_to_u8250p(port);
 	int baud = 9600;
 	int bits = 8;
 	int parity = 'n';
@@ -3448,6 +3590,8 @@  int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
 
+	up->console_newline_needed = false;
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else if (probe)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index e0717c8393d7..a968e6941237 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -153,6 +153,8 @@  struct uart_8250_port {
 #define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
 	unsigned char		msr_saved_flags;
 
+	bool			console_newline_needed;
+
 	struct uart_8250_dma	*dma;
 	const struct uart_8250_ops *ops;
 
@@ -204,6 +206,10 @@  void serial8250_init_port(struct uart_8250_port *up);
 void serial8250_set_defaults(struct uart_8250_port *up);
 void serial8250_console_write(struct uart_8250_port *up, const char *s,
 			      unsigned int count);
+void serial8250_console_write_atomic(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt);
+void serial8250_console_write_thread(struct uart_8250_port *up,
+				     struct nbcon_write_context *wctxt);
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe);
 int serial8250_console_exit(struct uart_port *port);