Mutex implementation for a uniprocessor bare metal embedded OS

I wrote this recently for one of my projects. Are there any error you can spot or a feature which could be implemented without eating up resources or some optimisations? Also, this isn't meant for multi-cores.

```
/*****
* @file mutex.c
* @date 31st August 2012
* @brief Generic implementation of mutex, read /notes/thread-safety
****/

#include

OS_ERR mutex_acquire_try(mutex_t *mutex)
{
if(unlikely(cpu_atomic_cmpxchg(&mutex->lock, OS_UNLOCKED, OS_LOCKED) != OS_OKAY))
{
if(mutex->owner == current_thread)
goto recursive;

return OS_EBUSY;
}

mutex->owner = current_thread;

recursive:
mutex->recnt++;
return OS_OKAY;
}

static inline OS_ERR mutex_acquire_helper(mutex_t *mutex, time_t timeout)
{
while(unlikely(cpu_atomic_cmpxchg(&mutex->lock, OS_UNLOCKED, OS_LOCKED) != OS_OKAY))
{
if(mutex->owner == current_thread)
goto recursive;

if(mutex->lock == OS_DEAD)
return OS_EINVAL;

if(current_thread->priority >= mutex->owner->priority)
thread_boost(mutex->owner);

if(thread_queue_block(&mutex->queue, timeout) != OS_OKAY)
return OS_ETIMEOUT;
}

mutex->owner = current_thread;

recursive:
mutex->recnt++;
return OS_OKAY;
}

OS_ERR mutex_acquire_timeout(mutex_t *mutex, time_t timeout)
{
return mutex_acquire_helper(mutex, timeout);
}

OS_ERR mutex_acquire(mutex_t *mutex)
{
return mutex_acquire_helper(mutex, OS_TIME_INFINITE);
}

OS_ERR mutex_release(mutex_t *mutex)
{
if(mutex->owner != current_thread)
return OS_EINVAL;

if(--mutex->recnt)
goto exit;

thread_deboost(mutex->owner);
mutex->owner = NULL;
cpu_atomic_set(&mutex->lock, OS_UNLOCKED);
thread_queue_wake_one_now(&mutex->queue);

exit:
return OS_OKAY;
}

void mute

Does this really work? You call cpu_atomic_cmpxchg without taking any notice of its return value. I guess the function must return true or false (1 or 0 etc) according to whether the lock was already taken or not and it seems likely that you should take notice of that.

Also, if it were my code I'd drop all the unlikely() and likely() calls. For my money, they just obscure the code and I doubt they are necessary.

I haven't read the functions in detail, so maybe I misunderstood the code...

EDIT: Read it again and have some comments:

-
Your point 10: "Mutex MUST not be freed without calling mutex_deinit." is
strange. Isn't there a difference between freeing the mutex (lock/free is
what you normally do, no?) and de-initialising it (which you would do when
it is no longer needed, if at all)?

-
'restrict' used in functions with a single parameter has no purpose

-
Reviewers and readers would be more likely to understand what is going on if
your functions had some commets as to their intended purpose/use/return
values etc.

-
How do the 'atomic' functions wait for mutex availability?

-
Shouldn't mutex_init fail on an already-initialised mutex?

-
How do you know when it is safe to deinit a mutex?

-
Your mutex flags are confusing. You have DEAD, BUSY, LOCKED, SUCCESS,
READY. Seems likely to me that there is some redundancy here. Or
conflating state flags and return values.

-
What is the point in all those MUTX flags if you assume the value of one of
them (in if(unlikely(ret))) - assuming the atomic function returns one.

-
What is the difference between BUSY and LOCKED?

-
mutex_acquire_timeout doesn't tell the caller whether a timeout occurred!
(or whether enquing falied!)

-
So if the mutex has an owner already in mutex_acquire_timeout, you put the
thread onto a queue, with a timeout. Presumably thread_queue_enqueue
returns either when awoken by thread_queue_wake_highest_priority in
mutex_release, or when there is a timeout. But you carry on and
replace the mutex owner whatever the case...

-
Your point 4 says a mutex can only be released by its owner, and yet
mutex_release goes ahead and plays with the mutex queue whoever calls it.
Also sets state (ie releasing the local lock) before playing with the queue; surely there will be a race condition there.

Sorry, but I don't believe this code 'works' in a meaningful way. Why are
you writing these primitives anyway? Why not use an existing kernel? It is
generally a mistake to write your own (although I've worked at several that
have done, with bad consequences).

StackExchange Code Review Q#14342, answer score: 3