Attention is currently required from: Hung-Te Lin, Shelley Chen, Paul Menzel, Yu-Ping Wu, Jianjun Wang. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59738 )
Change subject: lib: Add fls() (Find Last Set) ......................................................................
Patch Set 4:
(2 comments)
File src/include/lib.h:
https://review.coreboot.org/c/coreboot/+/59738/comment/e97f0a76_f8e61469 PS4, Line 56: /* Find Last Set: fls(1) == 1, fls(0) == 0, fls(1 << 31) == 32 */ Hmmm... this has the same problem that we originally had with ffs(). The established BSD versions of these functions count bits from 1, which is just intuitively wrong if you ask me. Everyone else counts bits from 0.
The Linux kernel has both variants with ffs()/fls() counting from 1 and __ffs()/__fls() counting from 0. We chose to only provide the intuitive one here, and follow Linux' naming convention to avoid misunderstandings... that's why we only have __ffs() and not ffs(), and that's why I think you should also only add an __fls() here which is calculated as (32 - clz(x) - 1) (and rewrite the storage code needing this to work appropriately).
Which then brings me to the thing I should have realized to begin with: at that point, __fls() is just the same as log2(). So you can really just use log2() in that code (and that's how we wrote all other code that needs something like this before, that's why nobody ever saw the need to add this here). But I'm also okay with adding __fls() as an alias for log2() if you think that makes things easier to read.
https://review.coreboot.org/c/coreboot/+/59738/comment/2659442b_64065031 PS4, Line 66: If you do add anything new here, please also add a 64-bit variant down here and add the equivalents to libpayload.h, for consistency.
-
Julius Werner (Code Review)