Re: [PATCH 4/7] irqchip/irq-qcom-mpm: Program wakeup timer when CPU cluster goes to LPM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Data race between the AP and RPM processor due to incorrect ordering of mailbox notification and timer programming.
- [High] Unhandled KTIME_MAX from unscheduled timers causes massive integer overflows and random hardware wakeups.
- [High] Expired timers produce negative relative times that underflow when cast to unsigned u64.
- [High] Invalid USECS_TO_CYCLES conversion macro uses CPU delay loop calibration instead of arch timer frequency, and truncates to 32 bits.
--

--- Patch [4]: [PATCH 4/7] irqchip/irq-qcom-mpm: Program wakeup timer when CPU cluster goes to LPM ---
commit c68a3cdf8bf38bac4bbcdaf3ea878c066a1246a0
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

irqchip/irq-qcom-mpm: Program wakeup timer when CPU cluster goes to LPM

This commit adds support for programming the next wakeup time into the MPM
timer when the CPU cluster enters power collapse. This allows the RPM processor
to set the hardware timer and wake up the SoC when the architectural timer
interrupt cannot do so during deep low power states.

> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> index 763eddee99dc4..f43c4a1c35f78 100644
> --- a/drivers/irqchip/irq-qcom-mpm.c
> +++ b/drivers/irqchip/irq-qcom-mpm.c

[ ... ]

> @@ -77,6 +80,13 @@ enum qcom_mpm_reg {
>  	MPM_REG_STATUS,
>  };
>  
> +#define USECS_TO_CYCLES(time_usecs)	xloops_to_cycles((time_usecs) * 0x10C7UL)
> +
> +static inline unsigned long xloops_to_cycles(u64 xloops)
> +{
> +	return (xloops * loops_per_jiffy * HZ) >> 32;
> +}

[Severity: High]
Does this macro use the correct frequency for the conversion, and is there a
risk of overflow?

In xloops_to_cycles(), loops_per_jiffy represents the CPU-specific delay
loop iterations rather than the fixed frequency of the architectural timer.

Also, the calculation (xloops * loops_per_jiffy * HZ) can overflow a 64-bit
integer for sleep times exceeding 25 seconds. Finally, returning an unsigned
long truncates the result to 32 bits on 32-bit ARM systems, which would
further cap the maximum sleep duration.

[ ... ]

> @@ -320,6 +331,36 @@ static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
>  	return ret;
>  }
>  
> +static void mpm_write_next_wakeup(struct qcom_mpm_priv *priv)
> +{
> +	ktime_t now, wakeup = KTIME_MAX;
> +	u64 wakeup_us, wakeup_cycles = ~0;
> +	u32 lo, hi;
> +
> +	/* Set highest time when system (timekeeping) is suspended */
> +	if (system_state == SYSTEM_SUSPEND)
> +		goto exit;
> +
> +	/* Find the relative wakeup in kernel time scale */
> +	wakeup = dev_pm_genpd_get_next_hrtimer(priv->dev);
> +
> +	/* Find the relative wakeup in kernel time scale */
> +	now = ktime_get();
> +	wakeup = ktime_sub(wakeup, now);

[Severity: High]
Could this overflow if there are no pending timers?

If dev_pm_genpd_get_next_hrtimer() returns KTIME_MAX, subtracting now from
KTIME_MAX in mpm_write_next_wakeup() yields a massive delta.

When this large delta is converted to microseconds and passed to
USECS_TO_CYCLES(), it seems it would overflow the 64-bit bounds during the
multiplication, leading to a randomized or incorrect wakeup time being
programmed into the hardware.

> +	wakeup_us = ktime_to_us(wakeup);

[Severity: High]
How does this handle expired timers where the scheduled wakeup is in the past?

If the scheduled wakeup is earlier than now, ktime_sub() will return a negative
time delta. When ktime_to_us() returns this negative value, assigning it to
the unsigned u64 wakeup_us variable will cause it to wrap around to a massive
positive number, which then gets passed to the conversion macros.

> +
> +	/* Convert the wakeup to arch timer scale */
> +	wakeup_cycles = USECS_TO_CYCLES(wakeup_us);
> +	wakeup_cycles += arch_timer_read_counter();
> +
> +exit:
> +	lo = wakeup_cycles;
> +	hi = wakeup_cycles >> 32;
> +
> +	qcom_mpm_write(priv, MPM_REG_TIMER, 0, lo);
> +	qcom_mpm_write(priv, MPM_REG_TIMER, 1, hi);
> +}
> +
>  static int handle_rpm_notification(struct qcom_mpm_priv *priv)
>  {
>  	int i, ret;
> @@ -332,6 +373,7 @@ static int handle_rpm_notification(struct qcom_mpm_priv *priv)
>  	if (ret < 0)
>  		return ret;
>  
> +	mpm_write_next_wakeup(priv);

[Severity: High]
Is there a data race here between notifying the RPM and writing the timer?

In handle_rpm_notification(), the mailbox message is sent to the RPM
via mbox_send_message() before mpm_write_next_wakeup() is called to program
the new timer values into the shared registers. 

Because the notification is sent before the write occurs, the RPM could read
the shared registers before the AP has actually written the updated wakeup
time, causing it to use stale or uninitialized values.

>  	mbox_client_txdone(priv->mbox_chan, 0);
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-b4-shikra_lpm_addition-v1-0-3d858df2cbbf@oss.qualcomm.com?part=4