imeghar2408-max commented on PR #20012: URL: https://github.com/apache/nuttx/pull/20012#issuecomment-5511445867
> @imeghar2408-max if you used some AI LLM to help you to fix this issue, please add to your commit message: Assisted-by: Name of AI vendor and model > > I asked Claude Code to analyze these changes and it reported: > > ``` > Problems with the PR itself > > 1. The test doesn't exercise the bug. On rv-virt with CONFIG_SMP, setintstack in qemu-rv/chip.h > computes g_intstacktop - up_cpu_index*INT_STACK_SIZE and never reads irq_stack from the > scratch area. Same for jh7110, k230, bl808, sg2000, eic7700x. The only chip whose SMP exception > entry reads RISCV_PERCPU_IRQSTACK is MPFS - and there's no in-tree MPFS SMP defconfig. So the > runtime log proves it boots, nothing more. It needs validation on MPFS SMP; @pussuw is the code > owner and the right reviewer for that. > > 2. spin_lock_irqsave() around percpu = &g_percpu[cpu] is now protecting an address computation. > Drop it. Arguably riscv_percpu_init()/g_initialized could go too (compute irq_stack inline for cpu). > > 3. Incomplete cleanup: union riscv_percpu_s in riscv_percpu.h still has the next member for "sl list > linkage". Should become a plain struct. > > 4. Correctness now depends on riscv_hartid_to_cpuid() being right. Without > CONFIG_ARCH_RV_CPUID_MAP it's the identity macro (hart), so any hart with hartid >= > CONFIG_SMP_NCPUS writes past g_percpu[] in a release build — the DEBUGASSERT only catches it > with debug asserts on. I checked the in-tree callers and they're fine (jh7110 rebases the hart ID in > head.S, the other SoCs only register hart 0 directly and secondaries go through the > riscv_cpuid_to_hartid(cpu) round-trip, MPFS has the map), but the freelist tolerated arbitrary hart > IDs and this doesn't. > > 5. The issue (#19875) gives no platform and no observed failure, and the description conflates two > things. The likely real-world symptom is a crash dump showing the wrong CPU's IRQ stack on an > MPFS-style port, not a runtime fault. > ``` Hi Alan, I went through all the points you raised and updated the PR accordingly. I addressed the locking/address-computation concern, fixed the incomplete cleanup of riscv_percpu_s, and reviewed the riscv_hartid_to_cpuid()/hart-ID mapping assumptions and the real-world failure scenario. I also rechecked the changes and validation after the updates. The only point I couldn't fully complete was the SMP runtime testing on MPFS, since I couldn't find a suitable in-tree MPFS SMP configuration to run that test. The updated changes are now pushed. When you have time, could you please take another look at the PR? I'd really appreciate your review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
