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

Toshiyuki Sato (Fujitsu)
2025-06-04T04:12:27Z

Hi Michael, John,

> Hi Petr,
>
> On 2025-06-03, John Ogness <john.ogness@linutronix.de> wrote:
> > On 2025-06-03, "Toshiyuki Sato (Fujitsu)" <fj6611ie@fujitsu.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. 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.
> >
> > [...]
> >
> >> After reproducing the issue,
> >> I plan to try a workaround that forcibly terminates the nbcon_reacquire_nobuf
> >> loop in pl011_console_write_thread if other_cpu_in_panic is true.
> >> Please comment if you have any other ideas.
> >
> > For panic, if it is OK to leave uap->clk enabled and not restore REG_CR,
> > then it should be fine to just return. But only for panic.
> >
> > So something like:
> >
> > while (!nbcon_enter_unsafe(wctxt)) {
> > if (other_cpu_in_panic())
> > return;
> > nbcon_reacquire_nobuf(wctxt);
> > }
>
> Actually this is not enough because there is also a loop inside
> nbcon_reacquire_nobuf().
>
> nbcon_reacquire_nobuf() needs to return an error for the panic case
> because it will never succeed. This is the only case where it will never
> succeed. Should we use a bool? Or return some code like -EPERM?
>
> So the above code becomes:
>
> while (!nbcon_enter_unsafe(wctxt)) {
> if (!nbcon_reacquire_nobuf(wctxt))
> return;
> }
>
> We should also add __must_check to the prototype.
>
> Thoughts?
>
> John

Thanks for the suggestion, John.

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.

Michael, could you please test this fix in your environment?

Regards,
Toshiyuki Sato

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.
*/
-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);
+ return true;
}
EXPORT_SYMBOL_GPL(nbcon_reacquire_nobuf);