From 766b5475631a24769777ce52b30f0fa520db0041 Mon Sep 17 00:00:00 2001 From: cepetr Date: Wed, 28 Aug 2024 07:50:09 +0200 Subject: [PATCH] refactor(embed/core): replace enable/disable_irq by irq_lock/unlock [no changelog] --- core/embed/bootloader/main.c | 2 +- core/embed/firmware/mpconfigport.h | 4 +- core/embed/trezorhal/stm32f4/bootutils.c | 2 +- core/embed/trezorhal/stm32f4/i2c_bus.c | 12 +++--- core/embed/trezorhal/stm32f4/irq.h | 54 +++++++++++++++++++----- core/embed/trezorhal/stm32f4/sdcard.c | 6 +-- core/embed/trezorhal/stm32f4/systick.c | 4 +- core/embed/trezorhal/stm32f4/systimer.c | 22 +++++----- core/embed/trezorhal/stm32u5/i2c_bus.c | 12 +++--- core/embed/trezorhal/stm32u5/sdcard.c | 6 +-- 10 files changed, 79 insertions(+), 45 deletions(-) diff --git a/core/embed/bootloader/main.c b/core/embed/bootloader/main.c index 5efaef1fc..6f471d12b 100644 --- a/core/embed/bootloader/main.c +++ b/core/embed/bootloader/main.c @@ -345,7 +345,7 @@ void real_jump_to_firmware(void) { __attribute__((noreturn)) void jump_to_fw_through_reset(void) { display_fade(display_backlight(-1), 0, 200); - disable_irq(); + __disable_irq(); delete_secrets(); NVIC_SystemReset(); for (;;) diff --git a/core/embed/firmware/mpconfigport.h b/core/embed/firmware/mpconfigport.h index b80f257c6..0e9e990b1 100644 --- a/core/embed/firmware/mpconfigport.h +++ b/core/embed/firmware/mpconfigport.h @@ -194,8 +194,8 @@ typedef long mp_off_t; #include "irq.h" -#define MICROPY_BEGIN_ATOMIC_SECTION() disable_irq() -#define MICROPY_END_ATOMIC_SECTION(state) enable_irq(state) +#define MICROPY_BEGIN_ATOMIC_SECTION() irq_lock() +#define MICROPY_END_ATOMIC_SECTION(state) irq_unlock(state) #define MICROPY_EVENT_POLL_HOOK \ do { \ extern void mp_handle_pending(bool); \ diff --git a/core/embed/trezorhal/stm32f4/bootutils.c b/core/embed/trezorhal/stm32f4/bootutils.c index 08ffc2147..4667fc5e8 100644 --- a/core/embed/trezorhal/stm32f4/bootutils.c +++ b/core/embed/trezorhal/stm32f4/bootutils.c @@ -72,7 +72,7 @@ void reboot_to_bootloader(void) { #ifdef STM32U5 // extern uint32_t g_boot_command; g_boot_command = boot_command; - disable_irq(); + __disable_irq(); delete_secrets(); NVIC_SystemReset(); #else diff --git a/core/embed/trezorhal/stm32f4/i2c_bus.c b/core/embed/trezorhal/stm32f4/i2c_bus.c index 14e878887..035bc4bec 100644 --- a/core/embed/trezorhal/stm32f4/i2c_bus.c +++ b/core/embed/trezorhal/stm32f4/i2c_bus.c @@ -367,9 +367,9 @@ void i2c_bus_close(i2c_bus_t* bus) { } i2c_status_t i2c_packet_status(i2c_packet_t* packet) { - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); i2c_status_t status = packet->status; - enable_irq(irq_state); + irq_unlock(irq_key); return status; } @@ -460,14 +460,14 @@ i2c_status_t i2c_bus_submit(i2c_bus_t* bus, i2c_packet_t* packet) { packet->status = I2C_STATUS_PENDING; // Insert packet into the queue - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); if (i2c_bus_add_packet(bus, packet)) { // The queue was empty, start the operation if (!bus->callback_executed && !bus->abort_pending) { i2c_bus_head_continue(bus); } } - enable_irq(irq_state); + irq_unlock(irq_key); return I2C_STATUS_OK; } @@ -478,7 +478,7 @@ void i2c_bus_abort(i2c_bus_t* bus, i2c_packet_t* packet) { return; } - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); if (packet->status == I2C_STATUS_PENDING) { if (i2c_bus_remove_packet(bus, packet) && bus->next_op > 0) { @@ -502,7 +502,7 @@ void i2c_bus_abort(i2c_bus_t* bus, i2c_packet_t* packet) { packet->status = I2C_STATUS_ABORTED; } - enable_irq(irq_state); + irq_unlock(irq_key); } // Completes the current packet by removing it from the queue diff --git a/core/embed/trezorhal/stm32f4/irq.h b/core/embed/trezorhal/stm32f4/irq.h index b237d7b08..525bb9204 100644 --- a/core/embed/trezorhal/stm32f4/irq.h +++ b/core/embed/trezorhal/stm32f4/irq.h @@ -36,22 +36,56 @@ extern uint32_t irq_stats[IRQ_STATS_MAX]; #define IRQ_EXIT(irq) #endif +typedef uint32_t irq_key_t; + // Checks if interrupts are enabled -#define IS_IRQ_ENABLED(state) (((state) & 1) == 0) +#define IS_IRQ_ENABLED(key) (((key) & 1) == 0) // Get the current value of the CPU's exception mask register. // The least significant bit indicates if interrupts are enabled or disabled. -static inline uint32_t query_irq(void) { return __get_PRIMASK(); } +static inline irq_key_t query_irq(void) { return __get_PRIMASK(); } -// Restore the CPU's exception mask register to a previous state -static inline void enable_irq(uint32_t state) { __set_PRIMASK(state); } +// Disables interrupts and returns the previous interrupt state. +// +// This function is used to create critical sections by disabling interrupts +// on a Cortex-M platform. It returns the current state of the PRIMASK register, +// which controls the global interrupt enable/disable state. +// +// Important: +// - The `"memory"` clobber is included to prevent the compiler from reordering +// memory operations across this function, ensuring that all memory accesses +// efore `irq_lock()` are completed before interrupts are disabled. +// - The order of operations on non-volatile variables relative to this +// function is not guaranteed without memory barriers or other +// synchronization mechanisms. +// - When using Link-Time Optimization (LTO), ensure that the behavior of these +// functions is thoroughly tested, as LTO can lead to more aggressive +// optimizations. While GCC typically respects the order of `volatile` +// operations, this is not guaranteed by the C standard. +static inline irq_key_t irq_lock(void) { + uint32_t key; + __asm volatile( + "MRS %0, PRIMASK\n" + "CPSID i" + : "=r"(key) + : + : "memory" // Clobber memory to ensure correct memory operations + ); + return key; +} -// Disable all interrupts and return the current state of the -// CPU's exception mask register -static inline uint32_t disable_irq(void) { - uint32_t state = __get_PRIMASK(); - __disable_irq(); - return state; +// Restores the interrupt state to what it was before `irq_lock`. +// +// This function re-enables interrupts based on the PRIMASK state passed to it. +// It should be used in conjunction with `irq_lock` to restore the previous +// interrupt state after a critical section. +static inline void irq_unlock(irq_key_t key) { + __asm volatile( + "MSR PRIMASK, %0\n" + : + : "r"(key) + : "memory" // Clobber memory to ensure correct memory operations + ); } // IRQ priority levels used throughout the system diff --git a/core/embed/trezorhal/stm32f4/sdcard.c b/core/embed/trezorhal/stm32f4/sdcard.c index 432e5cb49..21c7b099e 100644 --- a/core/embed/trezorhal/stm32f4/sdcard.c +++ b/core/embed/trezorhal/stm32f4/sdcard.c @@ -253,13 +253,13 @@ static HAL_StatusTypeDef sdcard_wait_finished(SD_HandleTypeDef *sd, uint32_t start = HAL_GetTick(); for (;;) { // Do an atomic check of the state; WFI will exit even if IRQs are disabled - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); if (sd->State != HAL_SD_STATE_BUSY) { - enable_irq(irq_state); + irq_unlock(irq_key); break; } __WFI(); - enable_irq(irq_state); + irq_unlock(irq_key); if (HAL_GetTick() - start >= timeout) { return HAL_TIMEOUT; } diff --git a/core/embed/trezorhal/stm32f4/systick.c b/core/embed/trezorhal/stm32f4/systick.c index 6de800b09..1028980d4 100644 --- a/core/embed/trezorhal/stm32f4/systick.c +++ b/core/embed/trezorhal/stm32f4/systick.c @@ -106,7 +106,7 @@ void systick_update_freq(void) { uint64_t systick_cycles(void) { systick_driver_t* drv = &g_systick_driver; - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); // Current value of the SysTick counter uint32_t val = SysTick->VAL; @@ -125,7 +125,7 @@ uint64_t systick_cycles(void) { uint64_t cycles = drv->cycles + ((val > 0) ? (drv->cycles_per_ms - val) : 0); - enable_irq(irq_state); + irq_unlock(irq_key); return cycles; } diff --git a/core/embed/trezorhal/stm32f4/systimer.c b/core/embed/trezorhal/stm32f4/systimer.c index 8636d3c6c..dd1d426d6 100644 --- a/core/embed/trezorhal/stm32f4/systimer.c +++ b/core/embed/trezorhal/stm32f4/systimer.c @@ -96,7 +96,7 @@ systimer_t* systimer_create(systimer_callback_t callback, void* context) { return NULL; } - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); // Find a free timer entry for (int i = 0; i < MAX_SYSTIMERS; i++) { @@ -108,13 +108,13 @@ systimer_t* systimer_create(systimer_callback_t callback, void* context) { timer->context = context; timer->callback = callback; - enable_irq(irq_state); + irq_unlock(irq_key); return timer; } } // No free timer entry found - enable_irq(irq_state); + irq_unlock(irq_key); return NULL; } @@ -137,11 +137,11 @@ void systimer_set(systimer_t* timer, uint32_t delay_ms) { uint64_t delay = systick_us_to_cycles((uint64_t)delay_ms * 1000); uint64_t expiration = systick_cycles() + delay; - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); timer->expiration = expiration; timer->period = 0; timer->scheduled = true; - enable_irq(irq_state); + irq_unlock(irq_key); } void systimer_set_periodic(systimer_t* timer, uint32_t period_ms) { @@ -154,11 +154,11 @@ void systimer_set_periodic(systimer_t* timer, uint32_t period_ms) { uint64_t period = systick_us_to_cycles((uint64_t)period_ms * 1000); uint64_t expiration = systick_cycles() + period; - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); timer->expiration = expiration; timer->period = period; timer->scheduled = true; - enable_irq(irq_state); + irq_unlock(irq_key); } bool systimer_unset(systimer_t* timer) { @@ -168,10 +168,10 @@ bool systimer_unset(systimer_t* timer) { return false; } - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); bool was_scheduled = timer->scheduled; timer->scheduled = false; - enable_irq(irq_state); + irq_unlock(irq_key); return was_scheduled; } @@ -182,10 +182,10 @@ systimer_key_t systimer_suspend(systimer_t* timer) { return false; } - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); bool was_suspended = timer->suspended; timer->suspended = true; - enable_irq(irq_state); + irq_unlock(irq_key); return was_suspended; } diff --git a/core/embed/trezorhal/stm32u5/i2c_bus.c b/core/embed/trezorhal/stm32u5/i2c_bus.c index 4237cd4b3..5aa413a6f 100644 --- a/core/embed/trezorhal/stm32u5/i2c_bus.c +++ b/core/embed/trezorhal/stm32u5/i2c_bus.c @@ -360,9 +360,9 @@ void i2c_bus_close(i2c_bus_t* bus) { } i2c_status_t i2c_packet_status(i2c_packet_t* packet) { - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); i2c_status_t status = packet->status; - enable_irq(irq_state); + irq_unlock(irq_key); return status; } @@ -499,14 +499,14 @@ i2c_status_t i2c_bus_submit(i2c_bus_t* bus, i2c_packet_t* packet) { packet->status = I2C_STATUS_PENDING; // Insert packet into the queue - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); if (i2c_bus_add_packet(bus, packet)) { // The queue was empty, start the operation if (!bus->callback_executed && !bus->abort_pending) { i2c_bus_head_continue(bus); } } - enable_irq(irq_state); + irq_unlock(irq_key); return I2C_STATUS_OK; } @@ -517,7 +517,7 @@ void i2c_bus_abort(i2c_bus_t* bus, i2c_packet_t* packet) { return; } - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); if (packet->status == I2C_STATUS_PENDING) { if (i2c_bus_remove_packet(bus, packet) && bus->next_op > 0) { @@ -546,7 +546,7 @@ void i2c_bus_abort(i2c_bus_t* bus, i2c_packet_t* packet) { packet->status = I2C_STATUS_ABORTED; } - enable_irq(irq_state); + irq_unlock(irq_key); } // Completes the current packet by removing it from the queue diff --git a/core/embed/trezorhal/stm32u5/sdcard.c b/core/embed/trezorhal/stm32u5/sdcard.c index b040e186a..714a48d4e 100644 --- a/core/embed/trezorhal/stm32u5/sdcard.c +++ b/core/embed/trezorhal/stm32u5/sdcard.c @@ -256,13 +256,13 @@ static HAL_StatusTypeDef sdcard_wait_finished(SD_HandleTypeDef *sd, uint32_t start = HAL_GetTick(); for (;;) { // Do an atomic check of the state; WFI will exit even if IRQs are disabled - uint32_t irq_state = disable_irq(); + irq_key_t irq_key = irq_lock(); if (sd->State != HAL_SD_STATE_BUSY) { - enable_irq(irq_state); + irq_unlock(irq_key); break; } __WFI(); - enable_irq(irq_state); + irq_unlock(irq_key); if (HAL_GetTick() - start >= timeout) { return HAL_TIMEOUT; }