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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <TY4PR01MB13777DAC71E234B9E58CC6C0DD76CA@TY4PR01MB13777.jpnprd01.prod.outlook.com>
Date: Wed, 4 Jun 2025 01:22:06 +0000
From: "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com>
To: 'John Ogness' <john.ogness@...utronix.de>
CC: 'Michael Kelley' <mhklinux@...look.com>, "pmladek@...e.com"
	<pmladek@...e.com>, 'Ryo Takakura' <ryotkkr98@...il.com>, Russell King
	<linux@...linux.org.uk>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-serial@...r.kernel.org"
	<linux-serial@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "Toshiyuki Sato (Fujitsu)"
	<fj6611ie@...itsu.com>
Subject: RE: Problem with nbcon console and amba-pl011 serial port

Hi John,

Thank you for your help with this issue.

> On 2025-06-03, Michael Kelley <mhklinux@...look.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

I believe the Common Clock Framework manages the enable count for clocks.
Specifically, uap->clk->core->enable_count is incremented by clk_enable
and decremented by clk_disable.
Wouldn't the clock remain enabled until enable_count reaches 0?

Regards, 
Toshiyuki Sato