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.

Sign up for GitHub

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

Jump to bottom

cpu/atmega_common: Bugfixes in irq_arch.c #11452

Merged
merged 2 commits into from
Sep 10, 2019
Merged

cpu/atmega_common: Bugfixes in irq_arch.c #11452

merged 2 commits into from
Sep 10, 2019

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 26, 2019

Contribution description

  1. Fixed the return code of irq_enable():
    • Previously the status register after enabling the interrupts was returned, not prior to enabling it
      • The doc states the return value is the "Previous value of the status register", not the new value
    • The first commit changes this to return the new value
  2. Added required memory barriers to irq_*() family of functions
    • Added barriers to prevent the compiler from reordering code across calls to irq_disable(), irq_restore() and irq_enable()
    • Added barrier to irq_is_in()

Testing procedure

  • Regarding the fixed return value:
    • I personally think a code review is sufficient here. If the reviewer insists, I will come up with a test
  • Regarding the barriers:
    • The optimization potential on the AVR platform by reordering or combining memory accesses is practically zero. Most instructions are executed in one cycle and access to memory can only be performed in 8 bit chunks
      • Thus: Reordering and combining memory accesses is unlikely to yield any benefit.
      • Thus: The compiler will not reorder or combine memory accesses most of the time even without memory barriers
    • I compiled examples/default for the Arduino Mega2560 using GCC 8.3.0 before (master + fix of return value of irq_enable()) and after (this PR):
    • As seen this does not change the binaries, so there cannot be a regression introduced.

Issues/PRs references

Same as #11440, but for AVR. Related to #11438

maribu added 2 commits April 25, 2019 17:42
@maribu
cpu/atmega_common: Fix return value of irq_enable
48fabca 
Citing the doc of irq_enable():

    @return Previous value of status register. [...]

On atmega however the new value of the status register is returned, not the one
prior to enabling interrupts.
@maribu
cpu/atmega_common: Add barriers to irq_*()
70cf9b3 
To prevent reordering of accesses to the interrupt control register when link
time optimization (LTO) is enabled, memory barriers are needed. Without LTO
calls to the external functions irq_disable(), irq_restore(), irq_enable() and
irq_is_in() have the same affect as compiler barriers, as the compiler is unable
to prove that reordering of memory accesses is safe (from a single-threaded
point of view). With LTO the compiler can easily prove that reordering is safe
from a single-threaded point of view: Thus, the compiler may move memory
accesses wrapped in irq_disable(), irq_restore() across those calls.

The memory barriers will have no effect on non-LTO builds.
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: AVR Platform: This PR/issue effects AVR-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 26, 2019
@maribu maribu requested a review from kaspar030 April 26, 2019 08:52
@maribu maribu requested review from kYc0o, ZetaR60 and jcarrano May 23, 2019 07:10
@jcarrano
Copy link
Contributor

As a note, the irq_enable is also broken in other archs (I know of cortex_m and fe310), see #10076. @kaspar030 mentioned in that issue that it would break the API, which is right, but given that the current return value is useless (it always returns "enabled") I cannot see how anyone could be using the return value for anything.

jcarrano
jcarrano approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

Looks good, should not break anything and makes sense. Ok.

@jcarrano jcarrano merged commit abc25f0 into RIOT-OS:master Sep 10, 2019
@maribu
Copy link
Member Author

maribu commented Sep 10, 2019

Thanks for the review!

@maribu maribu deleted the atmega_irq branch September 10, 2019 11:45
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcarrano jcarrano jcarrano approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@kYc0o kYc0o Awaiting requested review from kYc0o kYc0o is a code owner

@ZetaR60 ZetaR60 Awaiting requested review from ZetaR60

Assignees
No one assigned
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
3 participants
@maribu @jcarrano @kb2ma