Message-ID: <aEApOPTqbVOR35F_@pathway.suse.cz>
Date: Wed, 4 Jun 2025 13:08:40 +0200
From: Petr Mladek <pmladek@...e.com>
To: "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com>
Cc: 'Michael Kelley' <mhklinux@...look.com>,
	'John Ogness' <john.ogness@...utronix.de>,
	'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>
Subject: Re: Problem with nbcon console and amba-pl011 serial port

On Wed 2025-06-04 04:11:10, Toshiyuki Sato (Fujitsu) wrote:
> > On 2025-06-03, John Ogness <john.ogness@...utronix.de> wrote:
> > > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@...itsu.com> wrote:
> > >>> 4. pr_emerg() has a high logging level, and it effectively steals the console
> > >>> from the "pr/ttyAMA0" task, which I believe is intentional in the nbcon
> > design.
> > >>> Down in pl011_console_write_thread(), the "pr/ttyAMA0" task is doing
> > >>> nbcon_enter_unsafe() and nbcon_exit_unsafe() around each character
> > >>> that it outputs.  When pr_emerg() steals the console, nbcon_exit_unsafe()
> > >>> returns 0, so the "for" loop exits. pl011_console_write_thread() then
> > >>> enters a busy "while" loop waiting to reclaim the console. It's doing this
> > >>> busy "while" loop with interrupts disabled, and because of the panic,
> > >>> it never succeeds.

I am a bit surprised that it never succeeds. The panic CPU takes over
the ownership but it releases it when the messages are flushed. And
the original owner should be able to reacquire it in this case.

But maybe, this does not work well on virtualized systems when the
virtualized CPUs do not run at the same time.

> > >>> Whatever CPU is running "pr/ttyAMA0" is effectively
> > >>> stuck at this point.
> > >>>
> > >>> 5. Meanwhile panic() continues, calling panic_other_cpus_shutdown(). On
> > >>> ARM64, other CPUs are stopped by sending them an IPI. Each CPU receives
> > >>> the IPI and calls the PSCI function to stop itself. But the CPU running
> > >>> "pr/ttyAMA0" is looping forever with interrupts disabled, so it never
> > >>> processes the IPI and it never stops. ARM64 doesn't have a true NMI that
> > >>> can override the looping with interrupts disabled, so there's no way to
> > >>> stop that CPU.
> > >>>
> > >>> 6. The failure to stop the "pr/ttyAMA0" CPU then causes downstream
> > >>> problems, such as when loading and running a kdump kernel.

Thanks a lot for this great analyze. It makes sense. It must have been
hard to put all the pieces together.

> > >
> > > [...]
> > >

> This is a proposed fix to force termination by returning false from
> nbcon_reacquire_nobuf when a panic occurs within pl011_console_write_thread.
>  (I believe this is similar to what John suggested in his previous reply.)
> 
> While I couldn't reproduce the issue using sysrq-trigger in my environment
>  (It seemed that the panic was being executed before the thread processing),
> I did observe nbcon_reacquire_nobuf failing to complete when injecting an
> NMI (SError) during pl011_console_write_thread. 
> Applying this fix seems to have resolved the "SMP: failed to stop secondary
> CPUs" issue.
> 
> This patch is for test.
> Modifications to imx and other drivers, as well as adding __must_check, 
> will likely be required.
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 11d650975..c3a2b22e6 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2577,8 +2577,10 @@ pl011_console_write_thread(struct console *co, struct nbcon_write_context *wctxt
>  		}
>  	}
>  
> -	while (!nbcon_enter_unsafe(wctxt))
> -		nbcon_reacquire_nobuf(wctxt);
> +	while (!nbcon_enter_unsafe(wctxt)) {
> +		if (!nbcon_reacquire_nobuf(wctxt))
> +			return;
> +	}
>  
>  	while ((pl011_read(uap, REG_FR) ^ uap->vendor->inv_fr) & uap->vendor->fr_busy)
>  		cpu_relax();
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 8f10d0a85..ae278b875 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -604,14 +604,14 @@ extern void nbcon_cpu_emergency_exit(void);
>  extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
>  extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
>  extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
> -extern void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
> +extern bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt);
>  #else
>  static inline void nbcon_cpu_emergency_enter(void) { }
>  static inline void nbcon_cpu_emergency_exit(void) { }
>  static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
>  static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
>  static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
> -static inline void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
> +static inline bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt) { }
>  #endif
>  
>  extern int console_set_on_cmdline;
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index fd12efcc4..ec016bb24 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -909,14 +909,18 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
>   * output buffer because that has been lost. This function cannot be used to
>   * resume printing.
>   */

Please, add a comment explaining the new return value. Something
like:

 * Return:	True when the context reacquired the owner ship. The caller
 *		might try entering the unsafe state and restore the original
 *		console device setting. It just must not access the output
 *		buffer anymore.
 *
 *		False when another CPU is in panic() and a busy waiting for
 *		the ownership is not safe. It might prevent stopping this
 *		CPU and create further complications, especially on virtualized
 *		systems without proper NMI. The caller should bail out
 *		immediately without touching the console device or
 *		the output buffer.

It is very long. But I think that a good explanation might safe people
troubles in the future.

> -void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
> +bool nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
>  {
>  	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
>  
> -	while (!nbcon_context_try_acquire(ctxt))
> +	while (!nbcon_context_try_acquire(ctxt)) {
>  		cpu_relax();
> +		if (other_cpu_in_panic())
> +			return false;
> +	}
>  
>  	nbcon_write_context_set_buf(wctxt, NULL, 0);

It would make sense to set the NULL buffer even when returning "false".

> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(nbcon_reacquire_nobuf);

Powered by blists - more mailing lists