RE: Problem with nbcon console and amba-pl011 serial port

[John Ogness <john.ogness@linutronix.de>]

On 2025-06-03, Michael Kelley <mhklinux@outlook.com> wrote:
> The problem is the failure to stop secondary CPU 2.  (The CPU # that fails
> to stop varies from run-to-run.) It is mostly reproducible, but not always. I
> bisected to commit 2eb2608618ce ("serial: amba-pl011: Implement nbcon
> console") in the 6.15 kernel.

Unrelated to this particular report, I am looking at commit 2eb2608618ce
("serial: amba-pl011: Implement nbcon console") and I do not think it
implements atomic printing correctly.

pl011_console_write_atomic() assumes uap->clk is disabled when it is
called. However, if it took over ownership from the printing kthread,
the uap->clk is already enabled. And then after printing its line it
disables uap->clk, even though the interrupted printing kthread expects
uap->clk to still be enabled once it regains ownership.

The atomic printing needs to track if the clock is enabled or disabled
and act accordingly. I suppose something like this:

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 11d65097578cd..914449b46b95b 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2520,11 +2520,14 @@ pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt
 {
 	struct uart_amba_port *uap = amba_ports[co->index];
 	unsigned int old_cr = 0;
+	bool old_enabled;
 
 	if (!nbcon_enter_unsafe(wctxt))
 		return;
 
-	clk_enable(uap->clk);
+	old_enabled = __clk_is_enabled(uap->clk);
+	if (!old_enabled)
+		clk_enable(uap->clk);
 
 	if (!uap->vendor->always_enabled) {
 		old_cr = pl011_read(uap, REG_CR);
@@ -2542,7 +2545,8 @@ pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt
 	if (!uap->vendor->always_enabled)
 		pl011_write(old_cr, uap, REG_CR);
 
-	clk_disable(uap->clk);
+	if (!old_enabled)
+		clk_disable(uap->clk);
 
 	nbcon_exit_unsafe(wctxt);
 }

I am guessing that it is allowed to use __clk_is_enabled() for this
purpose. Otherwise it can be tracked as a bool in struct uart_amba_port.

John Ogness