[PATCH v2 0/2] serial: amba-pl011: fix console clock lifetime

[PATCH v2 0/2] serial: amba-pl011: fix console clock lifetime

[Karl Mehltretter]

Patch 1 fixes an independent bug: pl011_console_setup() prepares the UART
clock but nothing releases it when the console is unregistered, so the
clock's prepare count leaks one reference per unregister. It adds the
missing console .exit() and stands on its own.

Patch 2 fixes a PREEMPT_RT failure: pl011_console_write_atomic() runs in
nbcon atomic context but calls clk_enable(), which under RT can acquire a
sleeping lock. It keeps the clock enabled while the console is available
for output and releases it in .exit(); it depends on patch 1.

Changes in v2:
- Add suspend/resume management for the persistent clock reference.
- Keep the reference for no_console_suspend and RX-wakeup ports.
- Document the platform-dependent power impact.

Patch 1 is unchanged.

Tested in QEMU on raspi1ap, raspi2b, and RealView ARMv6 SMP, and on arm64
virt under PREEMPT_RT. Actual s2idle suspend/resume (RTC-wake) dropped and
restored the PL011 clock reference, balanced across repeated cycles, while
wake-capable and no_console_suspend cases kept it enabled. Strict clock
checking found no access while stopped, and RX behaved as expected.
Toggling /sys/class/tty/ttyAMA0/console showed the prepare-count leak
without patch 1 and a balanced count with it.

Full-platform-sleep and actual RX-wakeup were not exercised.

Karl Mehltretter (2):
  serial: amba-pl011: unprepare console clock on unregister
  serial: amba-pl011: keep console clock enabled for atomic writes

 drivers/tty/serial/amba-pl011.c | 53 ++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 11 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] serial: amba-pl011: unprepare console clock on unregister

[Karl Mehltretter]

pl011_console_setup() calls clk_prepare() on the UART clock, but the
console provides no matching teardown, so the clock is never unprepared
when the console is unregistered -- via the sysfs "console" attribute or
a driver unbind. Each re-registration prepares the clock again, leaking
one prepare reference per cycle.

Even where preparing the clock has no hardware effect, the stale
reference leaves the clock framework's prepare count unbalanced. For
providers with prepare/unprepare operations or runtime-PM integration,
it may also retain resources after the console is unregistered.

Add a console .exit() callback that clk_unprepare()s the clock,
balancing the clk_prepare() in pl011_console_setup().

Fixes: 4b4851c65d92 ("clk: amba-pl011: convert to clk_prepare()/clk_unprepare()")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/tty/serial/amba-pl011.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8ed91e1da22b..1aa43994a3cd 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2552,6 +2552,15 @@ static int pl011_console_setup(struct console *co, char *options)
 	return uart_set_options(&uap->port, co, baud, parity, bits, flow);
 }
 
