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/cortexm_common: flush pipeline before disabling interrupts in idle #15236

Merged
merged 1 commit into from
Oct 16, 2020
Merged

cpu/cortexm_common: flush pipeline before disabling interrupts in idle #15236

merged 1 commit into from
Oct 16, 2020

Conversation

benpicco
Copy link
Contributor

Contribution description

When enabling & disabling interrupts back-to-back pending interrupts are not serviced on Cortex-M23/M33.

Flush the pipeline to give interrupts a chance of executing in sched_arch_idle().

This fixes no_idle_thread on Cortex-M23.

Testing procedure

Issues/PRs references

#14557 (comment)

@benpicco
cpu/cortexm_common: flush pipeline before disabling interrupts in idle
c48b331 
When enabling & disabling interrupts back-to-back pending interrupts
are not serviced on Cortex-M23/M33.

Flush the pipeline to give interrupts a chance of executing in `sched_arch_idle()`.

This fixes `no_idle_thread` on Cortex-M23.
@benpicco benpicco added Area: cpu Area: CPU/MCU ports Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 16, 2020
@maribu
Copy link
Member

maribu commented Oct 16, 2020

@kfessel: This might solve the issue you recently experience on the Nucleo-F767ZI

maribu
maribu approved these changes Oct 16, 2020
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Even if the extra pipeline flush might not be needed for every Cortex M variant, it is better to be safe than sorry here. (And the additional overhead should be not too bad.)

@maribu maribu added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 16, 2020
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 16, 2020
@aabadie
Copy link
Contributor

aabadie commented Oct 16, 2020

This is also fixing the issue I had in #15192 (M33). With this PR (and without the idle thread), it works like a charm.

@benpicco
Copy link
Contributor Author

Failed tests are

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 16, 2020
@maribu maribu merged commit 652aac4 into RIOT-OS:master Oct 16, 2020
@benpicco benpicco deleted the cpu/cortexm_common_idle_flush branch October 16, 2020 21:18
@benpicco benpicco mentioned this pull request Oct 16, 2020
Merged
@kaspar030
Copy link
Contributor

Nice catch @benpicco! 😄

@kfessel
Copy link
Contributor

kfessel commented Oct 18, 2020 •
edited
Loading

@kfessel: This might solve the issue you recently experience on the Nucleo-F767ZI

@maribu: sandly i still got hourly "gnrc_netif: possibly lost interrupt." after i rebased on current master with this patch merged.

(but the examples stayed running (gnrc_simple and the gcoap example )) -> i will run a retest with my project code

@maribu
Copy link
Member

maribu commented Oct 19, 2020

@maribu: sandly i still got hourly "gnrc_netif: possibly lost interrupt."

That is expected. If the load is too high, more messages will be generated than the message queue can store. This is due to the design choice of choosing receiver allocated messages in GNRC for communication.

What this should fix is the hang you experienced one night during context switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@thomaseichinger thomaseichinger Awaiting requested review from thomaseichinger

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

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@benpicco @maribu @aabadie @kaspar030 @kfessel