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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mips: ingenic: invalid lock nesting in jz4780_boot_secondary()
- [High] mips: ingenic: boot BUG_ON in mips_smp_ipi_allocate() and missing present mask
- [High] mips: ingenic: missing cpu_set_core() breaks cache IPIs under CONFIG_MIPS_CPS
- [Low] mips: ingenic: 64-bit build failure when CONFIG_MIPS_INGENIC_SMP=y
--

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

MIPS: Ingenic: Add JZ4780 SMP support to the generic platform

This commit ports JZ4780 secondary core reset entry and mailbox support to the
generic MIPS platform, registering Ingenic multi-processor operations during
device tree initialization.

Link: https://lkml.iu.edu/hypermail/linux/kernel/2005.2/03488.html
Link: https://lists.openwall.net/linux-kernel/2020/05/13/1482
Link: https://lists.openwall.net/linux-kernel/2020/05/19/1099

> diff --git a/arch/mips/generic/ingenic-smp.c b/arch/mips/generic/ingenic-smp.c
> new file mode 100644
> index 0000000000000..98c1fd7483db5
> --- /dev/null
> +++ b/arch/mips/generic/ingenic-smp.c
> @@ -0,0 +1,240 @@

[ ... ]

> +static struct clk *cpu1_clock;
> +static DEFINE_RAW_SPINLOCK(smp_lock);

[ ... ]

