Re: [PATCH v15 03/16] arm64/delay: move, fixup usecs_to_cycles()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch fixes a 64-bit integer overflow bug in arm64 delay calculations while deliberately leaving the exact same arithmetic bug unfixed in the duplicated code in `qcom/rpmh-rsc.c`, which causes hardware wakeup times to be incorrectly programmed for sleep durations exceeding ~224 seconds.
--

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

arm64/delay: move, fixup usecs_to_cycles()

This patch updates xloops_to_cycles() to use mul_u64_u64_shr() instead of
32-bit fixed point arithmetic to avoid truncation for delay values larger
than ~10s. It also moves cycles computation out to a new header, makes 
__delay_cycles() available outside delay.c, and renames some colliding 
namespaces in qcom/rpmh-rsc.c.

> 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 isn't a bug introduced by this patch, but while renaming this function 
and macro to avoid a namespace collision, does it make sense to also apply 
the 128-bit math fix (mul_u64_u64_shr) here? 

The 64-bit arithmetic in rpmh_xloops_to_cycles() still wraps around the 
maximum limit for large xloops values, which appears to be the exact same 
integer overflow bug this patch successfully resolves in arch/arm64/lib/delay.c.

[ ... ]
> @@ -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:

[Severity: High]
This is also a pre-existing issue, but this seems to be the path where the 
overflow described above causes problems for device power management.

During system suspend, rpmh_rsc_write_next_wakeup() queries the next 
hrtimer. If the timer is roughly more than 224 seconds away (or if no 
timer exists, returning KTIME_MAX), wakeup_us can exceed the 64-bit 
mathematical limit of the inline calculation.

Does this cause an overflow of the u64 intermediate product, resulting 
in a corrupted, wrapped wakeup time being programmed into the hardware TCS, 
and leading to premature wakeups from deep sleep?

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