Re: [PATCH 6/7] arm64: dts: qcom: agatti: Do not mark MPM as power domain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing check for KTIME_MAX and expired timers causes 64-bit integer overflow, resulting in an unpredictable hardware wakeup time.
- [High] Mailbox notification is sent to the RPM processor before the timer registers are updated, causing a race where RPM reads stale timer values.
--

--- Patch [6]: [PATCH 6/7] arm64: dts: qcom: agatti: Do not mark MPM as power domain ---
commit 7c3965f0765f0a48723f4b17bac82d687c8614cc
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

arm64: dts: qcom: agatti: Do not mark MPM as power domain
    
Do not mark MPM device as power domain since it leads to idle-states init
failure because of probe dependencies. CPU cluster power domain node is kept
disabled and hence CPU cluster will never power collapse. Do not register
MPM under it in this case.

> diff --git a/arch/arm64/boot/dts/qcom/agatti.dtsi b/arch/arm64/boot/dts/qcom/agatti.dtsi
> index f0b6ae9b81528..ddb038b3bbbc4 100644
> --- a/arch/arm64/boot/dts/qcom/agatti.dtsi
> +++ b/arch/arm64/boot/dts/qcom/agatti.dtsi
> @@ -205,7 +205,6 @@ cpu_pd3: power-domain-cpu3 {
>  
>  		cluster_pd: power-domain-cpu-cluster {
>  			#power-domain-cells = <0>;
> -			power-domains = <&mpm>;
>  			domain-idle-states = <&cluster_sleep>;
>  		};
>  	};

[Severity: High]
Does removing the MPM power domain mapping cause issues with the timer
values?

When no hrtimers are pending, dev_pm_genpd_get_next_hrtimer() returns
KTIME_MAX. Subtracting 'now' leaves a huge positive value, which when
converted to microseconds and multiplied by 0x10C7UL overflows the 64-bit
boundary.

Similarly, if a timer has already expired, the negative difference
becomes a huge positive value when implicitly cast to u64 wakeup_us, leading
to overflow.

Could this happen here in mpm_write_next_wakeup() within
drivers/irqchip/irq-qcom-mpm.c:

	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);
	wakeup_us = ktime_to_us(wakeup);

	/* Convert the wakeup to arch timer scale */
	wakeup_cycles = USECS_TO_CYCLES(wakeup_us);

and cause random timer values to be programmed into the MPM hardware?

[ ... ]

> @@ -281,7 +280,6 @@ mpm: interrupt-controller {
>  			mboxes = <&apcs_glb 1>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> -			#power-domain-cells = <0>;
>  			interrupt-parent = <&intc>;
>  			qcom,mpm-pin-count = <96>;
>  			qcom,mpm-pin-map = <2 275>,  /* TSENS0 uplow */

[Severity: High]
Does this change expose a race condition with the RPM processor?

The AP signals the RPM processor to configure hardware state from the shared
vMPM SRAM before actually writing the new timer wakeup values to the SRAM.
Since the RPM is an independent processor, it is highly likely to read the
old, stale timer values.

Could this happen here in handle_rpm_notification() within
drivers/irqchip/irq-qcom-mpm.c:

	/* Notify RPM to write vMPM into HW */
	ret = mbox_send_message(priv->mbox_chan, NULL);
	if (ret < 0)
		return ret;

	mpm_write_next_wakeup(priv);

and cause the hardware to be programmed with the incorrect wakeup time?

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