> +static void __init jz4780_smp_setup(void)
> +{
> +	struct device_node *np;
> +	u32 entry, reim;
> +	u64 hwid;
> +
> +	for_each_of_cpu_node(np) {
> +		hwid = of_get_cpu_hwid(np, 0);
> +		if (hwid >= nr_cpu_ids || hwid >= JZ4780_NR_CPUS ||
> +		    !of_device_is_available(np))
> +			continue;
> +		__cpu_number_map[hwid] = hwid;
> +		__cpu_logical_map[hwid] = hwid;
> +		set_cpu_possible(hwid, true);
> +	}

[Severity: High]
How does this behave when booting a generic kernel image with
CONFIG_MIPS_GENERIC_KERNEL=y and CONFIG_BOARD_INGENIC=y?

In jz4780_smp_setup(), discovered cores are marked possible with
set_cpu_possible(hwid, true), while neither jz4780_smp_setup() nor
jz4780_prepare_cpus() calls set_cpu_present(hwid, true). In
arch/mips/kernel/smp.c, smp_prepare_cpus() only populates cpu_present_mask
when CONFIG_HOTPLUG_CPU is disabled:

    void __init smp_prepare_cpus(unsigned int max_cpus)
    {
        ...
    #ifndef CONFIG_HOTPLUG_CPU
        init_cpu_present(cpu_possible_mask);
    #endif
        cpumask_copy(&cpu_coherent_mask, cpu_possible_mask);
    }

When CONFIG_HOTPLUG_CPU=y (such as in generic_defconfig), the secondary core
is never marked present and smp_init() will not bring it up.

On the other hand, when the secondary core is marked present,
CONFIG_MIPS_GENERIC_KERNEL selects CONFIG_MIPS_GIC and
CONFIG_SYS_SUPPORTS_MULTITHREADING, which select CONFIG_GENERIC_IRQ_IPI when
CONFIG_SMP=y. During do_pre_smp_initcalls(), mips_smp_ipi_init() calls
mips_smp_ipi_allocate(cpu_possible_mask):

    if (!ipidomain) {
        BUG_ON(num_present_cpus() > 1);
        return 0;
    }

Because JZ4780 uses its own mailbox handlers in jz4780_smp_ops rather than
registering a DOMAIN_BUS_IPI irq_domain, ipidomain is NULL. Won't this trigger
the BUG_ON(num_present_cpus() > 1) check at boot when running a generic kernel
with CONFIG_GENERIC_IRQ_IPI=y?

[Severity: High]
Also in jz4780_smp_setup() (and jz4780_init_secondary()), should
cpu_set_core(&cpu_data[hwid], hwid) be called so each core gets a distinct
core identifier?

In arch/mips/kernel/cpu-probe.c, decode_configs() only populates the core
number when CONFIG_MIPS_CPS is disabled:

    #ifndef CONFIG_MIPS_CPS
        if (cpu_has_mips_r2_r6) {
            unsigned int core;

            core = get_ebase_cpunum();
            if (cpu_has_mipsmt)
                core >>= fls(core_nvpes()) - 1;
            cpu_set_core(c, core);
        }
    #endif

When CONFIG_MIPS_CPS=y is enabled alongside CONFIG_MIPS_INGENIC_SMP=y (for
example in a generic kernel config), neither decode_configs() nor the Ingenic
platform code calls cpu_set_core(). Both cores then keep core number 0 in
cpu_data[cpu].globalnumber, causing cpus_are_siblings(0, 1) to return true
and calculate_cpu_foreign_map() to leave cpu_foreign_map[] empty.

In arch/mips/mm/c-r4k.c, r4k_op_needs_ipi() checks:

    #ifdef CONFIG_SMP
        return !cpumask_empty(&cpu_foreign_map[0]);
    #else
        return false;
    #endif

With cpu_foreign_map[0] empty, won't r4k_on_each_cpu() skip cross-core
instruction and data cache flush calls between the two cores even though they
have separate non-coherent primary caches?

[ ... ]

> +static int jz4780_boot_secondary(int cpu, struct task_struct *idle)
> +{
> +	unsigned long flags;
> +	u32 ctrl;
> +	int ret;
> +
> +	if (cpu_logical_map(cpu) != 1 || IS_ERR_OR_NULL(cpu1_clock))
> +		return -ENODEV;
> +
> +	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);
> +	if (ret)
> +		goto unlock;

[Severity: High]
Is it valid to call clk_enable(cpu1_clock) while holding the raw spinlock
smp_lock?

In jz4780_boot_secondary(), smp_lock is a raw_spinlock_t acquired via
raw_spin_lock_irqsave(&smp_lock, flags). Calling clk_enable(cpu1_clock) under
smp_lock enters:

    jz4780_boot_secondary()
      raw_spin_lock_irqsave(&smp_lock, flags)
      clk_enable(cpu1_clock)
        clk_core_enable_lock()
          clk_enable_lock()
            spin_lock_irqsave(&enable_lock, flags)
          clk_core_enable()
            jz4780_core1_enable()
              spin_lock_irqsave(&cgu->lock, flags)

Both enable_lock in drivers/clk/clk.c and cgu->lock in
drivers/clk/ingenic/jz4780-cgu.c are non-raw spinlock_t locks, and
jz4780_core1_enable() also busy-polls CGU_REG_LCR via
readl_poll_timeout_atomic() for up to 5000 us. Doesn't nesting non-raw
spinlock_t acquisitions inside a raw_spinlock_t critical section violate
lockdep wait-context rules (triggering an invalid wait context warning when
CONFIG_PROVE_RAW_LOCK_NESTING is enabled and sleeping while atomic on
PREEMPT_RT)?

[ ... ]

> diff --git a/arch/mips/ingenic/Kconfig b/arch/mips/ingenic/Kconfig
> index f595b339a4b84..1dfd0bfd01937 100644
> --- a/arch/mips/ingenic/Kconfig
> +++ b/arch/mips/ingenic/Kconfig
> @@ -64,6 +64,12 @@ config MACH_JZ4780
>  	select MIPS_CPU_SCACHE
>  	select SYS_HAS_CPU_MIPS32_R2
>  	select SYS_SUPPORTS_HIGHMEM
> +	select SYS_SUPPORTS_SMP
> +
> +config MIPS_INGENIC_SMP
> +	def_bool y
> +	depends on MACH_JZ4780 && SMP
> +	select SMP_UP

[Severity: Low]
Can CONFIG_MIPS_INGENIC_SMP be enabled on a 64-bit generic kernel build?

In arch/mips/generic/Kconfig, CONFIG_BOARD_INGENIC can be enabled under
CONFIG_MIPS_GENERIC_KERNEL with CONFIG_64BIT=y, which selects
CONFIG_MACH_INGENIC_GENERIC and CONFIG_MACH_JZ4780. When CONFIG_SMP=y is also
enabled, CONFIG_MIPS_INGENIC_SMP is automatically selected.

When CONFIG_64BIT=y, arch/mips/include/asm/addrspace.h does not define
KSEG1ADDR or KSEG0, so building arch/mips/generic/ingenic-smp.c fails in
jz4780_smp_setup():

    entry = KSEG1ADDR((unsigned long)jz4780_secondary_cpu_entry);

and assembling arch/mips/generic/ingenic-smp-entry.S fails in
jz4780_secondary_cpu_entry at KSEG0 and at the 32-bit la and lw instructions
used on jz4780_cpu_entry_sp, jz4780_cpu_entry_gp, and smp_bootstrap:

    li	t0, KSEG0
    ...
    la	t0, jz4780_cpu_entry_sp
    lw	sp, 0(t0)
    la	t0, jz4780_cpu_entry_gp
    lw	gp, 0(t0)

    la	t0, smp_bootstrap

Should MIPS_INGENIC_SMP (or BOARD_INGENIC) depend on 32BIT, or use
pointer-sized macros and CKSEG0 / CKSEG1ADDR?

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