Re: [PATCH v13 03/15] arm64/delay: move some constants out to a separate header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer overflow in `USECS_TO_CYCLES()` and `RPMH_USECS_TO_CYCLES()` causes incorrect timer cycle calculations for durations in the seconds range.
--

commit 0184d882315b35a4fce3d21dea0ae947ba4b1d29
Author: Ankur Arora <ankur.a.arora@oracle.com>

arm64/delay: move some constants out to a separate header
    
Moves some constants and functions related to xloops, cycles computation
out to a new header. Also make __delay_cycles() available outside of
arch/arm64/lib/delay.c. Rename some macros in qcom/rpmh-rsc.c which were
occupying the same namespace. No functional change.

> diff --git a/arch/arm64/include/asm/delay-const.h b/arch/arm64/include/asm/delay-const.h
> new file mode 100644
> index 0000000000000..2a5acfb7bff17
> --- /dev/null
> +++ b/arch/arm64/include/asm/delay-const.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

[ ... ]

> +static inline unsigned long xloops_to_cycles(unsigned long xloops)
> +{
> +	return (xloops * loops_per_jiffy * HZ) >> 32;
> +}
> +
> +#define USECS_TO_CYCLES(time_usecs) \
> +	xloops_to_cycles((time_usecs) * __usecs_to_xloops_mult)
> +

[ ... ]

> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index c6f7d5c9c493d..ad5ec5c0de0a3 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -146,10 +146,10 @@ enum {
>   *  +---------------------------------------------------+
>   */
>  
> -#define USECS_TO_CYCLES(time_usecs)			\
> -	xloops_to_cycles((time_usecs) * 0x10C7UL)
> +#define RPMH_USECS_TO_CYCLES(time_usecs)		\
> +	rpmh_xloops_to_cycles((time_usecs) * 0x10C7UL)
>  
> -static inline unsigned long xloops_to_cycles(u64 xloops)
> +static inline unsigned long rpmh_xloops_to_cycles(u64 xloops)
>  {
>  	return (xloops * loops_per_jiffy * HZ) >> 32;
>  }

[Severity: High]
This is a pre-existing issue, but does this calculation silently overflow for
durations exceeding a few minutes?

In rpmh_rsc_write_next_wakeup(), the wakeup_us value is derived from the
next hrtimer. During system suspend, the next timer can be hours away, or
even KTIME_MAX if no timers are pending.

The RPMH_USECS_TO_CYCLES() macro multiplies wakeup_us by 0x10C7UL (4295),
and then rpmh_xloops_to_cycles() multiplies that further by
loops_per_jiffy * HZ. 

On arm64, loops_per_jiffy * HZ represents the arch timer frequency (e.g.,
19.2 MHz on QCOM). This means the intermediate value grows by roughly
4295 * 19,200,000 = ~8.24e10 per microsecond. A 64-bit unsigned integer
maxes out at 1.84e19, which means this multiplication wraps around when
wakeup_us exceeds approximately 224 seconds.

This overflow produces a severely truncated timer cycle count, which can
cause the device to wake up almost immediately from deep sleep instead of
sleeping for the requested duration, draining the battery.

Does promoting this flawed logic to a global header via the new generic
USECS_TO_CYCLES() macro in asm/delay-const.h also introduce an
architectural hazard for large timeouts?

> @@ -819,7 +819,7 @@ void rpmh_rsc_write_next_wakeup(struct rsc_drv *drv)
>  	wakeup_us = ktime_to_us(wakeup);
>  
>  	/* Convert the wakeup to arch timer scale */
> -	wakeup_cycles = USECS_TO_CYCLES(wakeup_us);
> +	wakeup_cycles = RPMH_USECS_TO_CYCLES(wakeup_us);
>  	wakeup_cycles += arch_timer_read_counter();
>  
>  exit:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702013334.140905-1-ankur.a.arora@oracle.com?part=3