stm32xx-i2c: remove I2cControl function table #2121
Conversation
This file contains hidden or 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
mkeeter
approved these changes
Jun 30, 2025
It turns out that, in practice, we only ever provide a single function for each of `wfi` and `enable` here. This makes the table itself an unnecessary level of indirection, which has some deleterious effects: - It opens the possibility for _different_ I2cControl structs containing _different_ function pointers to be passed to each call, complicating analysis. (Searching for this phenomenon was what led me to notice that we only ever use one implementation.) - It makes it a lot harder to recognize use of syscalls (or misuse of syscalls) in the I2C layer. - It routes important parts of the I2C stack through indirect function calls, defeating stack analysis. - It adds a bunch of code that isn't really necessary, as it turns out. This commit removes the dispatch table, inlining the existing functions into their callsites (and introducing a utility function for the wfi behavior).
mkeeter
reviewed
Jul 1, 2025
| /// This function, like `userlib::hl`, assumes that notification bit 31 is safe | ||
| /// to use for the timer. If `notification` also has bit 31 set, weird things | ||
| /// will happen, don't do that. | ||
| fn wfi_raw(event_mask: u32, timeout: I2cTimeout) -> I2cControlResult { |
There was a problem hiding this comment.
The docstring is now outdated (refers to notification rather than event_mask)
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.
It turns out that, in practice, we only ever provide a single function for each of
wfiandenablehere. This makes the table itself an unnecessary level of indirection, which has some deleterious effects:It opens the possibility for different I2cControl structs containing different function pointers to be passed to each call, complicating analysis. (Searching for this phenomenon was what led me to notice that we only ever use one implementation.)
It makes it a lot harder to recognize use of syscalls (or misuse of syscalls) in the I2C layer.
It routes important parts of the I2C stack through indirect function calls, defeating stack analysis.
It adds a bunch of code that isn't really necessary, as it turns out.
This commit removes the dispatch table, inlining the existing functions into their callsites (and introducing a utility function for the wfi behavior).