Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spinlock APIs need documentation #20993

Closed
nordic-krch opened this issue Nov 26, 2019 · 4 comments · Fixed by #25092
Closed

spinlock APIs need documentation #20993

nordic-krch opened this issue Nov 26, 2019 · 4 comments · Fixed by #25092
Assignees
Labels
area: Documentation bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@nordic-krch
Copy link
Contributor

Describe the bug
When using spin lock in a following code:

struct k_spinlock l;
k_spinlock_key_t k;

k = k_spin_lock(&l);

I'm hitting assert in z_spin_lock_valid.

__ASSERT(z_spin_lock_valid(l), "Recursive spinlock");

k_spin_lock is validating lock structure which might not be initialize and validation includes checking one of the fields of uninitialized structure. I'm not sure then if struct k_spinlock should be somehow initialized before being used? If so, i don't see any init function. What about single core case?

@nordic-krch nordic-krch added bug The issue is a bug, or the PR is fixing a bug question labels Nov 26, 2019
@aescolar aescolar removed the bug The issue is a bug, or the PR is fixing a bug label Nov 26, 2019
@andyross
Copy link
Contributor

Spinlocks are valid when zero-initialized. The normal use case is to put it in bss, but if you want to use a local you can do "struct k_spinlock l = {};". There probably should be a K_SPINLOCK_INITIALIZER macro defined.

@nordic-krch
Copy link
Contributor Author

seems like spin lock is in general missing any documentation. no doxygen for k_spin_lock and k_spin_unlock:

static ALWAYS_INLINE k_spinlock_key_t k_spin_lock(struct k_spinlock *l)

@nashif nashif added area: Documentation bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug and removed question labels Dec 17, 2019
@nashif
Copy link
Member

nashif commented Dec 17, 2019

@andyross can you please add missing documentation?

@thomasthorsen
Copy link

I don't think the documentation added in #25092 actually addresses the main issue here:
"I'm not sure then if struct k_spinlock should be somehow initialized before being used?"

There is no init API and there is no part of the added documentation that states the memory must be initialised in any way (as far as I can tell). I encountered this in a spin lock that was allocated by k_malloc, which had actually worked for some time in our system, and then when some code was moved around, it started failing oddly with this, because the pre-existing memory contents happened to be different for the new location. In the old location it just happened to be zero, and nothing bad happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Documentation bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants