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

Fix riscv_machine_timer mtime read #10859

Closed
wants to merge 1 commit into from

Conversation

Dolu1990
Copy link

This pullrequest fix #10788

@codecov-io
Copy link

codecov-io commented Oct 25, 2018

Codecov Report

Merging #10859 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10859   +/-   ##
=======================================
  Coverage   53.27%   53.27%           
=======================================
  Files         215      215           
  Lines       25851    25851           
  Branches     5695     5695           
=======================================
  Hits        13773    13773           
  Misses       9762     9762           
  Partials     2316     2316
Impacted Files Coverage Δ
subsys/bluetooth/host/hci_core.c 2.46% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40bf45a...f25db14. Read the comment docs.

@Dolu1990
Copy link
Author

This issue could lead to some timer freeze.
For instance on a system which take (let's say) 5 cycles between reading the mtime low and the mtime high, a timer freez would occure each 5/2^32 timer interrupts.

If that system is running at 100 Mhz, the freez would be about 40 seconds, and if the timer is running at 1Khz it would happen each 10 days.

Fix riscv_machine_timer mtime non-atomic 64 bits read.

Signed-off-by: Charles Papon <[email protected]>
@Dolu1990
Copy link
Author

@pgielda @kgugala Ready for review

@nategraff-sifive
Copy link
Contributor

Thanks for your contribution @Dolu1990!

Right now there's also a rewrite of a whole bunch of timer drivers in PR #10556, which includes a similar change for the RISC-V machine timer driver along with a number of other improvements.

@brouhaha
Copy link

brouhaha commented Nov 2, 2018

@Dolu1990 The code looks good. I think your analysis of the freeze is wrong. Unless some nested higher-priority interrupt(s) block the timer handler for near 2^32 mtime cycles, the loop will never iterate more than twice before it succeeds to get two consecutive identical reads of mtimeh.

@Dolu1990
Copy link
Author

Dolu1990 commented Nov 2, 2018

Oulala, look like there is some serious business around the timer ^^
thanks :)

@nategraff-sifive
Copy link
Contributor

Closing, #10556 included this same change. Thanks again for your contribution @Dolu1990!

@Dolu1990
Copy link
Author

<3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

riscv_machine_timer driver don't follow the spec ?
4 participants