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

nRF52 serial: Tighten/simplify atomics #9616

Merged
merged 1 commit into from Feb 20, 2019

Conversation

Projects
None yet
6 participants
@kjbracey-arm
Copy link
Contributor

commented Feb 5, 2019

Description

Use new atomics (exchange, load, store and bool types) to simplify
and improve the atomics in the nRF52 serial HAL.

  • Ensure mutexes are released last and atomically when done
    done inside a critical section.
  • Compare-and-swap is not required for the spinlock - exchange is
    sufficient. (Not clear a spinlock is needed anyway, but left in).
  • Remove unneeded volatile, and make mutexes bool.

Requires preceding PR #9600.

Pull request type

[ ] Fix
[ ] Refactor
[x] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@marcuschangarm

nRF52 serial: Tighten/simplify atomics
Use new atomics (exchange, load, store and bool types) to simplify
and improve the atomics in the nRF52 serial HAL.

* Ensure mutexes are released last and atomically when done
  done inside a critical section.
* Compare-and-swap is not required for the spinlock - exchange is
  sufficient. (Not clear a spinlock is needed anyway, but left in).
* Remove unneeded volatile, and make mutexes bool.

@ciarmcom ciarmcom requested review from marcuschangarm and ARMmbed/mbed-os-maintainers Feb 5, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@kjbracey-arm, thank you for your changes.
@marcuschangarm @ARMmbed/mbed-os-maintainers please review.

@kjbracey-arm kjbracey-arm requested a review from pan- Feb 5, 2019

@pan-

pan- approved these changes Feb 6, 2019

Copy link
Member

left a comment

Changes looks good however I'm concern about the flags tx_async and rx_asynch which are used to determine if a transaction is in progress.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

I see what you mean. I was under the impression that the async variables were protected by the in_progress atomic, but that's not really true (any more?), is it? The in_progress is actually only set when it's async anyway.

It might be okay - it's only read by the interrupt handler, and it's written by the async setup code to make it respond differently. As long as it writes to it before enabling interrupts, and the interrupt enable is a memory barrier, you're fine.

But interrupt enables might not be memory barriers: patch coming for that here - ARM-software/CMSIS_5#510

I'm not really sure how much any of this is required - most HALs don't have any sort of protection in this area, and just assume you don't mix async and sync operations, or attempt overlapping async.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 20, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Feb 20, 2019

@cmonr cmonr merged commit 59549b8 into ARMmbed:master Feb 20, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9337 cycles (-1131 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

cmonr added release-version: 5.12.0-rc1 release-version: 5.11.5 and removed release-version: 5.12.0-rc1 labels a day ago

Ha. Should've known. Preceeding PR was targeting 5.12, so this needs to as well.

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:nrf52_atomics branch Feb 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.