[BUG] Broken state management in STM32 HAL drivers

[Piranha]

Here is a code of HAL_I2SEx_TransmitReceive() function on F4 series reduced for the sake of example:

 

HAL_StatusTypeDef HAL_I2SEx_TransmitReceive(I2S_HandleTypeDef *hi2s,
 uint16_t *pTxData,
 uint16_t *pRxData,
 uint16_t Size,
 uint32_t Timeout)
{
 // ...
 HAL_StatusTypeDef errorcode = HAL_OK;

 if (hi2s->State != HAL_I2S_STATE_READY)
 {
 errorcode = HAL_BUSY;
 goto error;
 }

 // ...

 /* Process Locked */
 __HAL_LOCK(hi2s);
 
 // ...
 
error :
 hi2s->State = HAL_I2S_STATE_READY;
 __HAL_UNLOCK(hi2s);
 return errorcode;
}

 

The if block and it's goto is completely broken.

1. If the state is not HAL_I2S_STATE_READY, the code just sets it to that state and does the __HAL_UNLOCK(), damaging the current actual driver state.
2. The __HAL_UNLOCK() should not be executed without the corresponding __HAL_LOCK().
3. The state variables cannot be accessed outside of the critical section, because it breaks the whole logic behind the critical section.

Just a quick search reveals, that, for example, HAL_SPI_Receive() on L4 series and HAL_SPI_Receive() on F7 series has the same issues. The latter even writes the state variable outside of the critical section, which is always broken! And these are not the only such functions in those files - the code is full of these bugs. This report is not extensive and most likely the code for other drivers and series are similarly full of these bugs.

The overall inconsistency of some drivers using the goto, while most of the HAL doesn't, is also an indicator of chaos because of incompetence. And on top of this I will remind that, even 9 years since the beginning of HAL, the "HAL lock" itself and therefore almost all drivers are still broken.

[TDK]

I agree with the sentiment here: if you're going to implement a lock/mutex system in order to make the code thread-safe, do it right. Otherwise, it only provides a false sense of thread safety and is going to trip up customers who don't know any better.

The HAL_LOCK/HAL_UNLOCK is entirely unnecessary if your code is only calling things in the main thread, or has some similar execution barrier. Take it out or do it correctly.

[Patrice LF]

Hello @Piranha

I confirm the issues and there are reported internally.

Internal ticket numbers: 159365, 159378 and 159380 (This are internal tracking numbers not accessible or usable by customers).

Regards

Patrice