ESP32: Only access PSRAM inside a critical section - #230
Conversation
bugadani
changed the title
a4c1561 to
bb1c956
Compare
bb1c956 to
60f12cd
Compare
22b5886 to
c0afe95
Compare
|
@taiki-e is the CI failure somehow my fault? What do I need to do to clear it? |
60f12cd to
993a65c
Compare
taiki-e
left a comment
taiki-e
left a comment
There was a problem hiding this comment.
Thanks! This looks good overall.
aa32cee to
f47ed8c
Compare
|
@taiki-e thank you for your review. I've removed the Atomic64 implementation, cleaned up the rest of your observations, and also added one more commit (also LLM-assisted) that fixes a potential issue with losing stores when they race with an RMW operation. |
be0e1a4 to
6c92015
Compare
70a1c4e to
6c8f764
Compare
0df045b to
eaaa579
Compare
955f5c8 to
55f8b99
Compare
eaaa579 to
e0ebde6
Compare
taiki-e
left a comment
taiki-e
left a comment
There was a problem hiding this comment.
Sorry, I hadn't merged it due to a CI issue (see review comment), but I forgot to comment on that.
Co-authored-by: Taiki Endo <te316e89@gmail.com>
5a38561 to
90b4a61
Compare
| // disabled, the PSRAM path panics. | ||
| // | ||
| // `$ptr` is the backing raw pointer; the CS-path closure is an | ||
| // `unsafe` block that uses `read_volatile`/`write_volatile` on it. |
There was a problem hiding this comment.
@bugadani Is there a specific reason why pre::volatile_{read,write} is used here? (interrupt/mod.rs uses normal ptr::{read,write}.) If this is a type of memory where normal ptr::{read,write} are not permitted, I guess using static Mutex/Spinlock would result in UB; therefore, I don't believe it should be usable as a normal memory region.
There was a problem hiding this comment.
No particular reason besides muscle memory, I can come back and relax this.
I'm not entirely sure about your Mutex/Spinlock comment, we aren't placing a Mutex into PSRAM, that would either be incorrect (our Mutex implementation contains an AtomicU32), or circular.
CS's barrier is sufficient to prevent memory accesses within the CS being moved by optimizer outside the CS. #230 (comment)
Second attempt of trying to progress on esp-rs/esp-hal#2027, an alternative approach to #225. This PR has (almost) entirely been generated by Cursor/Claude, with minimal touchups from me after reading through what changes were made. I tried my bst to make sure it's not entirely nonsense, though I don't necessarily understand everything (especially
NotRefUnwindSafe).The key differences from the previous PR are:
cfgmaze. Not making a mess also allows us to make changes like supporting unsafe-assume-single-core easier in the future.core_atomicwith a similar-ish implementation for the affected chips.core::sync::atomic. I'm walking back on assembly for the time being to allow progressing on this issue in any (limited) way.critical-sectionis not enabled, access to PSRAM will panic. Trying to force-require the feature is a weird state to me, although I can flip it into a compile error easily enough if preferred.