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

up_rtc_gettime: add spinlock to protect up_rtc_gettime #15413

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hujun260
Copy link
Contributor

@hujun260 hujun260 commented Jan 3, 2025

Summary

up_rtc_gettime: add spinlock to protect up_rtc_gettime

Impact

up_rtc_gettime

Testing

ci

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: renesas Issues related to the Renesas chips Arch: x86_64 Issues related to the x86_64 architecture Size: M The size of the change in this PR is medium labels Jan 3, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 3, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial details. Here's a breakdown:

Missing Information:

  • Summary: While the title mentions adding a spinlock, the why is missing. What race condition or problem does this fix? Which part of the code is changed (the specific function is mentioned, but a more general area like "RTC driver" would be helpful)? How does the spinlock solve the problem? Issue numbers are missing.
  • Impact: The impact section is severely lacking. It just repeats the function name. All the YES/NO questions need to be answered with explanations. Does this change affect any particular architectures, boards, or drivers? Does it change the API? Does it require documentation updates? Are there security implications of adding this lock?
  • Testing: "ci" is insufficient. What specific targets were tested? What tests were run? Actual log output (or a link to it) demonstrating the problem before the change and the successful operation after the change is needed. Host build environment information is also missing.

Example of how to improve the Summary:

"The up_rtc_gettime function was prone to race conditions when accessed concurrently from multiple threads, potentially leading to incorrect time readings. This change adds a spinlock to protect access to the RTC hardware during the up_rtc_gettime operation within the common RTC driver code. This fixes [Issue #XYZ]. This change also affects the up_rtc_settime function, which now also acquires the same spinlock.

Example of how to improve the Impact:

  • Is new feature added? NO
  • Is existing feature changed? YES, the up_rtc_gettime and up_rtc_settime functions are now thread-safe.
  • Impact on user (will user need to adapt to change)? NO
  • Impact on build (will build process change)? NO
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO. This change is within the common RTC driver code, so all architectures using this driver benefit.
  • Impact on documentation (is update required / provided)? YES, the documentation for up_rtc_gettime and up_rtc_settime should mention the thread-safety improvements. A documentation update is included in this PR.
  • Impact on security (any sort of implications)? NO
  • Impact on compatibility (backward/forward/interoperability)? NO
  • Anything else to consider? The addition of the spinlock introduces a small performance overhead.

Example of how to improve the Testing:

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:qemu-arm, stm32f4discovery:nsh

Testing logs before change: (demonstrate the race condition - e.g., inconsistent time readings from two threads accessing the RTC simultaneously)

... log output showing the problem ...

Testing logs after change: (show consistent time readings and no errors)

... log output showing the fix ...

Or:
Testing logs are available in the CI run for this PR: [Link to CI run]

By providing this level of detail, reviewers can quickly understand the change, its impact, and verify its correctness, leading to a faster and smoother review process.

arch/x86_64/src/intel64/intel64_rtc.c Outdated Show resolved Hide resolved
arch/x86_64/src/intel64/intel64_rtc.c Outdated Show resolved Hide resolved
arch/arm/src/cxd56xx/cxd56_rtc.c Outdated Show resolved Hide resolved
arch/arm/src/max326xx/max32660/max32660_rtc.c Outdated Show resolved Hide resolved
arch/arm/src/sam34/sam_rtc.c Outdated Show resolved Hide resolved
arch/arm/src/sam34/sam_rtc.c Outdated Show resolved Hide resolved
arch/renesas/src/rx65n/rx65n_rtc.c Show resolved Hide resolved
arch/renesas/src/rx65n/rx65n_rtc.c Outdated Show resolved Hide resolved
reason:
We have removed the critical section protection
for the up_rtc_gettime function in common code.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
…x_pio.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
…g/SEGGER_RTT_Conf.h

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@github-actions github-actions bot added the Area: Drivers Drivers issues label Jan 6, 2025
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 Arch: renesas Issues related to the Renesas chips Arch: x86_64 Issues related to the x86_64 architecture Area: Drivers Drivers 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.

3 participants