+static int pl011_console_exit(struct console *co)
+{
+	struct uart_amba_port *uap = amba_ports[co->index];
+
+	clk_unprepare(uap->clk);
+
+	return 0;
+}
+
 /**
  *	pl011_console_match - non-standard console matching
  *	@co:	  registering console
@@ -2705,6 +2714,7 @@ static struct console amba_console = {
 	.name		= "ttyAMA",
 	.device		= uart_console_device,
 	.setup		= pl011_console_setup,
+	.exit		= pl011_console_exit,
 	.match		= pl011_console_match,
 	.write_atomic	= pl011_console_write_atomic,
 	.write_thread	= pl011_console_write_thread,
-- 
2.53.0

Re: [PATCH v2 1/2] serial: amba-pl011: unprepare console clock on unregister

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing NULL check for `uap` in `pl011_console_exit()` can lead to a NULL pointer dereference.
--

--- Patch [1]: [PATCH v2 1/2] serial: amba-pl011: unprepare console clock on unregister ---
commit 52f009641a6d04998b843ae51d6c555711f29559
Author: Karl Mehltretter <kmehltretter@gmail.com>

serial: amba-pl011: unprepare console clock on unregister

This commit adds a console .exit() callback to balance the clock prepare
count. It ensures that the UART clock is unprepared when the console is
unregistered, preventing stale reference leaks.

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8ed91e1da22be..1aa43994a3cdc 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2552,6 +2552,15 @@ static int pl011_console_setup(struct console *co, char *options)
>  	return uart_set_options(&uap->port, co, baud, parity, bits, flow);
>  }
>  
> +static int pl011_console_exit(struct console *co)
> +{
> +	struct uart_amba_port *uap = amba_ports[co->index];
> +
> +	clk_unprepare(uap->clk);

[Severity: High]
Is it possible for uap to be NULL here in pl011_console_exit() within
drivers/tty/serial/amba-pl011.c?

During device probe, uart_add_one_port() registers the console via
uart_configure_port(). If a subsequent operation in
serial_core_add_one_port() fails (for example, allocating uport->tty_groups
returns -ENOMEM), the error path returns the error but does not unregister
the console.

The driver then cleans up by calling pl011_unregister_port(), which clears
the amba_ports array entry by setting amba_ports[co->index] = NULL.

The console remains globally registered. If this console is eventually
unregistered (such as when displaced by a preferred console or via sysfs
unbind), pl011_console_exit() is invoked. It unconditionally assigns uap
and dereferences it, leading to a kernel panic. Other console callbacks like
pl011_console_setup() correctly check for a NULL uap before proceeding.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724213348.77418-1-kmehltretter@gmail.com?part=1

Re: [PATCH v2 1/2] serial: amba-pl011: unprepare console clock on unregister

[Karl Mehltretter]

On Fri, Jul 24, 2026 at 09:55:47PM +0100, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Is it possible for uap to be NULL here in pl011_console_exit() within
> drivers/tty/serial/amba-pl011.c?
> 

The root cause is in serial core: serial_core_add_one_port() registers the
console (uart_configure_port()) before the fallible tty_groups allocation,
so a failure there leaves the console registered after the driver has
cleared amba_ports[]. A NULL check in exit() alone would hide one of
several symptom sites and change nothing on the write path.

This is fixed for all console UART drivers by a separate series [1],
"serial: fix console lifetime bugs on failed bind and removal" (1/4 moves
the fallible allocation before console registration); it reproduces this
exact pl011_console_write_thread oops and an i.MX use-after-free. So I'd
keep this series as-is.

[1] https://lore.kernel.org/all/20260719221014.44354-1-kmehltretter@gmail.com/

[PATCH v2 2/2] serial: amba-pl011: keep console clock enabled for atomic writes

[Karl Mehltretter]

pl011_console_write_atomic() runs from nbcon atomic context, where
sleeping is not allowed. It calls clk_enable(), which takes the common-clk
enable_lock. Under PREEMPT_RT that is a sleeping lock:
clk_enable_lock() first tries spin_trylock_irqsave(), but on contention
falls back to spin_lock_irqsave(). Therefore, an atomic-context printk on
an RT kernel with a clk-backed pl011 can trip:

  BUG: sleeping function called from invalid context at spinlock_rt.c:48
    __might_resched from rt_spin_lock
    rt_spin_lock from clk_enable_lock
    clk_enable_lock from clk_enable
    clk_enable from pl011_console_write_atomic
    ... from vprintk_emit

This was found and reproduced on PREEMPT_RT. Arm32 and arm64 DT SoCs are
affected; arm64 SBSA/ACPI has no clk, so clk_enable(NULL) short-circuits
before the lock. In addition, write_atomic() may be invoked from NMI
context and is documented to avoid locking. Removing clk_enable() from
the callback also avoids a potentially unsafe NMI acquisition of the
common-clock enable_lock.

An nbcon atomic-capable console must be printable from any context, so
the clock cannot be gated between writes. Enable the clock while the
console is available for output: use clk_prepare_enable() in
pl011_console_setup(), release it via clk_disable_unprepare() in the
console .exit() callback, and drop the per-write clk_enable()/clk_disable()
pairs from write_atomic() and write_thread().

When printk suspends consoles, drop the reference after
uart_suspend_port() stops console access and restore it before
uart_resume_port() -- but only if suspend actually marked the port
suspended (a wake-capable tty stays running and must keep its clock), and
keep it when console_suspend_enabled is false so no_console_suspend works.

The active power cost of keeping the clock enabled is platform-dependent:
none where the UART clock is a fixed always-on oscillator, real where it
is a gateable clock branch, which then cannot be gated (nor possibly can
its parent clocks) while the console is available for output. When serial
core actually suspends the port, the reference is released so the clock
provider can gate the clock tree.

Fixes: 2eb2608618ce ("serial: amba-pl011: Implement nbcon console")
Suggested-by: John Ogness <john.ogness@linutronix.de>
Link: https://lore.kernel.org/all/8733xeaxix.fsf@jogness.linutronix.de/
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/tty/serial/amba-pl011.c | 40 +++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 1aa43994a3cd..611d1f6a0590 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2523,7 +2523,7 @@ static int pl011_console_setup(struct console *co, char *options)
 	/* Allow pins to be muxed in and configured */
 	pinctrl_pm_select_default_state(uap->port.dev);
 
-	ret = clk_prepare(uap->clk);
+	ret = clk_prepare_enable(uap->clk);
 	if (ret)
 		return ret;
 
@@ -2556,7 +2556,7 @@ static int pl011_console_exit(struct console *co)
 {
 	struct uart_amba_port *uap = amba_ports[co->index];
 
-	clk_unprepare(uap->clk);
+	clk_disable_unprepare(uap->clk);
 
 	return 0;
 }
@@ -2630,8 +2630,6 @@ pl011_console_write_atomic(struct console *co, struct nbcon_write_context *wctxt
 	if (!nbcon_enter_unsafe(wctxt))
 		return;
 
-	clk_enable(uap->clk);
-
 	if (!uap->vendor->always_enabled) {
 		old_cr = pl011_read(uap, REG_CR);
 		pl011_write((old_cr & ~UART011_CR_CTSEN) | (UART01x_CR_UARTEN | UART011_CR_TXE),
@@ -2648,8 +2646,6 @@ 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);
-
 	nbcon_exit_unsafe(wctxt);
 }
 
@@ -2662,8 +2658,6 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
 	if (!nbcon_enter_unsafe(wctxt))
 		return;
 
-	clk_enable(uap->clk);
-
 	if (!uap->vendor->always_enabled) {
 		old_cr = pl011_read(uap, REG_CR);
 		pl011_write((old_cr & ~UART011_CR_CTSEN) | (UART01x_CR_UARTEN | UART011_CR_TXE),
@@ -2692,8 +2686,6 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
 	if (!uap->vendor->always_enabled)
 		pl011_write(old_cr, uap, REG_CR);
 
-	clk_disable(uap->clk);
-
 	nbcon_exit_unsafe(wctxt);
 }
 
@@ -3080,21 +3072,45 @@ static void pl011_remove(struct amba_device *dev)
 static int pl011_suspend(struct device *dev)
 {
 	struct uart_amba_port *uap = dev_get_drvdata(dev);
+	int ret;
 
 	if (!uap)
 		return -EINVAL;
 
-	return uart_suspend_port(&amba_reg, &uap->port);
+	ret = uart_suspend_port(&amba_reg, &uap->port);
+	if (ret)
+		return ret;
+
+	if (console_suspend_enabled && uap->port.suspended &&
+	    uart_console_registered(&uap->port))
+		clk_disable_unprepare(uap->clk);
+
+	return 0;
 }
 
 static int pl011_resume(struct device *dev)
 {
 	struct uart_amba_port *uap = dev_get_drvdata(dev);
+	bool resume_console;
+	int ret;
 
 	if (!uap)
 		return -EINVAL;
 
-	return uart_resume_port(&amba_reg, &uap->port);
+	resume_console = console_suspend_enabled &&
+			 uap->port.suspended &&
+			 uart_console_registered(&uap->port);
+	if (resume_console) {
+		ret = clk_prepare_enable(uap->clk);
+		if (ret)
+			return ret;
+	}
+
+	ret = uart_resume_port(&amba_reg, &uap->port);
+	if (ret && resume_console)
+		clk_disable_unprepare(uap->clk);
+
+	return ret;
 }
 #endif
 
-- 
2.39.5 (Apple Git-154)

RE: [PATCH v2 2/2] serial: amba-pl011: keep console clock enabled for atomic writes

[Toshiyuki Sato (Fujitsu)]

Hi Karl,

Regarding Petr's comment [1] as well, I'm concerned about the potential impact when the clk remains enabled during periods without console output.
When creating nbcon patch, I saw a similar patchh [2] from the past.
Have you considered coordinating with the clk subsystem implementation for this?

[1] https://lore.kernel.org/all/al4KdsU9YLOmwDoV@pathway.suse.cz/
[2] https://lore.kernel.org/all/1359475526-17523-1-git-send-email-walimisdev@gmail.com/

Thanks,
Toshiyuki Sato

RE: [PATCH v2 2/2] serial: amba-pl011: keep console clock enabled for atomic writes

[John Ogness]

Hi Toshiyuki,

On 2026-07-27, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.com> wrote:
> Regarding Petr's comment [1] as well, I'm concerned about the
> potential impact when the clk remains enabled during periods without
> console output.

Can you elaborate on your concerns? Do you actually need such low-power
_and_ kernel logging directly on serial?

> When creating nbcon patch, I saw a similar patch [2] from the past.
> Have you considered coordinating with the clk subsystem implementation
> for this?

AFAICT there was no real justification for enabling clocks per write
other than because we can. A lot has changed since 2013 and neither
spin_locks nor raw_spin_locks are appropriate because atomic printing
can occur in _any_ context (including NMI's).

If the amba-pl011 insists on enabling clocks per write, I would
recommend not implementing the write_atomic() callback. Since I assume a
significant amount of users _will_ want atomic printing support, perhaps
you can add a Kconfig to toggle building with clock-disabling and no
atomic, or clock-always-on and atomic.

Note that there is also CON_NBCON_ATOMIC_UNSAFE available, if the driver
wants to somehow blindly enable clocks on panic in order to unsafely
dump panic logs.

John Ogness

> [1] https://lore.kernel.org/all/al4KdsU9YLOmwDoV@pathway.suse.cz/
> [2] https://lore.kernel.org/all/1359475526-17523-1-git-send-email-walimisdev@gmail.com/