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

Revert "sem: change sem wait to atomic operation" #14804

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 15, 2024

Summary

This reverts commit befe298.

Because a few regressions have been reported and
it likely will take some time to fix them:

Impact

Testing

tested with esp32s3-devkit:smp ostest

This reverts commit befe298.

Because a few regressions have been reported and
it likely will take some time to fix them:

* for some configurations, semaphore can be used on the special
  memory region, where atomic access is not available.
  cf. apache#14625

* include/nuttx/lib/stdatomic.h is not compatible with
  the C11 semantics, which the change in question relies on.
  cf. apache#14755
@github-actions github-actions bot added Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Nov 15, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 15, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: While it explains why the revert is happening, it lacks details on what functionality befe298 introduced in the first place. What part of the code did that commit modify? What was its intended purpose? This context is essential for reviewers.
  • Impact: This section is completely empty. Reverting a change can have significant impacts. The PR needs to address all the impact points, even if the answer is "NO". Specifically, it needs to detail the impact the reverted functionality had so reviewers can assess the consequences of removing it. For example, what features are being disabled/re-enabled? Are any build changes being undone?
  • Testing: While the target is mentioned, the build host information is missing. Also, "works as intended" is vague. What was the expected behavior before and after the revert? The provided logs should demonstrate the fix for the reported regressions (ideally, show the failing behavior before the revert and the working behavior after).

To make this PR compliant, the author should:

  1. Expand the Summary: Explain what functionality befe298 introduced and what parts of the code it affected.
  2. Complete the Impact section: Address all impact points. Consider the impact of removing the functionality added by the reverted commit.
  3. Provide complete testing details: Include the build host information (OS, CPU, compiler). Clarify the expected behavior and ensure the logs demonstrate the resolution of the mentioned regressions.

By addressing these points, the PR will be more complete, easier to review, and better aligned with the NuttX contribution guidelines.

@yamt
Copy link
Contributor Author

yamt commented Nov 20, 2024

@xiaoxiang781216
can you please consider to merge this for now, until an alternative like #14827 gets ready?
it's a bad idea to leave things broken for weeks.

@xiaoxiang781216 xiaoxiang781216 merged commit 4c3ae2e into apache:master Nov 20, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants