Re: [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu: nova-core: u64::checked_sub() fails on negative offset in timer_time()
--

commit 8e6e7110b2947fca24843940e52d712c197357df
Author: Dave Airlie <airlied@redhat.com>

gpu: nova-core: add support for GPU timer block. (v4)

This commit adds GPU timer block initialization support for nova-core across
Turing/Ampere/Ada (tu102 HAL) and Hopper/Blackwell (gh100/gb100 HALs) to
compute the system timer offset passed to GSP initialization.

> diff --git a/drivers/gpu/nova-core/timer/hal/gh100.rs b/drivers/gpu/nova-core/timer/hal/gh100.rs
> new file mode 100644
> index 0000000000000..0486df5889647
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,79 @@

[ ... ]

> +    fn timer_time(&self, bar: Bar0<'_>, time_ns: u64) -> Result<u64> {
> +        let mut sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        let sec_timer_ns: u64;
> +        loop {
> +            let sec_lo = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0));
> +            let sec_hi = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +            if sec_hi == sec_hi2 {
> +                sec_timer_ns = u64::from(sec_hi) << 32 | u64::from(sec_lo);
> +                break;
> +            }
> +            sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        }
> +
> +        // The system and the GPU timer are both free-running with unrelated epochs, so the
> +        // difference can be negative. Both operands are < 2^62, hence this cannot overflow.
> +        let sys_timer_offset_ns = time_ns.checked_sub(sec_timer_ns).ok_or(EINVAL)?;

[Severity: High]
In Gh100::timer_time(), both time_ns and sec_timer_ns are u64 values. When
the difference is negative (time_ns < sec_timer_ns), u64::checked_sub()
returns None, causing Gh100::timer_time() to return Err(EINVAL) and abort
Gpu::new() -> Timer::init() -> Gh100::init() during probe.

Since the comment notes that the difference can be negative and both operands
are less than 2^62, should this use wrapping_sub() (or signed subtraction)
instead of checked_sub()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-nova-timer-v4-0-45a7c121d4de@redhat.com?part=2