[BUG] STM32 HAL driver lock mechanism is not interrupt safe
STM32 HAL driver library is full of flawed and sub-optimal constructs. The most common one, which impacts almost all drivers, is the lock mechanism. It's a bad and limiting design and getting rid of it requires a major rewrite, but the worst fact is that it's not even interrupt safe and therefore doesn't provide locking for which it was introduced. The current __HAL_LOCK() (reformatted) code looks like this:
#define __HAL_LOCK(__HANDLE__) \
do{ \
if((__HANDLE__)->Lock == HAL_LOCKED) \
{ \
return HAL_BUSY; \
} \
else \
{ \
(__HANDLE__)->Lock = HAL_LOCKED; \
} \
}while (0U)Between testing and setting the ->Lock an interrupt can happen and also test and set the ->Lock. Therefore both - main thread and interrupt - will continue execution as if the object was unlocked and the interrupt will unlock it before the main thread has completed it's "locked" part, which makes it even more prone to next interrupt calls. The same will happen when higher priority interrupt interrupts lower priority interrupt. Additionally the ->Lock variable is not marked as volatile and therefore is prone to reorder by compiler optimization.
The proposed fix is simple and requires adding of only a few lines of code in stm32XXxx_hal_def.h files for all STM32 series:
#define __HAL_LOCK(__HANDLE__) \
do { \
uint32_t rPriMask = __get_PRIMASK(); \
__disable_irq(); \
if ((__HANDLE__)->Lock == HAL_UNLOCKED) { \
(__HANDLE__)->Lock = HAL_LOCKED; \
__set_PRIMASK(rPriMask); \
} else { \
__set_PRIMASK(rPriMask); \
return HAL_BUSY; \
} \
} while (0)
typedef volatile enum {
HAL_UNLOCKED = 0x00U,
HAL_LOCKED = 0x01U
} HAL_LockTypeDef;Thus this will make that bad construct at least interrupt safe and actually provide locking as was intended.
Note that __HAL_UNLOCK() code doesn't need modifications as it already is atomic.