Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arch_atomic : Introduce CONFIG_LIBC_ARCH_NXATOMIC #14681

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TakuyaMiyasita
Copy link
Contributor

Summary

Some armv7-m-based SoCs do not work with atomic instructions, even though armv7-m supports them.

To avoid using atomic instructions generated by gcc, CONFIG_LIBC_ARCH_NXATOMIC is newly introduced with which arch_atomic.c is linked explicitly.

However, the function names need to be changed to avoid build errors, since the functions described in stdatomic.h are gcc built-in and inlined when the code is compiled.

Impact

CONFIG_ARCH_CHIP_CXD32XX.

Testing

About ARCH_CHIP_CXD32XX, the build is passed and ostes is passed, but the board is out-of-tree.
About raspberrypi-pico:nsh, the build is passed. (not executed)
About spresense:smp, the build is passed. (not executed)

tool-chain version:

arm-none-eabi-gcc (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

and

arm-none-eabi-gcc (GNU Tools for ARM Embedded Processors) 5.3.1 20160307 (release) [ARM/embedded-5-branch revision 234589]
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Some armv7-m-based SoCs do not work with atomic instructions,
even though armv7-m supports them.

To avoid using atomic instructions generated by gcc,
CONFIG_LIBC_ARCH_NXATOMIC is newly introduced with which
arch_atomic.c is linked explicitly.

However, the function names need to be changed to avoid
build errors, since the functions described in stdatomic.h
are gcc built-in and inlined when the code is compiled.

Signed-off-by: Takuya Miyasita <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Nov 7, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 7, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information/Improvements Needed:

  • Summary: While you explain what you changed, the why is not sufficiently clear. Why do these SoCs not work with atomic instructions even though armv7-m supports them? Is this a silicon bug? A missing coprocessor? More details are crucial. Also, be more specific about "what functional part of the code is being changed." Mention affected files/modules (e.g., arch_atomic.c, stdatomic.h).
  • Impact: You only mention CONFIG_ARCH_CHIP_CXD32XX. This is incomplete. Address all the impact points. Even if the answer is "NO," explicitly state it. For example:
    • Is new feature added?: NO
    • Is existing feature changed?: YES (atomic operation implementation for CXD32XX)
    • Impact on user: Potentially YES if users rely on atomics on CXD32XX. Explain what they might observe before/after the change.
    • Impact on build: YES (new config option CONFIG_LIBC_ARCH_NXATOMIC) Explain how to use the new option.
    • Impact on hardware: YES (CXD32XX). Explain why other armv7-m are not affected.
    • Impact on documentation: YES (at least for the new config option). State if documentation has been updated in the PR or if a separate documentation update is planned.
    • Impact on security: Possibly YES or NO. Justify your answer. Incorrect atomic operations can have security implications.
    • Impact on compatibility: Potentially YES. Does this break compatibility with existing code that uses atomics on CXD32XX?
  • Testing: This is insufficient.
    • "ostes is passed" is too vague. Which tests in ostes?
    • "Build is passed" for other platforms isn't real testing. You need to run tests on representative platforms to ensure no regressions. If you cannot run on actual hardware, explain why and consider using a simulator.
    • Provide relevant log snippets. Don't just dump entire logs. Focus on the sections that demonstrate the issue before and the fix after. Show that atomic operations are now working correctly on CXD32XX.
    • Testing on only one compiler version is not ideal. If feasible, test with other common toolchains.

Example of Improved Impact Section:

  • Is new feature added?: NO
  • Is existing feature changed?: YES (atomic operation implementation for CXD32XX)
  • Impact on user: YES. Previously, atomic operations on CXD32XX could lead to undefined behavior due to silicon errata [link to errata if available]. This change ensures correct atomic behavior. Users relying on atomic operations on CXD32XX should see improved stability and data consistency.
  • Impact on build: YES. A new Kconfig option CONFIG_LIBC_ARCH_NXATOMIC is introduced. To enable it for CXD32XX, add CONFIG_LIBC_ARCH_NXATOMIC=y to your defconfig.
  • Impact on hardware: YES (CXD32XX). This change specifically addresses the atomic operation issue on CXD32XX. Other armv7-m architectures are not affected because they correctly implement the required atomic instructions.
  • Impact on documentation: YES. The Kconfig documentation has been updated to describe the CONFIG_LIBC_ARCH_NXATOMIC option and its usage. (Provide path to updated documentation file.)
  • Impact on security: YES. Incorrect atomic operations could potentially lead to race conditions and data corruption, which could have security implications. This change mitigates these risks on CXD32XX.
  • Impact on compatibility: Potentially YES. If existing code on CXD32XX relied on the previous (incorrect) behavior of atomics, it might require adaptation. However, this is unlikely as the previous behavior was undefined.

By addressing these points, your PR will be much stronger and easier for reviewers to evaluate. Remember, clarity and thoroughness are key to a successful contribution!

@TakuyaMiyasita
Copy link
Contributor Author

@nuttxpr

Why do these SoCs not work with atomic instructions even though armv7-m supports them? Is this a silicon bug? A missing coprocessor

please refer following comment.
4cec713#commitcomment-148460811

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

Sorry I have to cancel the CI Check, there's a problem with CI Test:

#define atomic_fetch_sub_explicit(obj, val, type) atomic_fetch_sub_n(obj, val, type)

#else

#define atomic_store_n(obj, val, type) \
(sizeof(*(obj)) == 1 ? __atomic_store_1(obj, val, type) : \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's always map to to nx_ version regardless CONFIG_LIBC_ARCH_NXATOMIC

#ifdef CONFIG_LIBC_ARCH_NXATOMIC

#define atomic_store_n(obj, val, type) \
(sizeof(*(obj)) == 1 ? __nx_atomic_store_1(obj, val, type) : \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chang ALL __nx_ to nx_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants