Attention is currently required from: Andrey Pronin, Christian Walter, Eric Lai, Erik van den Bogaert, Felix Held, Frans Hendriks, Fred Reitberger, Jason Glenesk, Jon Murphy, Julius Werner, Karthik Ramasubramanian, Matt DeVillier, Nick Vaccaro, Subrata Banik.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77666?usp=email )
Change subject: treewide: convert to tpm_result_t ......................................................................
Patch Set 19:
(3 comments)
Patchset:
PS19:
If the goal here is to make the return codes more spec-compliant we can't really do that. […]
Following what the kernel does (e.g., `return -EINVAL`) allows us to use the defined spec values, while making them negative prevents breaking existing checks. In my opinion, this keeps the best of both worlds:
* The error codes are searchable, both in code and the spec. * The rest of the code can continue without needing to be updated, and the error values can be returned as far up the stack as we want with minimal worry that errors will be lost.
Note that using positive values for errors goes against Coreboot's coding style (emphasis added):
https://doc.coreboot.org/contributing/coding_style.html?highlight=style#func...
### Function return values and names Functions can return values of many different kinds, and one of the most common is a value indicating whether the function succeeded or failed. Such a value can be represented as an error-code integer (`CB_ERR_xxx` (**negative number**) = failure, `CB_SUCCESS` (0) = success) or a “succeeded” boolean (0 = failure, non-zero = success).
We can pursue that approach anyway, as long we can convince ourselves we've updated all of the necessary code paths (which is where I'm concerned we'll miss something).
This is what I see just in `src/security`, some of which may have already been updated in these CLs, and others likely don't apply (like `entry_idx < 0` checks).
``` timvp@timvp-p620-9115096:~/code/coreboot/src/security$ rg if | ag "< 0" | wc -l 18 ```
That's a small enough number to get them all updated. However, looking at the entire code base, I don't think that's feasible:
``` timvp@timvp-p620-9115096:~/code/coreboot/src$ rg if | ag "< 0" | wc -l 2107 ```
We obviously don't need to update everything, but it may take some very careful code inspection to know exactly where we do need to be mindful. Otherwise, an error will be interpreted as "good", and the device will continue along until the failed command (hopefully) triggers some other side effect that stops things. Worst-case, we don't hit an error until much later, possibly after we've made it to the OS.
This also seems to show how much we're fighting against Coreboot's coding convention of using negative values to signal errors.
Is there a good way to determine when to make the switch from positive to negative values for errors when passing things up the stack?
File src/drivers/crb/tpm.c:
https://review.coreboot.org/c/coreboot/+/77666/comment/952c384c_98c6ad78 : PS19, Line 168: Error(%d) nit: Space before the "(": `Error (%d)`
File src/drivers/i2c/tpm/tpm.c:
https://review.coreboot.org/c/coreboot/+/77666/comment/1122f6ce_eefa23f3 : PS19, Line 86: static int iic_tpm_read(uint8_t addr, uint8_t *buffer, size_t len) Why aren't these functions updated also?
-
Tim Van Patten (Code Review)