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

kernel: atomic: consistently use named type for atomic pointer values #33106

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Mar 7, 2021

There's a typedef for non-pointer values compatible with atomic non-pointer objects. Add a similar typedef for pointer values, and the corresponding macro for initializing atomic pointer types.

This also will simplify replacing the Zephyr atomic API with one based on C11 atomics, should that be desirable. C11 atomic pointer values are not void *.

Pulled out of #28902 which is stalled based on review and consensus on what Zephyr's atomic API should do. See also #32170 (comment) and related issues.

@github-actions github-actions bot added area: API Changes to public APIs area: Kernel area: Tests Issues related to a particular existing or missing test labels Mar 7, 2021
@pabigot
Copy link
Collaborator Author

pabigot commented Mar 7, 2021

The compliance check failure is a false positive (no new typedefs).

@zephyrbot zephyrbot added the area: Base OS Base OS Library (lib/os) label Mar 8, 2021
@zephyrbot zephyrbot requested a review from ceolin March 8, 2021 12:56
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me. Note that it's going to collide with some header rework in #32356 that splits up the atomic.h header across the variant backends, so one of us is going to have some rebase work to do.

@pabigot
Copy link
Collaborator Author

pabigot commented Mar 8, 2021

This seems fine to me. Note that it's going to collide with some header rework in #32356 that splits up the atomic.h header across the variant backends, so one of us is going to have some rebase work to do.

I wasn't aware of that. Any chance of a back-end that uses C11 API? I had one in progress, but reconciling the many implementations demotivated me.

I had thought to make use of this for a work-related bug in tcp, but couldn't because the pointer value I was going to use as the "in-use" flag is a pointer-to-function, which is not safely storable in a void *.

@ceolin
Copy link
Member

ceolin commented Mar 8, 2021

This seems fine to me. Note that it's going to collide with some header rework in #32356 that splits up the atomic.h header across the variant backends, so one of us is going to have some rebase work to do.

I wasn't aware of that. Any chance of a back-end that uses C11 API? I had one in progress, but reconciling the many implementations demotivated me.

I had thought to make use of this for a work-related bug in tcp, but couldn't because the pointer value I was going to use as the "in-use" flag is a pointer-to-function, which is not safely storable in a void *.

Use C11 on Zephyr should not be discussed in a particular feature context IMHO. It impacts other areas like safety, toolchains, ... Technically speaking I'd love to be able use it, but ... as I mentioned it impacts other areas, MISRA-C, for instance, currently does not support C11, only C90 and C99. Also, there are C11 features that are optional, this can impact tooolchains compatibility (even assuming that they support C11).

There's a typedef for non-pointer values compatible with atomic
non-pointer objects.  Add a similar typedef for pointer values, and
the corresponding macro for initializing atomic pointer types.

This also will simplify replacing the Zephyr atomic API with one
based on C11 atomics, should that be desirable.  C11 atomic pointer
values are not void*.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
@pabigot
Copy link
Collaborator Author

pabigot commented Mar 15, 2021

rebased on merged #32356

@carlescufi carlescufi merged commit f69a381 into zephyrproject-rtos:master Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Base OS Base OS Library (lib/os) area: Kernel area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants