Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Missing 64-bit cast leads to 32-bit multiplication overflow in numerator calculation. - [High] 32-bit overflow in `modulo_hz * 100` causes incorrect divisor and potential divide-by-zero. - [Medium] `ASSERT` logic is broken due to 32-bit integer overflow. --
commit 9cda336bf5ea701c3d123b38512793b3554f5c3b Author: Arnd Bergmann <[email protected]> drm/amd/display: avoid 64-bit division This commit avoids 64-bit division on 32-bit targets in DCE clock source calculations by replacing open-coded division with div_u64(). > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c > b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c > index 83dcfae6c3c0d..ed9a6733d3457 100644 > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c > @@ -1229,9 +1229,9 @@ static bool get_dp_dto_frequency_100hz( > */ > modulo_hz = REG_READ(MODULO[inst]); > if (modulo_hz) { > - temp = div_u64((uint64_t)clock_hz * > dp_dto_ref_khz * 10, modulo_hz); > - ASSERT(temp / 100 <= 0xFFFFFFFFUL); > - *pixel_clk_100hz = (unsigned int)(temp / 100); > + temp = clock_hz * dp_dto_ref_khz * 10; [Severity: High] Does this drop a required 64-bit cast, leading to a 32-bit multiplication overflow? Both clock_hz and dp_dto_ref_khz are 32-bit unsigned integers representing frequencies. dp_dto_ref_khz is typically around 600,000 (600 MHz in kHz), and clock_hz can also be extremely large. Their product easily exceeds the 32-bit maximum. Without the (uint64_t) cast that was present in the original code, the multiplication is performed entirely using 32-bit arithmetic. The result overflows, wraps around, and is then implicitly cast to the 64-bit temp variable, resulting in an incorrect numerator. > + ASSERT(temp <= INT_MAX * modulo_hz * 100); [Severity: Medium] Can this expression overflow 32-bit integer limits and break the assertion logic? The expression INT_MAX * modulo_hz * 100 is evaluated using 32-bit integer arithmetic. Since INT_MAX is already nearly half of the maximum 32-bit unsigned value, multiplying it by 100 and any typical modulo_hz frequency will overflow 32-bit limits multiple times, producing an arbitrary wrapped value. The 64-bit temp is then compared against this 32-bit value. > + *pixel_clk_100hz = div_u64(temp, modulo_hz * > 100); [Severity: High] Does modulo_hz * 100 overflow, resulting in an incorrect divisor or a potential divide-by-zero? modulo_hz is a 32-bit unsigned integer representing a clock frequency (often around 600 MHz). Multiplying it by 100 evaluates as a 32-bit operation, which exceeds UINT_MAX and wraps around. The div_u64() function expects a u32 divisor, so it uses this truncated, wrapped value instead of the intended divisor. If modulo_hz happens to be exactly 1073741824 (about 1.07 GHz), modulo_hz * 100 wraps to exactly 0, which would trigger a divide-by-zero kernel panic in div_u64(). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
