Attention is currently required from: Paul Menzel, Yidi Lin, Yu-Ping Wu.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78800?usp=email )
Change subject: arch/arm64/arch_timer: Fix possible overflow in multiplication ......................................................................
Patch Set 2:
(3 comments)
File src/arch/arm64/arch_timer.c:
https://review.coreboot.org/c/coreboot/+/78800/comment/eea52878_ae8cb645 : PS2, Line 16: assert(tfreq > 0) Missing semicolon (also, this is probably not necessary, you'll get a pretty obvious division-by-zero exception when this is not initialized anyway).
https://review.coreboot.org/c/coreboot/+/78800/comment/088f888b_eaa045be : PS2, Line 18: div = gcd(mult, tfreq); nit: depending on the GCD implementation you'll probably want to write it as `gcd(tfreq, mult)` to help it terminate a little faster (in the common case where tfreq is a multiple of mult).
https://review.coreboot.org/c/coreboot/+/78800/comment/107e6c51_75e29c0f : PS2, Line 11: static uint32_t tfreq, mult; : uint32_t div; : : if (tfreq == 0) { : tfreq = raw_read_cntfrq_el0(); : assert(tfreq > 0) : mult = 1000000; : div = gcd(mult, tfreq); : tfreq /= div; : mult /= div; : } : : long usecs = (tvalue * mult) / tfreq;
Nice, but why not just cast `tvalue`, and let the compiler figure out the rest? Is that possible?
Not really sure what you want to cast it to? It's already 64-bit. The concern here is that even the 64-bit multiplication could overflow since the initialization value of CNTPCT is unknown.