Attention is currently required from: Aaron Durbin, Martin Roth, Furquan Shaikh, Marshall Dawson, Julius Werner, Eric Peers. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56048 )
Change subject: lib/future: Implement Futures API ......................................................................
Patch Set 3:
(4 comments)
File src/lib/future.c:
https://review.coreboot.org/c/coreboot/+/56048/comment/39586af5_d46c6968 PS2, Line 14: static struct future *futures[8];
you never init this to NULL but count on it being null. […]
BSS section is guaranteed to be 0 before c main() starts. So no need to explicitly set it.
https://review.coreboot.org/c/coreboot/+/56048/comment/2fca3839_ccd2cf53 PS2, Line 34: die
Maybe assert? This shouldn't be a die in my opinion - it should be the same as not having the confi […]
Good point.
https://review.coreboot.org/c/coreboot/+/56048/comment/e26aa4ae_89a297f7 PS2, Line 54: die
Similar to the last die - Maybe assert and return?
This is where I dislike the assert API... If fatal asserts are disabled, then we continue and bad things will happen. So we need to add a conditional to check the assert again afterwards.
https://review.coreboot.org/c/coreboot/+/56048/comment/989aa6ac_8dce1562 PS2, Line 68: state = futures[i]->poll(futures[i], false);
For debug, maybe add an optional print before and after and include the amount of time it took?
So I was tempted to add a bunch of debug style information in the the futures object. i.e., start time, total poll time, end time, etc. I didn't since I wanted to keep the API and object model very simple.
I didn't add any prints here since it will get called in a tight loop by `wait_for_future`, so it could potentially produce a lot of output. Do you still think I should do it?