Re: [PATCH v3 3/3] MIPS: CI20: Enable SMP with two CPUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] mips: ingenic: invalid lock nesting in xburst_boot_secondary()
- [High] mips: ingenic: boot BUG_ON in mips_smp_ipi_allocate() and
  missing present mask
- [High] mips: ingenic: missing cpu_set_core() skips cache IPIs under
  CONFIG_MIPS_CPS
- [Low] mips: ingenic: 64-bit build failure in xburst_smp_setup() and
  entry asm
--

commit 2540a85d35c671f5a1df0622376784adb3fe4123
Author: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>

MIPS: CI20: Enable SMP with two CPUs

This commit enables SMP support with two CPUs in ci20_defconfig so both
JZ4780 cores are used on the CI20 board.

Link: https://lists.openwall.net/linux-kernel/2020/05/19/739

> diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
> index 90536cab417ee..09d65094b1bd2 100644
> --- a/arch/mips/configs/ci20_defconfig
> +++ b/arch/mips/configs/ci20_defconfig
> @@ -22,6 +22,8 @@ CONFIG_EXPERT=y
>  CONFIG_MACH_INGENIC_SOC=y
>  CONFIG_JZ4780_CI20=y
>  CONFIG_HIGHMEM=y
> +CONFIG_SMP=y
> +CONFIG_NR_CPUS=2
>  CONFIG_HZ_100=y
>  # CONFIG_SUSPEND is not set
>  # CONFIG_SECCOMP is not set

[Severity: High]
Does xburst_boot_secondary() in commit "MIPS: Ingenic: Add XBurst SMP
support" violate lock nesting rules by calling clk_enable(cpu1_clock)
while holding smp_lock?

arch/mips/ingenic/ingenic-smp.c:xburst_boot_secondary() {
    ...
	raw_spin_lock_irqsave(&smp_lock, flags);
	ctrl = read_c0_corectrl() | CORECTRL_SWRST(1) | CORECTRL_RPC(1);
	write_c0_corectrl(ctrl);
	back_to_back_c0_hazard();

	ret = clk_enable(cpu1_clock);
    ...
}

smp_lock is a raw_spinlock_t, whereas clk_enable() -> clk_enable_lock()
acquires the non-raw spinlock_t enable_lock and jz4780_core1_enable()
acquires the non-raw spinlock_t cgu->lock.  Booting CPU1 triggers a
lockdep invalid wait context splat and sleeps while atomic on preempt-rt
kernels, while also holding smp_lock with interrupts disabled across the
5000 us power-status poll in jz4780_core1_enable().

[Severity: Low]
Can arch/mips/ingenic/ingenic-smp.c and
arch/mips/ingenic/ingenic-smp-entry.S in commit "MIPS: Ingenic: Add XBurst
SMP support" fail to build on 64-bit generic kernels?

arch/mips/ingenic/ingenic-smp.c:xburst_smp_setup() {
    ...
	entry = KSEG1ADDR((unsigned long)xburst_secondary_cpu_entry);
	write_c0_reim((reim & ~REIM_ENTRY) | (entry & REIM_ENTRY));
}

arch/mips/ingenic/ingenic-smp-entry.S:xburst_secondary_cpu_entry() {
    ...
	li	t0, KSEG0
    ...
	la	t0, xburst_cpu_entry_sp
	lw	sp, 0(t0)
	la	t0, xburst_cpu_entry_gp
	lw	gp, 0(t0)

	la	t0, smp_bootstrap
}

BOARD_INGENIC can be enabled in a 64-bit generic kernel build, where
asm/addrspace.h only defines CKSEG1ADDR and CKSEG0 (not KSEG1ADDR or
KSEG0), and the 32-bit la and lw instructions in
xburst_secondary_cpu_entry fail to assemble against 64-bit unsigned long
and pointer symbols instead of using PTR_LA and LONG_L.

[Severity: High]
Does xburst_smp_setup() in commit "MIPS: Ingenic: Add XBurst SMP support"
leave CPU1 out of cpu_present_mask and trigger a boot crash in
mips_smp_ipi_allocate() on generic kernels?

arch/mips/ingenic/ingenic-smp.c:xburst_smp_setup() {
	for_each_of_cpu_node(np) {
    ...
		__cpu_number_map[hwid] = cpu;
		__cpu_logical_map[cpu] = hwid;
		set_cpu_possible(cpu, true);
		cpu++;
	}
}

Neither xburst_smp_setup() nor xburst_prepare_cpus() calls
set_cpu_present(cpu, true), and smp_prepare_cpus() only copies
cpu_possible_mask to cpu_present_mask when CPU hotplug is disabled, so
CPU1 is never brought up when hotplug is enabled.  In addition, generic
MIPS kernels select GENERIC_IRQ_IPI, which causes mips_smp_ipi_init() ->
mips_smp_ipi_allocate() to hit BUG_ON(num_present_cpus() > 1) at boot
because JZ4780 uses custom mailbox callbacks in xburst_smp_ops without
registering an IPI irq domain.

[Severity: High]
Does xburst_smp_setup() in commit "MIPS: Ingenic: Add XBurst SMP support"
miss calling cpu_set_core(&cpu_data[cpu], hwid)?

When MIPS_CPS is enabled (such as in generic_defconfig), decode_configs()
skips cpu_set_core(), leaving both JZ4780 cores with core ID 0.
cpus_are_siblings(0, 1) then returns true and leaves cpu_foreign_map
empty, causing r4k_on_each_cpu() to skip cross-CPU L1 I-cache and D-cache
flush IPIs between the two cores' non-coherent private caches.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112041422.2997275-1-f.fainelli@gmail.com?part=3