I'm trying to understand code which is written by another programmer. It used I²C communication to write data on the EEPROM of an STM32 microcontroller.
Generally I understood how his code works, but I can't understand why he used the HAL_LOCK and HAL_UNLOCK functions.
These is the code of these methods:
typedef enum
{
HAL_UNLOCKED = 0x00U,
HAL_LOCKED = 0x01U
} HAL_LockTypeDef;
#if (USE_RTOS == 1)
/* Reserved for future use */
#error "USE_RTOS should be 0 in the current HAL release"
#else
#define __HAL_LOCK(__HANDLE__) \
do{ \
if((__HANDLE__)->Lock == HAL_LOCKED) \
{ \
return HAL_BUSY; \
} \
else \
{ \
(__HANDLE__)->Lock = HAL_LOCKED; \
} \
} while (0)
#define __HAL_UNLOCK(__HANDLE__) \
do{ \
(__HANDLE__)->Lock = HAL_UNLOCKED; \
} while (0)
In which cases can these methods be used?
Someone was trying to implement a basic, advisory, non-recursive mutex with these macros (sometimes called a "lock" or "critical section", but those terms have other meanings; "mutex" is unambiguous). The idea is that you write code like this:
int frob_handle(handle_t hdl)
{
__HAL_LOCK(hdl);
hdl->frob_counter += 1;
__HAL_UNLOCK(hdl);
return 0;
}
And then only one thread of execution at a time can execute the statement hdl->frob_counter += 1. (In a real program, there would probably be quite a bit more code in there.) It's "advisory" because nothing stops you from forgetting to use the mutex when it's needed, and it's "non-recursive" because you can't call __HAL_LOCK a second time if you already have it locked. These are both relatively normal properties for a mutex to have.
In comments on the question, I said that these macros are "catastrophically buggy" and "I believe you are better off not using them at all." The most important problem is that __HAL_LOCK is not atomic.
// This code is incorrect.
if ((__HANDLE__)->Lock == HAL_LOCKED)
return HAL_BUSY;
else
(__HANDLE__)->Lock = HAL_LOCKED;
Imagine that two threads of execution are trying to acquire the lock at the exact same time. They will both fetch __HANDLE__->Lock from memory at the same time, so they will both observe its value to be HAL_UNLOCKED, and they will both go on to the code that was supposed only to be executed by one thread at a time. It's maybe easier to see the problem if I write out the assembly language that might be generated:
; This code is incorrect.
; r1 contains the __HANDLE__ pointer
load.b r0, Lock(r1)
test.b r0
bnz .already_locked
inc.b r0
store.b Lock(r1), r0
...
.already_locked:
mov.b r0, #HAL_BUSY
ret
There's nothing that would prevent both threads from executing the load instruction simultaneously, and thus both observing the mutex to be unlocked. Even if there's only one CPU, an interrupt could fire while thread 1 is in between the load and the store, causing a context switch and allowing thread 2 to execute the load before thread 1 can execute the store.
For the mutex to do its job, you must somehow ensure that it is impossible for two concurrent threads both to load HAL_UNLOCKED from __HANDLE__->Lock, and this cannot be done with ordinary C. In fact, it can't be done with ordinary machine language; you need to use special instructions, such as compare-and-swap.
If your compiler implements C2011, then you can get at those special instructions using the new feature of atomic types, but I don't know how to do it off the top of my head, and I'm not going to write out something that might be wrong. Otherwise, you need to use either compiler extensions or hand-written assembly.
A second problem is that __HAL_LOCK doesn't implement the operation that is usually called "lock". "Lock" is supposed to wait if it can't acquire the lock immediately, but what __HAL_LOCK does is fail. That operation's name is "try-lock", and the macro should be named accordingly. Also, macros that may cause the calling function to return are considered bad practice.