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 ?
   
   
   

[mƎALLEm]

@TDK 

Yes I mean atomic for thread safe context. Let's assume two tasks (Task1 & Task2) are accessing the same GPIO port. Task2 has the highest priority.

  odr = GPIOx->ODR;
  /* <- Task with higher priority becomes running here after ODR register reading *\

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

Now Task1 reads the ODR register by executing the line odr = GPIOx->ODR; and stores it in its stack. Just after that line Task2 with the highest priority interrupts Task1 and executes HAL_GPIO_TogglePin(). It sets the BSRR with another values which in turn modifies ODR register content. When the scheduler switches to Task1, ODR has already another value than what it was saved in its stack. 

By executing the line GPIOx->BSRR = ((odr & GPIO_Pin) << GPIO_NUMBER) | (~odr & GPIO_Pin); will set the an old (wrong) mask having the value "odr".

And this is what I understood from the OP post: two tasks are accessing the same resource (GPIO) : "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."

[TDK]

> Yes I mean atomic for thread safe context.

HAL_GPIO_TogglePin only reads the relevant bit of ODR. It doesn't matter if it reads the value before the other thread updates or after, the relevant bit is the same.

It is 100% thread-safe to call this from two different threads. There are no race conditions affecting the result.

HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_1);

HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_2);

[mƎALLEm]

---

---

Sorry I'm not convinced :). The bit mask has changed, therefore the result written to BSRR is not what is intended to be.. I've already explained the staff in my previous post ;)

[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.