Re: [PATCH v2] MIPS: SGI-IP27: print the NMI dump on an nbcon console

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] printk: NMI dump output lost due to nbcon timeout if console is in unsafe state
--

commit f9358977a85eff3b02febdb716b027b3e2f5107f
Author: Imre Kaloz <kaloz@kernel.org>

MIPS: SGI-IP27: print the NMI dump on an nbcon console

Since the 8250 console became nbcon, printk() in nmi_dump() only queues
records for a printer thread that never runs, as no CPU leaves the NMI
handler. Print from an emergency section and flush pending records
before the hub reset.

> diff --git a/arch/mips/sgi-ip27/ip27-nmi.c b/arch/mips/sgi-ip27/ip27-nmi.c
> index fc2816398d0cf..c502982621e6b 100644
> --- a/arch/mips/sgi-ip27/ip27-nmi.c
> +++ b/arch/mips/sgi-ip27/ip27-nmi.c
> @@ -183,6 +184,12 @@ static void nmi_dump(void)
>  	 */
>  	arch_spin_lock(&nmi_lock);

[Severity: High]
If another CPU was interrupted while holding an nbcon console in an unsafe
state, will it be trapped here spinning indefinitely on nmi_lock?

If so, it would never be able to release the console.

>  
> +	/*
> +	 * No CPU leaves the NMI handler before the hub reset below, so an
> +	 * nbcon console's printer thread would never print the dump.
> +	 */
> +	nbcon_cpu_emergency_enter();

[Severity: High]
Does entering EMERGENCY priority here instead of PANIC prevent a hostile
takeover of the console?

Hostile unsafe takeovers require panic_on_this_cpu() to be true via
nbcon_allow_unsafe_takeover().

Since panic() is not called during this NMI dump sequence, it appears this
would fall short of allowing a takeover if the console is locked.

> +
>  #ifdef REAL_NMI_SIGNAL
>  	/*

[ ... ]

> @@ -227,5 +234,6 @@ static void nmi_dump(void)
>  	 * Save the nmi cpu registers for all cpu in the eframe format.
>  	 */
>  	nmi_eframes_save();
> +	printk_trigger_flush();

[Severity: High]
Will this flush completely suppress the dump output if the console was held
by one of the trapped CPUs?

If the system attempts a handover here, it will wait 2000us and time out
because the other CPU is trapped on nmi_lock.

Following the timeout, nbcon_context_try_acquire_hostile() will fail with
-EPERM because PANIC priority was not established.

Could this cause the flush to fail entirely, silently triggering a hardware
reset via LOCAL_HUB_S() without outputting any diagnostic information?

>  	LOCAL_HUB_S(NI_PORT_RESET, NPR_PORTRESET | NPR_LOCALRESET);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001191938.3256-1-kaloz@kernel.org?part=1