Can __HAL_LOCK in stm32 code cause race condition issues

[Kefei Yao]

Posted on March 19, 2018 at 10:24

The __HAL_LOCK macro is defined as in stm32f1xx_hal_def.h

#define __HAL_LOCK(__HANDLE__) \

do{ \

if((__HANDLE__)->Lock == HAL_LOCKED) \

{ \

return HAL_BUSY; \

} \

else \

{ \

(__HANDLE__)->Lock = HAL_LOCKED; \

} \

}while (0)

So the Lock is read first and modified if needed. Would it cause race condition while the task calling __HAL_LOCK is preempted by a higher priority task which call __HAL_LOCK subsequently.  

For example, task1 calls __HAL_LOCK and find the lock is open. task 1 is preempted by task2 whose priority is higher than task1 right before task1 write the Lock as HAL_LOCKED. task2 finds the lock is open and set the Lock as HAL_LOCKED. The task2 goes to block state to wait for some resource. Then task1 runs again and continue to get the lock. This will potentially corrupt the resources protect by the __HAL_LOCK for task2.

I would be happy if someone could correct me if I'm wrong since I could learn a lesson from it

[Darius B]

Posted on March 19, 2018 at 11:18

Hi,

It seem to me, that your are right with multitasking. This code   

__HAL_LOCK

is dangerous.  It must be used from 1 task.

If you want use in multitask like you, then need use critical sections (disable/enable interrupts)  example (not tested).

#define __HAL_LOCK(__HANDLE__) \

do{ \

    __disable_interrupts();

   if((__HANDLE__)->Lock == HAL_LOCKED) \

   { \

      /*

return HAL_BUSY; if we return, our task lost

*/  \

    __enable_interrupts();   /*let other task unlock */  

\

Os_delay(1); /*let other task unlock */  \

   } \

   else \

   { \

      (__HANDLE__)->Lock = HAL_LOCKED; \

      __enable_interrupts(); /*let other task unlock */  \

      break; \

   } \

}while (1)

BR.

Darius Babrauskas

[Kefei Yao]

Posted on March 19, 2018 at 12:03

Thanks for the reply. What do you mean by 'if we return, our task lost'. The original __HAL_LOCK macro return HAL_BUSY if it is already locked by others. Shoudn't the thread safe version __HAL_LOCK follow the same regulation as the original one?

[Darius B]

Posted on March 19, 2018 at 12:17

I mean, that we lost task (maybe better job). See code below from stm 

if in   HAL_ADC_Start     __HAL_LOCK(hadc);  return,   then   ADC not start measure.

HAL_StatusTypeDef HAL_ADC_Start(ADC_HandleTypeDef* hadc)

{  HAL_StatusTypeDef tmp_hal_status = HAL_OK;  ADC_TypeDef       *tmpADC_Master;    /* Check the parameters */  assert_param(IS_ADC_ALL_INSTANCE(hadc->Instance));    /* Perform ADC enable and conversion start if no conversion is on going */  if (ADC_IS_CONVERSION_ONGOING_REGULAR(hadc) == RESET)  {    /* Process locked */    __HAL_LOCK(hadc);

.............................

BR.

Darius Babrauskas

[Kefei Yao]

Posted on March 20, 2018 at 03:21

Yes, the task content will not get executed if __HAL_LOCK fails. But the caller can get this status from the return value and decide whether to delay and retry.

[Darius B]

Posted on March 20, 2018 at 08:24

Yes you right, caller can get this status and decide whether to delay and retry.  But her we have dangerous, because   __HAL_LOCK  can return incorrect condition or can execute protected code  example:

&sharpdefine __HAL_LOCK(__HANDLE__) \

do{ \

if((__HANDLE__)->Lock == HAL_LOCKED) \

{ \

//other task switched her and unlock, so   return not correct condition- it return busy, but it free!

         return HAL_BUSY; \

} \

else \

{ \

//other task switched her and lock too, so   2 task use protected code (resource) !

         (__HANDLE__)->Lock = HAL_LOCKED; \

} \

}while (0)

[Morty Morty]

Posted on May 14, 2018 at 17:21

It's broken. See

https://community.st.com/0D50X00009XkeOGSAZ

for solutions.

[Darius B]

Posted on May 14, 2018 at 19:21

Thank you Morty.

I readed your link. But not sure that they are good solution. 

My solution are tested by me more times and worked Ok. 

BR.

Darius

[T J]

Posted on May 15, 2018 at 03:38

another reason to never use multi tasking.

I find it best to process all state machines within a single foreground loop.

that way, there is no issue of Printf being interrupted on your debug screen.

in this foreground loop we process each state machine sequentially, much like a Multitasking solution.

the down side is we never sleep, but in our case we have power to burn.

Our unit draws 6 Watts since we have real work to do, the processor only draws 60mA at 3.3V is 0.2Watts

Sleeping will drop that to about 1/3 (I guess), so we save 0.13Watts if we sleep.

not worth the huge debug issues when switching tasks in the middle of a print statement.

[AvaTar]

Posted on May 15, 2018 at 08:25

another reason to never use multi tasking.

IMHO a bit too broad of a statement.

There do exist processors with multiple physical cores, which are only properly exploited with a multitasking OS.

But for a plain Cortex M, I agree.

And comprehending multiple, concurrent processes (including it's consequences on data handling) is quite a task ...