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
9a94c72 to
be6bcf2
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.
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.