Attention is currently required from: Nico Huber, Angel Pons, Arthur Heymans, Kyösti Mälkki. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55436 )
Change subject: device: Add fixed_mem_resource64() ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
One reason to have the API take values in KiB could be to avoid having to deal with 64-bit values, which could unnecessarily bloat the code.
Right, but is that actually how it's being used here? I mean, are the changes in CB:55428 all exactly those where we know using 64-bit values is no concern, while it is on other instances that don't get changed? Because I don't see the pattern.
I'm not a fan of just arbitrarily allowing multiple ways to do the same thing just because there may be some odd fringe reason that one is better than the other, if there is no reasonable expectation that most people who are using this stuff would really know about and explicitly make a decision regarding that trade-off. I think consistency is the foundation of maintainability in all things and we should always strive to minimize situations where different people would write the same thing in different ways just because they have different personal preferences. In cases where you really *need* a different form of an API for something that's actually performance-relevant, we should try to make it so there's one "default" form that most cases use and specialized forms that only the cases which really need them use, with well-documented criteria for when they should be picked. In this case, I think the performance difference of using a 64-bit argument here is probably too negligible to care about, and I personally really don't care whether we stick with the kilobytes or not, but let's please only have one way to do it.
Still, I believe that changing this API in a single commit is just unreviewable, and I'd much prefer to have these two APIs coexist for a while.
I mean... if it's a coccinelle patch I'd generally just review the spatch and trust the author that they applied it correctly. I understand it's a big change, but I also don't think waiting is going to make anything better about that, and a situation where we permanently carry an old and a new way of doing things around would be undesirable (especially because in practice, you'll have a really hard time getting the new way to catch on if you don't remove and replace all instances of the old one... most times people just keep coping this stuff from the respective previous generation board/SoC over and over).
-
Julius Werner (Code Review)