Need a "lock" wrapper around k_sem

[andyross]

Lots and lots and lots of zephyr code uses semaphores as a blocking mutex when irq_lock() would be a latency problem, e.g.:

static K_SEM_DEFINE(sem, 1, 1);
...
{
    k_sem_take(&sem);
    /* critical section */
    k_sem_give(&sem);
}

This works well, and is a simple idiom that is easily understood.

Nonetheless the semantics of a proper mutex and a semaphore are a little different: semaphores allow for multiple gives, and thus for nested calls that only deadlock "sometimes". The initial count of a mutex must always be 1, while semaphores naturally like to start at zero. Semaphores may be used from multiple threads where a mutex must always be locked and unlocked out of the same thread. All of these represent "mistake points" that a simpler mutex API would not have.

Also, there's a practical problem discovered in #8368: there are flash drivers that use a sempahore lock like this, and thus they refuse to work when CONFIG_MULTITHREADING=n because the semaphore implementation requires the threading APIs. But in that particular case, there are no other user threads and so the a proper lock API (but not a semaphore API) would be able to degrade to a noop and thus preserve the driver semantics without worry.

Zephyr should have a simple mutex lock API that is a wrapper around k_sem. It should be typesafe (i.e. not interoperable with the semaphore calls) and have assertions included to validate the appropriate rules. But at runtime it should produce the same code.

(Note: the existing k_mutex abstraction is not a candidate. That is a priority-inheriting mutex with significant code complexity and performance overhead.)

[pabigot]

It sounds like this is a functional requirement for #27415, and would require that uses of k_sem for locks in the tree (if in code expected to work without threads) switch to the new API.

A concern is where the semaphore is used as mutex protection, but in full multithreading context a non-forever delay is used to detect errors. It's not clear whether replacing it with something that doesn't support delay would be acceptable.

[nashif]

It sounds like this is a functional requirement for #27415,

how? the requirement says:

A subset of driver classes, and within them only those that have been written with non-multithreaded mode in mind:

[pabigot]

@nashif Yes, I was confused; it's not mutex but synchronization between ISR and non-ISR that needs a solution for no-multithreading.

Back to the original question: If we're going to use a lock wrapper around semaphore to control mutex for drivers, does that lock require support for a timeout? @tbursztyka relates to your #24511.

In #25678 I'd thought that priority inheritance might be a useful feature for driver locks, but after reading this I'm not going to argue for it.

In any case if we can agree on the expected semantics it seems adding this would be pretty straightforward and would help make progress towards parts of #22941.

[andyross]

(Doing a grooming pass on bugs/issues that are sitting around with my name on or near them. No one pays me right now so I get to be an unaccountable whirlwind of chaos!)

Closing, because for small (and in-kernel) critical sections spinlocks have largely become the standard solution, and also for the same reason as #8497. It's true that we could do with a rework of the core kernel synchronization primitives, but what we absolutely don't want is yet another API. Instead we probably want a simple core with a handful of semantic wrappers that map to the existing APIs.