RFC: device: convert device struct instances to const #25208
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pabigot
changed the title
|
Some checks failed. Please fix and resubmit. checkpatch issuesGitlint issuesCommit 01bf452: Commit a0eca13: Commit b1ad4bc: Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
pabigot
force-pushed
the
nordic/issue/22941a
branch
2 times, most recently
from
d9cc292 to
a0bec95
Compare
|
Existing code that passes a device pointer as an argument to a function that receives a |
tbursztyka
reviewed
May 12, 2020
| @@ -254,6 +271,7 @@ struct device { | |||
| const void *config_info; | |||
| const void *driver_api; | |||
| void * const driver_data; | |||
| struct device_context * const driver_context; | |||
There was a problem hiding this comment.
I'd just call it context (imo all these attributes should be renamed: config_info -> config, driver_api -> api, driver_data -> data), but that's a side note.
What's the benefit exposing it that way? (adding a pointer in the struct, instead of a fully separate array as in #24873). It's a trade-off between ROM occupation and runtime computation, anything else than that?
In the very end, we should merge driver_data and driver_context, and provide the means for the drivers to access the data seamlessly (via a macro or else). I just did not go myself in that path in my RFC because it generates a lot of changes in drivers
There was a problem hiding this comment.
Exactly as you say: the right way is to put the common context into a prefix of the driver-specific data. That can't be done if they're separate objects.
This is a prototype using a different object relationship than yours; we don't know yet what the best path will be. Better to try several and compare their pros and cons.
There was a problem hiding this comment.
That can't be done if they're separate objects.
Of course it can be. Current RFCs (mine and yours) are only trying to solve the device instance as constant.
Once done then we can move on with modifying driver_data to provide also a driver context.
But, as I said in my other comment, if the goal is only to solve the device instances to be constants, there is 0 point to introduce right now a struct device_context since an atomic bits array would just do the work.
In my RFC I did introduce such context because of all the other features (sync/lock and status reporting), not because of const-ifying devices.
So maybe we should really proceed step by steps:
- let's fix the device constant by having such atomic bits (and let device_get_binding() check these when relevant)
- let's add a struct device_context for everything else that would come afterwards (sync/lock & status reporting).
As you already mentioned in relevant PRs, you are not convinced about adding such sync/lock and status report features... so adding a device context right now would be a bad idea then.
There was a problem hiding this comment.
You're right, I was combining multiple features. I'll switch it to a bool init_ok since that's the only state we need right now.
tbursztyka
reviewed
May 12, 2020
| @@ -39,7 +39,7 @@ static bool device_get_config_level(const struct shell *shell, int level) | |||
| bool devices = false; | |||
|
|
|||
| for (dev = levels[level]; dev < levels[level+1]; dev++) { | |||
| if (dev->driver_api != NULL) { | |||
| if (dev->driver_context->init_ok) { | |||
There was a problem hiding this comment.
hm, if we stick to the goal of this RFC: const-ifying struct device instances, then such driver context is overkill.
Better having an array of atomic bits then, like PM's busy bit array. Less changes to reach the goal.
I looked at them and this told me it wasn't right: We can't go removing const from pointers. It's outlawed by MISRA 11.8:
|
Did not know that, I should probably download someday the misra papers. Anyway my point was that instead of chasing the same rabbit in parallel, let's use each-others works properly. At least, I am not going to waste my time on a task if I know somebody else is doing the same work. Unless, you want to take over this task (though it makes already my work a bit of a waste then) |
pabigot
force-pushed
the
nordic/issue/22941a
branch
from
a0bec95 to
89b49de
Compare
|
I'm not interested in taking over your wider work on the device structure, but I am interested in pursuing issues related to making I expect to have my results in the next day or two after a few more passes through shippable to identify those issues, then we can assess how to proceed. This effort has already identified existing MISRA violations in current master that should probably be addressed before 2.3 release (#25247). |
pabigot
force-pushed
the
nordic/issue/22941a
branch
3 times, most recently
from
a35dc88 to
d249eb6
Compare
pabigot
force-pushed
the
nordic/issue/22941a
branch
6 times, most recently
from
42a6497 to
99d6ac3
Compare
|
Dev-Review (May 14):
|
This was referenced May 14, 2020
pabigot
force-pushed
the
nordic/issue/22941a
branch
2 times, most recently
from
e0c1074 to
00772d8
Compare
Let's set the api at built time, or this will create a bug once device instance pointers become constant. Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
Replace individual device instance definitions with the macro that
expands to the equivalent change.
git grep -l 'struct device DEVICE_NAME_GET' \
| xargs sed -i -r \
-e 's@static struct device DEVICE_NAME_GET\(([^)]*)\);@DEVICE_DECLARE(\1);@'
Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
This is done by the init infrastructure and should not be implemented in the driver itself. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
The k_object API associates mutable state structures with known kernel objects to support userspace. The kernel objects themselves are not modified by the API, and in some cases (e.g. device structures) may be const-qualified. Update the API so that pointers to these const kernel objects can be passed without casting away the const qualifier. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Avoid duplication of the encoding of the public device identifier by using the constructing macro in definitions. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
At this time the only thing preventing moving `struct device` objects into flash is the use of the API pointer as a flag indicating initialization success. Add a structure to hold dynamic state common to all devices. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
These should now have no mutable content. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Rebases should pause immediately before this commit and run the steps
described below, which should be committed with `-C SHA` where SHA is
is the commit SHA of this commit. This may reduce the amount of
conflict arising from upstream changes.
1: coccinelle: ensure all code references point to const struct device
/* Force struct device to be const in all pointer declarations
* Invoke:
spatch --sp-file devptr.cocci --dir . --include-headers --very-quiet \
--macro-file scripts/coccinelle/macros.h
*/
@
disable optional_qualifier
@
@@
+const
struct device *
@
disable optional_qualifier
@
@@
+const
struct device * const
2: sed: ensure all non-code/macro references point to const struct device
git grep -l '(struct device' \
| xargs sed -i -r \
-e 's@\(struct device@(const struct device@g'
Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Now that struct device instances are immutable store the flag that indicates whether initialization was successful in the device context. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
This macro is generally used in contexts where something is expecting a pointer to (non-const) void. Reduce the complaints by lying about the type. This needs to be fixed properly by changing all API where an application of this macro is passed as a parameter; i.e. specifically API related to ISRs. Here we're just testing build and behavior. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Various generic data pointers are typed as pointer-to-void but some uses pass device pointers. Add const-removing casts to avoid the build errors. Some of these were identified from shippable logs, e.g.: grep discarded-qualif /tmp/zephyrproject-rtos-zephyr-72591.1.log \ | sed -e 's@^.*/@@' \ | sort \ | uniq Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
External modules may include code with prototypes that must be updated to accept const device objects. Update to the version of those modules that is ready for the new expectations. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
pabigot
force-pushed
the
nordic/issue/22941a
branch
from
00772d8 to
ddf90bc
Compare
|
Latest run passes all tests that ran before timeout, so we'll leave it here. Some of the commits should be submitted for master regardless of whether we continue with making definitions const. https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/72918/summary/console |
This was referenced May 26, 2020
This was referenced Jun 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has an alternative prototype of the part of #24873 that allows struct device instances to be put into flash. The intent of this draft PR is to examine the impact of declaring device structures with a
constqualifier.A simple representation of generic mutable driver state ("context") was selected and named to enable evaluation of the impact of making device structures immutable. The final approach to associating context with drivers will be determined in other contexts including #24873.
Initial results reveal the significance of the change:
const struct device*wherestruct device*is currently passed. This can be done relatively easily using a Coccinelle script for code, and a sed script for documentation and macros where Coccinelle can't detect the pattern.const device *pointers to functions withvoid *parameters is a much bigger issue; at this time the situations can only be detected by build failures. Many are currently being masked in this draft by a commit that removes theconstqualifier from the device pointer produced byDEVICE_GET(). Device pointers are sometimes passed:void *arguments)log_output_ctx_set()SYS_INIT()used in the Segger RTT capability.