nuttxpr commented on PR #14803: URL: https://github.com/apache/nuttx/pull/14803#issuecomment-2477931184
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR summary provided is insufficient and lacks crucial information required by the NuttX guidelines. Here's a breakdown of the issues and a suggested improvement: **Missing/Insufficient Information:** * **Summary - Why is this change *necessary*?** The summary mentions some ARMv7-M SoCs don't work with atomic instructions despite supporting them. This needs more detail. *Why* don't they work? Is it a silicon bug? A compiler issue? What are the specific symptoms of failure? What problems does this PR solve for users? * **Summary - What changes are being made?** The summary is vague. Mentioning `CONFIG_LIBC_ARCH_ATOMIC` is introduced and function names changed is not enough. *Which* functions are renamed? How does this new config option affect the build? How does it interact with existing configurations? Be specific. * **Summary - How does the change work?** Explain at a high level *how* introducing this config option and renaming functions resolves the atomic instruction issue. * **Impact - Is this a new feature?** Yes, `CONFIG_LIBC_ARCH_ATOMIC` is a new configuration option. This needs to be explicitly stated. * **Impact - User impact?** While the PR states ARCH_CHIP_CXD32XX is impacted, it doesn't say *how*. Will users need to enable this new config option? Will it be enabled by default for CXD32XX? What changes are required in user applications? Provide clear instructions for users of affected architectures. * **Impact - Build impact?** Yes, there's a build impact. Describe it in more detail. Adding a new Kconfig option changes the build process. How are dependencies handled? How is `arch_atomic.c` now included in the build based on this config option? * **Impact - Hardware impact?** Yes, there is an impact. Specify which architectures/boards are directly affected. While you list several in the testing section, explicitly state them in the Impact section. Explain *how* they are affected. * **Impact - Documentation impact?** Does this change require documentation updates? If so, confirm that they are included in the PR. If not, explain why. * **Impact - Security impact?** Even if there's no security impact, explicitly state "NO". * **Impact - Compatibility impact?** Explicitly state whether there are any backward compatibility issues. * **Testing - "Works as intended" is too vague.** What was the intended outcome? What tests were performed to validate it? The listed test configurations are helpful, but the logs are missing. Provide *relevant* log snippets demonstrating the issue before the change and the successful operation after the change. **Suggested Improved Summary:** ## Summary This PR addresses an issue where atomic instructions fail on certain ARMv7-M SoCs (specifically, family X, model Y, etc. - list affected SoCs), despite being architecturally supported. This failure manifests as [describe the specific symptoms of the failure, e.g., data corruption, system crashes, etc.]. The root cause is [explain the root cause - silicon errata, compiler bug, etc.]. To resolve this, a new Kconfig option `CONFIG_LIBC_ARCH_ATOMIC` is introduced. Enabling this option forces the use of software-emulated atomic operations provided by `arch_atomic.c`, bypassing the faulty hardware instructions. This requires renaming the affected functions in `arch_atomic.c` from `__atomic_*` to `nx_atomic_*` to avoid conflicts with GCC built-in atomics. This also addresses compatibility issues with libcxx by using `nx_atomic_*` versions in `libcxx/include/__atomic/cxx_atomic_lmpl.h` when `CONFIG_LIBC_ARCH_ATOMIC` is enabled. Fixes [NuttX Issue # if applicable]. ## Impact * **New Feature:** YES. `CONFIG_LIBC_ARCH_ATOMIC` configuration option is added. * **User Impact:** YES. Users with affected SoCs (specifically, list them) must enable `CONFIG_LIBC_ARCH_ATOMIC` in their `.config` file to utilize the software atomic emulation. [Provide clear instructions on how to enable the option and any potential impact on application code]. * **Build Impact:** YES. The build system now conditionally compiles and links `arch_atomic.c` based on the `CONFIG_LIBC_ARCH_ATOMIC` setting. [Explain any Kconfig dependencies added.] * **Hardware Impact:** YES. This change specifically targets [list affected architectures and boards]. Enabling the new config option provides a workaround for the atomic instruction issue on these platforms. * **Documentation Impact:** YES/NO. [Explain what documentation is updated/added or why no documentation update is needed.] * **Security Impact:** NO. * **Compatibility Impact:** [Explain any potential backward compatibility concerns or explicitly state NO.] * **Other Considerations:** [Mention any other relevant points, such as future work or related issues.] ## Testing I confirm that changes are verified on a local setup and work as intended. The intended outcome was to enable correct operation of atomic instructions on the affected platforms by utilizing software emulation. The following tests were performed: * **Build Host:** [OS, CPU, Compiler details] * **Targets:** * ARCH_CHIP_CXD32XX:nsh (Build passes, ostest passes). Note: This board is out-of-tree. * ... (Other tested targets) **Testing Logs (Before Change - ARCH_CHIP_CXD32XX):** ``` [Relevant log snippets demonstrating the failure, e.g., data corruption, incorrect values] ``` **Testing Logs (After Change - ARCH_CHIP_CXD32XX):** ``` [Relevant log snippets demonstrating the corrected behavior, e.g., correct data values, successful operation] ``` ...(Repeat log examples for other tested platforms, showing the problem before and the solution after). By providing more detail and clarity, the PR will be much easier for reviewers to understand and approve. Remember to always provide specific information and demonstrate the problem and solution with concrete examples. -- 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]
