Atomic GPIO register modifications in stm32u5xx_hal_gpio.c

[RaduB]

Hello, 

We have found that, in an OS context, if two tasks change the configuration of different GPIOs belonging to the same port, then the register can get corrupted due to a RMW race.

It seems the RMW operations are not protected:

// stm32u5xx_hal_gpio.c :232
tmp = GPIOx->MODER;
tmp &= ~(LPGPIO_MODER_MOD0 << position);
tmp |= ((pGPIO_Init->Mode & GPIO_MODE_OUTPUT_PP) << position);
GPIOx->MODER = tmp;

Our solution is as follows:

ATOMIC_MODIFY_REG(GPIOx->MODER,
 (LPGPIO_MODER_MOD0 << position),
 ((pGPIO_Init->Mode & GPIO_MODE_OUTPUT_PP) << position));

1. Is there any reason why these operations were not atomic in the first place ?
2. Do you see any issue with our solution ?
3. Is there a better way to handle this ?
   
   
   

[TDK]

> Is there any reason why these operations were not atomic in the first place ?

LDREX/STREX is slow and not available on all cores. Modifying MODER in several different threads is not typical.

> Do you see any issue with our solution ?

No, apart from the above statements.

> Is there a better way to handle this ?

Only modify MODER during startup, or only in one thread. Or do what you're doing. All valid solutions.

[mƎALLEm]

Or use a mutex to protect the access to the GPIO ODR register.

[RaduB]

Thank you for your reply !

@TDK 

The use-case is fairly common, I assume. This is an Ultra Low Power MCU. In order to achieve the longest battery life, the board as a whole,  must be kept most time in its lowest power power state. This means that peripherals  and clocks are enabled only when used and external devices are driven only when needed.

In order to achieve this low power state (board level) sometimes the state or functions of the pins must be modified. Naturally, this happens in the context of the task handling the specific functionality (in this case, on the same port there are SPI and I2C peripherals connected, handled by separate tasks).

@mƎALLEm 

The resource to protect in this case would have to be GPIOB as a whole, since there are multiple registers having the same implementation pattern. Then this would have to be extended to all GPIO ports where the behavior is possible.

Get_Mutex(GPIOB_Mutex,WAIT_FOREVER)
HAL_GPIO_Init(...)
Release_Mutex(GPIOB_Mutex)

Did I understand your proposal correctly ? 

 

[mƎALLEm]

---

@RaduB wrote:

@mƎALLEm 

The resource to protect in this case would have to be GPIOB as a whole, since there are multiple registers having the same implementation pattern. Then this would have to be extended to all GPIO ports where the behavior is possible.

Get_Mutex(GPIOB_Mutex,WAIT_FOREVER)
HAL_GPIO_Init(...)
Release_Mutex(GPIOB_Mutex)

Did I understand your proposal correctly ? 

 

---

Create an OS abstraction layer for each GPIO HAL API you need to protect for GPIOB.

For example for GPIO toggle:

void GPIO_TogglePin_portb_os(uint16_t pin) 
{ 
 Get_Mutex(GPIOB_Mutex,WAIT_FOREVER);
 HAL_GPIO_TogglePin(GPIOB, pin); 
 Release_Mutex(GPIOB_Mutex) 
}

Other ports will be accessible without mutex and in the suggested code only port B is used.

[RaduB]

duplicate by mistake

[gbm]

@mƎALLEm TogglePin should not require any locking as it doesn't perform RMW access.

[mƎALLEm]

---

@gbm wrote:

@mƎALLEm TogglePin should not require any locking as it doesn't perform RMW access.

---

I'm not specifically talking about toggle pin (it was just an example), here I'm talking about protecting the GPIOB from any simultaneous access using the HAL.

Meanwhile, this is the HAL toggle pin implementation:

void HAL_GPIO_TogglePin(GPIO_TypeDef *GPIOx, uint16_t GPIO_Pin)
{
 uint32_t odr;

 /* Check the parameters */
 assert_param(IS_GPIO_PIN(GPIO_Pin));

 /* get current Output Data Register value */
 odr = GPIOx->ODR;

 /* Set selected pins that were at low level, and reset ones that were high */
 GPIOx->BSRR = ((odr & GPIO_Pin) << GPIO_NUMBER) | (~odr & GPIO_Pin);
}

Which is not atomic.

Otherwise -and indeed- to set/reset a pin he needs to access directly BSRR/BRR registers for atomic access:

[TDK]

@mƎALLEm HAL_GPIO_TogglePin can be called by multiple threads. If they all operate on different pins, the operations complete without issue. In this way, it is atomic. Perhaps thread-safe is the better term here, as that is what this thread means by atomic. ODR is never written, only BSRR.

It used to operate on ODR directly, but this was fixed many years ago.

[RaduB]

@mƎALLEm 

I understood the proposal.

We will keep our solution for now, understanding the limitations.

It seems there are multiple places where these race conditions can occur. Some baked-in hooks for defining these protections in OS environment would be useful and would save many of such issue. Or some big red warning flag when an OS is dragged in would help.

Determining where such protections are needed (in case an OS is used) can be a non-trivial, error prone task.
Other peripherals that may be impacted that come to mind are RCC, NVIC, EXTI and probably others.

Some HAL support would help here, considering the above.

Thank you all for the replies so far !

[gbm]

@mƎALLEm (Tilen?) I can see two problems here:

1. The old, still not fixed HAL bug: TogglePin is, and always was, simply incorrect.

GPIOx->BSRR = ((odr & GPIO_Pin) << GPIO_NUMBER) | (~odr & GPIO_Pin);

contains non-needed operation which cannot be optimized out by the compiler, so it's much slower than it should be.

It should be written as:

GPIOx->BSRR = (GPIO_Pin << GPIO_NUMBER) | (~odr & GPIO_Pin);

(BTW: the function should be defined in a header file as static inline rather than in .c file as normal function, as it currently is.)

2. In your example, if these two tasks toggle different pins, there is no problem - no danger of incorrect operation. If they both toggle the same pin, then still there is no problem, this time logically, cause we must assume that these toggle operation may happen almost simultaneously and toggle the pin in a hard-to-predict way which is still supposed to be logically correct in this strange application.