Re: [PATCH tty v8 1/2] serial: 8250: Switch to nbcon console, take 2

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-07-22 13:15: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.
> 
> All hardware access in the callbacks is within unsafe sections.
> The ->write_atomic() and ->write_thread() callbacks allow safe
> handover/takeover per byte and add a preceding newline if they
> take over from another context mid-line.
> 
> For the ->write_atomic() callback, a new irq_work is used to defer
> modem control since it may be called from a context that does not
> allow waking up tasks. During suspend/resume the irq_work is not
> used as this has been shown to cause suspend problems for some
> hardware. Upon resume, any pending modem control is performed.
> 
> Note: A new __serial8250_clear_IER() is introduced for direct
> clearing of UART_IER during console writing (which will not be
> holding the port lock for atomic printing or KDB/KGDB). This
> allows restoring a lockdep check to serial8250_clear_IER() in
> a follow-up commit.
> 
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -584,13 +609,19 @@ void serial8250_suspend_port(int line)
>  	struct uart_8250_port *up = &serial8250_ports[line];
>  	struct uart_port *port = &up->port;
>  
> -	if (!console_suspend_enabled && uart_console(port) &&
> -	    port->type != PORT_8250) {
> -		unsigned char canary = 0xa5;
> +	if (uart_console(port)) {
> +		/* No irq_work may be queued when suspending. */
> +		scoped_guard(uart_port_lock_irqsave, port)

Strictly speaking, it should be enough to call "uart_port_lock_irq"
instead of the "_irqsafe" variant.

This code must be called in a schedulable context. Otherwise,
we can't call irq_work_sync() below.

Honestly, I have been a bit lost in the maze of macros and callbacks.
And I wanted to be sure where this is called. So, I added WARN_ON(1)
and dumped all locks.

And it seems that we are on the safe side. This code is
called here:

[   78.676626] CPU: 7 UID: 0 PID: 1542 Comm: bash Kdump: loaded Not tainted 7.2.0-rc4-default+ #24 PREEMPT(full)  252cd4fa4cf2d3bc860af0a8bfee0a3e125ae7f8
[   78.676634] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-2-g4f253b9b-prebuilt.qemu.org 04/01/2014
[   78.676640] RIP: 0010:serial8250_suspend_port.cold+0x1f/0x1e4
[   78.676648] Code: dc e8 70 f6 02 00 e9 b3 28 df 00 53 48 c7 c7 d0 6d 0d 9f 48 83 ec 20 48 89 54 24 10 4c 89 4c 24 08 4c 89 04 24 e8 2c 1b fb ff <0f> 0b e8 d5 ee fa ff 48 8b 3c 24 e8 6c fa
 49 01 83 3d 79 0c 6b 02
[   78.676653] RSP: 0018:ffffd16d82743c10 EFLAGS: 00010246
[   78.676662] RAX: 0000000000000041 RBX: ffff8d8a00ab3800 RCX: 0000000000000001
[   78.676667] RDX: 0000000000000000 RSI: ffffffff9f1ae0d7 RDI: 00000000ffffffff
[   78.676672] RBP: 0000000000000001 R08: 0000000000000000 R09: 0000000000000001
[   78.676677] R10: 0000000000000007 R11: 0000000000000000 R12: ffffffff9f7f5a80
[   78.676681] R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000000
[   78.676689] FS:  00007fadbce4e200(0000) GS:ffff8d8ada0a0000(0000) knlGS:0000000000000000
[   78.676695] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   78.676699] CR2: 000055b5982aea10 CR3: 000000010cb6b000 CR4: 0000000000750ef0
[   78.676707] PKRU: 55555554
[   78.676712] Call Trace:
[   78.676718]  <TASK>
[   78.676732]  serial_pnp_suspend+0x11/0x20
[   78.676745]  __pnp_bus_suspend+0x3d/0x100
[   78.676761]  ? __pfx_pnp_bus_freeze+0x10/0x10
[   78.676770]  dpm_run_callback+0x66/0x1f0
[   78.676790]  device_suspend+0x235/0x700
[   78.676798]  ? srso_alias_return_thunk+0x5/0xfbef5
[   78.676807]  ? __lock_release.isra.0+0x1c9/0x300
[   78.676825]  ? __pfx_dpm_watchdog_handler+0x10/0x10
[   78.676851]  ? __mutex_unlock_slowpath+0x1a9/0x410
[   78.676874]  dpm_suspend+0x18f/0x400
[   78.676888]  hibernation_snapshot+0xcb/0x1e0
[   78.676908]  hibernate.cold+0xfd/0x48b
[   78.676925]  state_store+0xc3/0xd0
[   78.676940]  kernfs_fop_write_iter+0x163/0x240
[   78.676960]  vfs_write+0x222/0x560
[   78.676997]  ksys_write+0x70/0xf0
[   78.677012]  do_syscall_64+0xb1/0x6d0
[   78.677021]  ? srso_alias_return_thunk+0x5/0xfbef5
[   78.677036]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[   78.677043] RIP: 0033:0x7fadbc106af3

And the locks are:

[   78.677780] 7 locks held by bash/1542:
[   78.677784]  #0: ffff8d8a038a4470 (sb_writers#5){.+.+}-{0:0}, at: ksys_write+0x70/0xf0
[   78.677820]  #1: ffff8d8a08f44288 (&of->mutex#2){+.+.}-{4:4}, at: kernfs_fop_write_iter+0x105/0x240
[   78.677849]  #2: ffff8d8a00d9a230 (kn->active#105){.+.+}-{0:0}, at: kernfs_fop_write_iter+0x11a/0x240
[   78.677878]  #3: ffffffff9f48c0a8 (system_transition_mutex){+.+.}-{4:4}, at: lock_system_sleep+0x2b/0x40
[   78.677904]  #4: ffffffff9f804968 (device_hotplug_lock){+.+.}-{4:4}, at: hibernate.cold+0xac/0x48b
[   78.677930]  #5: ffff8d8a00ab3938 (&dev->mutex){....}-{4:4}, at: device_suspend+0x1ff/0x700
[   78.677955]  #6: ffffffff9f5ab1a0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire.constprop.0+0x7/0x30

, where the rcu_read_lock() is from debug_show_all_locks() itself.

The function serial8250_suspend_port() is called under dev->mutex
so we are on the safe side.


> +			up->console_msr_work_allow = false;
> +		irq_work_sync(&up->console_msr_work);
>  
> -		serial_out(up, UART_SCR, canary);
> -		if (serial_in(up, UART_SCR) == canary)
> -			up->canary = canary;
> +		if (!console_suspend_enabled && port->type != PORT_8250) {
> +			unsigned char canary = 0xa5;
> +
> +			serial_out(up, UART_SCR, canary);
> +			if (serial_in(up, UART_SCR) == canary)
> +				up->canary = canary;
> +		}
>  	}
>  
>  	uart_suspend_port(&serial8250_reg, port);
> @@ -620,6 +651,18 @@ void serial8250_resume_port(int line)
>  		port->uartclk = 921600*16;
>  	}
>  	uart_resume_port(&serial8250_reg, port);
> +
> +	if (uart_console(port)) {
> +
> +		guard(uart_port_lock_irqsave)(port);

This should be symetrical -> s/irqsave/irq

> +		/* irq_work allowed again. */
> +		up->console_msr_work_allow = true;
> +
> +		/* Handle any pending MSR changes. */
> +		if (up->msr_saved_flags)
> +			serial8250_modem_status(up);
> +	}
>  }
>  EXPORT_SYMBOL(serial8250_resume_port);


Plus the problem reported by Sashiko.

Otherwise, it looks good to me.

Best Regards,
Petr