Re: [PATCH v6 3/4] MIPS: Ingenic: Add XBurst SMP support

[Florian Fainelli <f.fainelli@gmail.com>]

On 11/13/26 11:40, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] mips: idle: cached need_resched() load after dcache flush in
>    xburst_wait_irqoff()
> - [Medium] MIPS: Ingenic: missing IRQF_NO_SUSPEND on mailbox IPI in
>    xburst_prepare_cpus()
> - [Low] mips: idle: xburst_wait_irqoff() calls non-noinstr
>    r4k_blast_dcache()
> --
> 
> commit 2871a54058216bdde1330a67071ead0fedc13363
> Author: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> 
> MIPS: Ingenic: Add XBurst SMP support
> 
> This commit adds SMP support for the second XBurst core on the JZ4780 SoC
> using CP0 mailboxes for IPIs and adds an XBurst idle handler that flushes
> the private data cache before executing the wait instruction.
> 
> 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/ingenic/ingenic-smp.c b/arch/mips/ingenic/ingenic-smp.c
>> new file mode 100644
>> index 0000000000000..27b9b573a7e07
>> --- /dev/null
>> +++ b/arch/mips/ingenic/ingenic-smp.c
> 
> [ ... ]
> 
>> +static void __init xburst_prepare_cpus(unsigned int max_cpus)
>> +{
>> +	struct device_node *np;
>> +	u32 reim;
>> +	int ret;
>> +
>> +	ret = request_irq(XBURST_MBOX_IRQ, xburst_mbox_interrupt,
>> +			  IRQF_PERCPU | IRQF_NO_THREAD, "core mailbox", NULL);
> 
> [Severity: Medium]
> Should this request_irq() call include IRQF_NO_SUSPEND?
> 
> Unlike request_percpu_irq(), request_irq() with IRQF_PERCPU does not set
> IRQF_NO_SUSPEND automatically, leaving desc->no_suspend_depth at 0.  During
> system suspend (such as suspend-to-idle), suspend_device_irqs() disables
> XBURST_MBOX_IRQ, which runs mask_mips_irq() and clears STATUSF_IP3 in
> CP0_STATUS on the calling CPU.  That blocks IPIs during noirq suspend and
> s2idle, and if resume_device_irqs() runs on the other CPU, STATUSF_IP3 stays
> masked after resume and hangs subsequent cross-CPU smp_call_function() calls.
> 
>> diff --git a/arch/mips/kernel/idle.c b/arch/mips/kernel/idle.c
>> index 80e8a04a642e0..6c44715d81d5b 100644
>> --- a/arch/mips/kernel/idle.c
>> +++ b/arch/mips/kernel/idle.c
>> @@ -18,6 +18,7 @@
>>   #include <asm/cpu-type.h>
>>   #include <asm/idle.h>
>>   #include <asm/mipsregs.h>
>> +#include <asm/r4kcache.h>
>>   
>>   /*
>>    * Not all of the MIPS CPUs have the "wait" instruction available. Moreover,
>> @@ -71,6 +72,28 @@ static void __cpuidle rm7k_wait_irqoff(void)
>>   		"	.set	pop					\n");
>>   }
>>   
>> +/* XBurst gates its private cache clock in WAIT; write back dirty data first. */
>> +static void __cpuidle xburst_wait_irqoff(void)
>> +{
>> +	unsigned long pending;
>> +
>> +	if (need_resched())
>> +		return;
>> +
>> +	r4k_blast_dcache();
> 
> [Severity: Low]
> Does calling r4k_blast_dcache() from __cpuidle xburst_wait_irqoff() violate
> noinstr idle rules?
> 
> xburst_wait_irqoff() runs between ct_cpuidle_enter() and ct_cpuidle_exit()
> while RCU is not watching, but r4k_blast_dcache() calls out-of-line
> blast_dcache32() in .text, which only has notrace from inline and lacks
> __no_sanitize_coverage, __no_profile and __no_kcsan.  Besides making
> cpu_in_idle() return false during the flush, builds with CONFIG_KCOV,
> CONFIG_GCOV_PROFILE_ALL or CONFIG_KCSAN run instrumentation while RCU is not
> watching and dirty the private L1 D-cache on return from blast_dcache32()
> right before wait gates the cache clock.
> 
>> +	/* Recheck work that may have arrived during the cache flush. */
>> +	pending = read_c0_cause() & read_c0_status() & CAUSEF_IP;
>> +	if (!need_resched() && !pending) {
> 
> [Severity: High]
> Does calling need_resched() after r4k_blast_dcache() pull the idle task's
> struct thread_info cache line back into the private L1 D-cache right before
> wait stops the cache clock?
> 
> need_resched() loads current_thread_info()->flags from KSEG0 cacheable
> memory, caching the line with _TIF_NEED_RESCHED clear in the sleeping CPU's
> private L1 D-cache.  When the other CPU sets _TIF_NEED_RESCHED on the idle
> thread_info->flags and sends SMP_RESCHEDULE_YOURSELF while the cache clock is
> gated:
> 
> CPU0                              CPU1
> xburst_wait_irqoff()
>    r4k_blast_dcache()
>    need_resched()
>      /* caches thread_info with
>       * _TIF_NEED_RESCHED == 0
>       */
>    wait
>                                    resched_curr()
>                                      set_tsk_need_resched(idle)
>                                      smp_send_reschedule()
>    <mailbox IPI wakes CPU>
>    handle_int / irq_enter()
>      /* writes regs / preempt_count
>       * in stale thread_info line
>       */
>    scheduler_ipi() /* no-op */
>    /* _TIF_NEED_RESCHED lost */
> 
> On MIPS, scheduler_ipi() does not modify thread_info->flags, so when CPU0
> takes the interrupt and writes other fields in the same 32-byte struct
> thread_info cache line (or if remote snoops stall while the cache clock is
> stopped), the reschedule is lost or the waking CPU stalls.  Since local IRQs
> are disabled and any remote wakeup during r4k_blast_dcache() raises the
> mailbox IPI in CP0_CAUSE, checking pending alone avoids touching the D-cache
> after the flush.
> 

All valid points, thanks for the feedback.
-- 
Florian