From 6f42bf7a7b85e301f0e0d146810bd2d172048639 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Hlusi=C4=8Dka?= Date: Thu, 26 Feb 2026 20:32:14 +0100 Subject: [PATCH] Revise usage of mutexes --- firmware/acid-firmware/src/console.rs | 4 +- firmware/acid-firmware/src/ffi/crypto.rs | 6 +-- firmware/acid-firmware/src/logging.rs | 47 ++++++++++--------- firmware/acid-firmware/src/main.rs | 3 +- .../src/peripherals/st7701s/mod.rs | 2 +- firmware/acid-firmware/src/ui/backend.rs | 24 +++++----- firmware/acid-firmware/src/ui/mod.rs | 4 +- firmware/acid-firmware/src/util.rs | 37 +++++++++++++++ 8 files changed, 85 insertions(+), 42 deletions(-) diff --git a/firmware/acid-firmware/src/console.rs b/firmware/acid-firmware/src/console.rs index b157f4e..45ba52f 100644 --- a/firmware/acid-firmware/src/console.rs +++ b/firmware/acid-firmware/src/console.rs @@ -18,11 +18,11 @@ impl embedded_io::ErrorType for Writer { impl embedded_io::Write for Writer { fn write(&mut self, buf: &[u8]) -> Result { - with_uart_tx(|_, uart| uart.write(buf)) + with_uart_tx(|uart| uart.write(buf)) } fn flush(&mut self) -> Result<(), Self::Error> { - with_uart_tx(|_, uart| uart.flush()) + with_uart_tx(|uart| uart.flush()) } } diff --git a/firmware/acid-firmware/src/ffi/crypto.rs b/firmware/acid-firmware/src/ffi/crypto.rs index 7b4de2e..c29e4bb 100644 --- a/firmware/acid-firmware/src/ffi/crypto.rs +++ b/firmware/acid-firmware/src/ffi/crypto.rs @@ -3,7 +3,6 @@ use core::{ ffi::{c_char, c_int, c_size_t, c_uchar, c_ulonglong}, }; -use critical_section::Mutex; use embassy_sync::blocking_mutex::{self, raw::CriticalSectionRawMutex}; use hmac::digest::{FixedOutput, KeyInit, Update}; use password_hash::Key; @@ -39,7 +38,8 @@ unsafe extern "C" fn __spre_crypto_hash_sha256( /// This is the encrypted user key currently being used in the key derivation function of spectre. /// It decrypts using the user's password into the key that would be derived with the original password hashing function. -pub static ACTIVE_ENCRYPTED_USER_KEY: Mutex> = Mutex::new(Cell::new([0; _])); +pub static ACTIVE_ENCRYPTED_USER_KEY: blocking_mutex::Mutex> = + blocking_mutex::Mutex::new(Cell::new([0; _])); #[unsafe(no_mangle)] #[must_use] @@ -65,7 +65,7 @@ unsafe extern "C" fn __spre_crypto_pwhash_scryptsalsa208sha256_ll( }; let output: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(output, output_len) }; - let mut user_key = critical_section::with(|cs| ACTIVE_ENCRYPTED_USER_KEY.borrow(cs).get()); + let mut user_key = ACTIVE_ENCRYPTED_USER_KEY.lock(|user_key| user_key.get()); password_hash::decrypt_with(&mut user_key, &encryption_key); output.copy_from_slice(&user_key); diff --git a/firmware/acid-firmware/src/logging.rs b/firmware/acid-firmware/src/logging.rs index 149ab8f..36fd092 100644 --- a/firmware/acid-firmware/src/logging.rs +++ b/firmware/acid-firmware/src/logging.rs @@ -69,28 +69,29 @@ pub mod usb { #[cfg(feature = "alt-log")] #[macro_use] pub mod uart { + use crate::util::MutexExt; + use super::*; - + use core::{cell::RefCell, fmt::Write}; - use critical_section::{CriticalSection, Mutex}; - use esp_hal::{ - Blocking, - uart::UartTx, - }; + use embassy_sync::{blocking_mutex::raw::CriticalSectionRawMutex, mutex::Mutex}; + use esp_hal::{Blocking, uart::UartTx}; use log::{Log, info}; - static ALT_LOGGER_UART: Mutex>>> = - Mutex::new(RefCell::new(None)); + static ALT_LOGGER_UART: Mutex< + CriticalSectionRawMutex, + RefCell>>, + > = Mutex::new(RefCell::new(None)); - pub fn with_uart_tx( - f: impl FnOnce(CriticalSection<'_>, &'_ mut UartTx<'static, Blocking>) -> R, - ) -> R { - critical_section::with(|cs| { - let mut uart = ALT_LOGGER_UART.borrow(cs).borrow_mut(); - let uart = uart.as_mut().unwrap(); + pub fn with_uart_tx(f: impl FnOnce(&'_ mut UartTx<'static, Blocking>) -> R) -> R { + // Safety: + // * The guard is not held across yield points. + // * **CARE MUST BE TAKEN NOT TO INVOKE THIS FUNCTION FROM AN INTERRUPT HANDLER.** + let uart = unsafe { ALT_LOGGER_UART.lock_blocking() }; + let mut uart = uart.borrow_mut(); + let uart = uart.as_mut().unwrap(); - (f)(cs, uart) - }) + (f)(uart) } #[allow(unused)] @@ -106,7 +107,7 @@ pub mod uart { #[allow(unused)] fn do_print(args: core::fmt::Arguments<'_>) { - with_uart_tx(|_, uart| { + with_uart_tx(|uart| { uart.write_fmt(format_args!("{}\n", args)).unwrap(); uart.flush().unwrap(); }) @@ -123,7 +124,7 @@ pub mod uart { #[allow(unused)] fn log(&self, record: &log::Record) { - with_uart_tx(|cs, uart| { + with_uart_tx(|uart| { with_formatted_log_record(record, |args| uart.write_fmt(args)).unwrap(); uart.flush().unwrap(); }) @@ -154,9 +155,13 @@ pub mod uart { } pub fn setup_logging(uart_tx: UartTx<'static, Blocking>) { - critical_section::with(|cs| { - *ALT_LOGGER_UART.borrow(cs).borrow_mut() = Some(uart_tx); - }); + { + // Safety: + // * The guard is not held across yield points. + // * This function is not invoked from an interrupt handler. + let uart = unsafe { ALT_LOGGER_UART.lock_blocking() }; + *uart.borrow_mut() = Some(uart_tx); + } unsafe { log::set_logger_racy(&UartLogger).unwrap(); diff --git a/firmware/acid-firmware/src/main.rs b/firmware/acid-firmware/src/main.rs index 6ab5ed4..c7c5932 100644 --- a/firmware/acid-firmware/src/main.rs +++ b/firmware/acid-firmware/src/main.rs @@ -30,6 +30,7 @@ use alloc::vec::Vec; use embassy_embedded_hal::adapter::BlockingAsync; use embassy_embedded_hal::flash::partition::Partition; use embassy_executor::Spawner; +use embassy_sync::blocking_mutex; use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex; use embassy_sync::channel::Channel; use embassy_sync::mutex::Mutex; @@ -753,7 +754,7 @@ async fn main_task(peripherals: MainPeripherals) { window: RefCell::new(None), swapchain: RefCell::new(swapchain_writer), quit_event_loop: Default::default(), - events: Arc::new(critical_section::Mutex::new(RefCell::new(VecDeque::new()))), + events: Arc::new(blocking_mutex::Mutex::new(RefCell::new(VecDeque::new()))), }; spawner.must_spawn(ui::run_renderer_task(slint_backend, flash_part_acid)); }); diff --git a/firmware/acid-firmware/src/peripherals/st7701s/mod.rs b/firmware/acid-firmware/src/peripherals/st7701s/mod.rs index 221538b..b9a4b85 100644 --- a/firmware/acid-firmware/src/peripherals/st7701s/mod.rs +++ b/firmware/acid-firmware/src/peripherals/st7701s/mod.rs @@ -1276,7 +1276,7 @@ where // // Adafruit would use 11 MHz. // I had lowered the frequency, so that `DmaBounce` could keep up. - .with_frequency(Rate::from_mhz(5)) // From Adafruit + .with_frequency(Rate::from_mhz(6)) // From Adafruit .with_clock_mode(ClockMode { polarity: Polarity::IdleLow, // From Adafruit phase: Phase::ShiftHigh, // From Adafruit diff --git a/firmware/acid-firmware/src/ui/backend.rs b/firmware/acid-firmware/src/ui/backend.rs index b62f068..fd6d0c5 100644 --- a/firmware/acid-firmware/src/ui/backend.rs +++ b/firmware/acid-firmware/src/ui/backend.rs @@ -7,7 +7,7 @@ use core::{ use alloc::{ boxed::Box, collections::vec_deque::VecDeque, rc::Rc, string::ToString, sync::Arc, vec::Vec, }; -use critical_section::Mutex; +use embassy_sync::blocking_mutex::{self, raw::CriticalSectionRawMutex}; use esp_hal::time::Instant; use esp_hal_bounce_buffers::SwapchainWriter; use log::{debug, info}; @@ -29,7 +29,9 @@ pub struct SlintBackend { pub window: RefCell>>, pub swapchain: RefCell, pub quit_event_loop: Arc, - pub events: Arc>>>>, + pub events: Arc< + blocking_mutex::Mutex>>>, + >, } impl slint::platform::Platform for SlintBackend { @@ -66,13 +68,9 @@ impl slint::platform::Platform for SlintBackend { /* loop */ { - let drained_events = critical_section::with(|cs| { - self.events - .borrow(cs) - .borrow_mut() - .drain(..) - .collect::>() - }); + let drained_events = self + .events + .lock(|events| events.borrow_mut().drain(..).collect::>()); for event in drained_events { (event)(); @@ -146,7 +144,9 @@ impl slint::platform::Platform for SlintBackend { struct AcidEventLoopProxy { pub quit_event_loop: Arc, - pub events: Arc>>>>, + pub events: Arc< + blocking_mutex::Mutex>>>, + >, } impl EventLoopProxy for AcidEventLoopProxy { @@ -159,8 +159,8 @@ impl EventLoopProxy for AcidEventLoopProxy { &self, event: Box, ) -> Result<(), EventLoopError> { - critical_section::with(|cs| { - self.events.borrow(cs).borrow_mut().push_back(event); + self.events.lock(|events| { + events.borrow_mut().push_back(event); }); Ok(()) } diff --git a/firmware/acid-firmware/src/ui/mod.rs b/firmware/acid-firmware/src/ui/mod.rs index 9651d8e..49c60fe 100644 --- a/firmware/acid-firmware/src/ui/mod.rs +++ b/firmware/acid-firmware/src/ui/mod.rs @@ -61,8 +61,8 @@ fn spectre_derive_user_key( encrypted_key: Option, ) -> SpectreUserKey { if let Some(encrypted_key) = encrypted_key { - critical_section::with(|cs| { - ACTIVE_ENCRYPTED_USER_KEY.borrow(cs).set(encrypted_key); + ACTIVE_ENCRYPTED_USER_KEY.lock(|user_key| { + user_key.set(encrypted_key); }); } diff --git a/firmware/acid-firmware/src/util.rs b/firmware/acid-firmware/src/util.rs index 80abdf4..c5ae610 100644 --- a/firmware/acid-firmware/src/util.rs +++ b/firmware/acid-firmware/src/util.rs @@ -1,5 +1,9 @@ use core::fmt::Display; +use embassy_sync::{ + blocking_mutex::raw::RawMutex, + mutex::{Mutex, MutexGuard, TryLockError}, +}; use embassy_time::Duration; pub struct FormattedDuration<'a>(&'a Duration); @@ -46,3 +50,36 @@ pub const fn get_file_name(path: &str) -> &str { Err(_) => panic!("Failed to extract the file name from a path."), } } + +pub trait MutexExt { + type Guard<'a> + where + M: 'a, + T: 'a, + Self: 'a; + + /// Locks the mutex, while entering a critical section only during locking and unlocking. + /// + /// # Safety: + /// * The guard must not be kept across async `yield` points. + /// * This must not be called from within interrupt handlers. + /// Otherwise, a deadlock might occur. + unsafe fn lock_blocking(&self) -> Self::Guard<'_>; +} + +impl MutexExt for Mutex { + type Guard<'a> + = MutexGuard<'a, M, T> + where + M: 'a, + T: 'a, + Self: 'a; + + unsafe fn lock_blocking(&self) -> Self::Guard<'_> { + loop { + if let Ok(guard) = self.try_lock() { + return guard; + } + } + } +}