[PATCH v3 0/2] Add fallback method to handle RPM notification in MPM driver

[PATCH v3 0/2] Add fallback method to handle RPM notification in MPM driver

[Sneh Mankad]

MPM irqchip needs to notify RPM (Resource Power Manager) processor to read
the latest wake up capable interrupts when the CPU cluster is entering the
deepest idle state. This is done by sending IPC interrupt to RPM and is
implemented as .power_off() callback by registering MPM as parent power
domain to CPU cluster.

Such implementation earlier introduced a hard probe dependency between MPM
irqchip and CPU cluster power domains. That is MPM irqchip needs to finish
probe before PSCI power domains are probed.

The above problem has since been resolved after bd7f488a8e5d ("cpuidle:
psci: Fix support for probe deferral by dropping the faux device") / [1].
However, it makes sense to remodel the RPM notification in MPM irqchip due
to general consistency.

MPM is not really a power domain provider, all it needs is a notification
when CPU cluster is going to power collapse to carry out the RPM
notification. We can register for CPU_PM notifications for that purpose.
This way we can avoid such unrelated dependencies.

We cannot register MPM under CPU cluster power domain and rely on
GENPD notifications here, since that way cluster low power modes will be
blocked whenever some consumer of MPM, and in turn MPM itself, are
runtime active. This will add another unnecessary dependency for CPU
cluster low power modes.

For now as first step, register with cpu_pm as a fallback to manage RPM
notification. Trigger the RPM notification when the last CPU in CPU cluster
goes to power collapse.

Add idle states for CPUs and CPU cluster for Shikra.

[1] https://lore.kernel.org/all/20260909134240.685115374@linuxfoundation.org

Signed-off-by: Sneh Mankad <sneh.mankad@oss.qualcomm.com>
---
Changes in v3:
- Skip timer programming for now till we add support to register MPM
  under CPU cluster power domain without blocking its idle states.
- Skip modifying DTs other than Shikra to prioritize Shikra LPM
  enablement.
- Move genPD/cpu_pm registration after mbox_request_channel().
- Link to v2: https://lore.kernel.org/r/20261015-b4-shikra_lpm_addition-v2-0-ac7f1a0d5638@oss.qualcomm.com

Changes in v2:
- Enable Shikra LPMs in the first patch, following patches concern
  MPM driver.
- Register with cpu_pm as a fallback to manage RPM notification.
- Move USECS_TO_CYCLES and xloops_to_cycles to common header delay.h.
- Reformat qcom_mpm_offset() as per feedback.
- Avoid deleting cluster_pd node in qrb2210-rb1.dts.
- Link to v1: https://lore.kernel.org/r/20260713-b4-shikra_lpm_addition-v1-0-3d858df2cbbf@oss.qualcomm.com

---
Sneh Mankad (2):
      irqchip: irq-qcom-mpm: Register MPM with cpu_pm for RPM notification
      arm64: dts: qcom: shikra: Add CPU idle states

 arch/arm64/boot/dts/qcom/shikra.dtsi |  93 ++++++++++++++++++++++++++++++-
 drivers/irqchip/irq-qcom-mpm.c       | 104 ++++++++++++++++++++++++++---------
 2 files changed, 170 insertions(+), 27 deletions(-)
---
base-commit: 7777cc195ca1301a28008ca5cdb98bdb2a9d0def
change-id: 20260617-b4-shikra_lpm_addition-4a9bc82bafee

Best regards,
-- 
Sneh Mankad <sneh.mankad@oss.qualcomm.com>

[PATCH v3 1/2] irqchip: irq-qcom-mpm: Register MPM with cpu_pm for RPM notification

[Sneh Mankad]

MPM irqchip needs to notify RPM (Resource Power Manager) processor to read
the latest wake up capable interrupts when the CPU cluster is entering the
deepest idle state. This is done by sending IPC interrupt to RPM and is
implemented as .power_off() callback by registering MPM as parent power
domain to CPU cluster.

Such implementation earlier introduced a hard probe dependency between MPM
irqchip and CPU cluster power domains. That is MPM irqchip needs to finish
probe before PSCI power domains are probed.

The above problem has since been resolved after bd7f488a8e5d ("cpuidle:
psci: Fix support for probe deferral by dropping the faux device") / [1].
However, it makes sense to remodel the RPM notification in MPM irqchip due
to general consistency.

MPM is not really a power domain provider, all it needs is a notification
when CPU cluster is going to power collapse to carry out the RPM
notification. We can register for CPU_PM notifications for that purpose.
This way we can avoid such unrelated dependencies.

We cannot register MPM under CPU cluster power domain and rely on
GENPD notifications here, since that way cluster low power modes will be
blocked whenever some consumer of MPM, and in turn MPM itself, are
runtime active. This will add another unnecessary dependency for CPU
cluster low power modes.

For now as first step, register with cpu_pm as a fallback to manage RPM
notification. Trigger the RPM notification when the last CPU in CPU cluster
goes to power collapse.

[1] https://lore.kernel.org/all/20260909134240.685115374@linuxfoundation.org

Signed-off-by: Sneh Mankad <sneh.mankad@oss.qualcomm.com>
---
 drivers/irqchip/irq-qcom-mpm.c | 104 ++++++++++++++++++++++++++++++-----------
 1 file changed, 78 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
index 181320528a47ac1bde6cf7d2d0a9f79499990092..136d126f0ea24bb10b5183d6ce47b630aa77b0cf 100644
--- a/drivers/irqchip/irq-qcom-mpm.c
+++ b/drivers/irqchip/irq-qcom-mpm.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2010-2020, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/cpu_pm.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -85,6 +86,8 @@ struct qcom_mpm_priv {
 	unsigned int reg_stride;
 	struct irq_domain *domain;
 	struct generic_pm_domain genpd;
+	struct notifier_block mpm_pm;
+	atomic_t cpus_in_pm;
 };
 
 static u32 qcom_mpm_read(struct qcom_mpm_priv *priv, unsigned int reg,
@@ -292,10 +295,8 @@ static irqreturn_t qcom_mpm_handler(int irq, void *dev_id)
 	return ret;
 }
 
-static int mpm_pd_power_off(struct generic_pm_domain *genpd)
+static int handle_rpm_notification(struct qcom_mpm_priv *priv)
 {
-	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
-						  genpd);
 	int i, ret;
 
 	for (i = 0; i < priv->reg_stride; i++)
@@ -307,10 +308,52 @@ static int mpm_pd_power_off(struct generic_pm_domain *genpd)
 		return ret;
 
 	mbox_client_txdone(priv->mbox_chan, 0);
-
 	return 0;
 }
 
+static int mpm_pd_power_off(struct generic_pm_domain *genpd)
+{
+	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
+						  genpd);
+
+	return handle_rpm_notification(priv);
+}
+
+static int mpm_cpu_pm_callback(struct notifier_block *nfb, unsigned long action, void *v)
+{
+	struct qcom_mpm_priv *priv = container_of(nfb, struct qcom_mpm_priv, mpm_pm);
+	int cpus_in_pm;
+
+	switch (action) {
+	case CPU_PM_ENTER:
+		cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
+		/*
+		 * NOTE: comments for num_online_cpus() point out that it's
+		 * only a snapshot so we need to be careful. It should be OK
+		 * for us to use, though.  It's important for us not to miss
+		 * if we're the last CPU going down so it would only be a
+		 * problem if a CPU went offline right after we did the check
+		 * AND that CPU was not idle AND that CPU was the last non-idle
+		 * CPU. That can't happen. CPUs would have to come out of idle
+		 * before the CPU could go offline.
+		 */
+		if (cpus_in_pm < num_online_cpus())
+			return NOTIFY_OK;
+		break;
+	case CPU_PM_ENTER_FAILED:
+	case CPU_PM_EXIT:
+		atomic_dec(&priv->cpus_in_pm);
+		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	if (handle_rpm_notification(priv))
+		return NOTIFY_BAD;
+
+	return NOTIFY_OK;
+}
+
 static bool gic_hwirq_is_mapped(struct mpm_gic_map *maps, int cnt, u32 hwirq)
 {
 	int i;
@@ -415,26 +458,6 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
 	if (irq < 0)
 		return irq;
 
-	genpd = &priv->genpd;
-	genpd->flags = GENPD_FLAG_IRQ_SAFE;
-	genpd->power_off = mpm_pd_power_off;
-
-	genpd->name = devm_kasprintf(dev, GFP_KERNEL, "%s", dev_name(dev));
-	if (!genpd->name)
-		return -ENOMEM;
-
-	ret = pm_genpd_init(genpd, NULL, false);
-	if (ret) {
-		dev_err(dev, "failed to init genpd: %d\n", ret);
-		return ret;
-	}
-
-	ret = of_genpd_add_provider_simple(np, genpd);
-	if (ret) {
-		dev_err(dev, "failed to add genpd provider: %d\n", ret);
-		goto remove_genpd;
-	}
-
 	priv->mbox_client.dev = dev;
 	priv->mbox_client.knows_txdone = true;
 	priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
@@ -444,6 +467,35 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
 		return ret;
 	}
 
+	if (of_find_property(np, "#power-domain-cells", NULL)) {
+		genpd = &priv->genpd;
+		genpd->flags = GENPD_FLAG_IRQ_SAFE;
+		genpd->power_off = mpm_pd_power_off;
+
+		genpd->name = devm_kasprintf(dev, GFP_KERNEL, "%s", dev_name(dev));
+		if (!genpd->name)
+			return -ENOMEM;
+
+		ret = pm_genpd_init(genpd, NULL, false);
+		if (ret) {
+			dev_err(dev, "failed to init genpd: %d\n", ret);
+			return ret;
+		}
+
+		ret = of_genpd_add_provider_simple(np, genpd);
+		if (ret) {
+			dev_err(dev, "failed to add genpd provider: %d\n", ret);
+			goto remove_genpd;
+		}
+	} else {
+		priv->mpm_pm.notifier_call = mpm_cpu_pm_callback;
+		ret = cpu_pm_register_notifier(&priv->mpm_pm);
+		if (ret) {
+			dev_err(dev, "failed to register for CPU_PM notifications: %d\n", ret);
+			return ret;
+		}
+	}
+
 	parent_domain = irq_find_host(parent);
 	if (!parent_domain) {
 		dev_err(dev, "failed to find MPM parent domain\n");
@@ -473,10 +525,10 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
 
 remove_domain:
 	irq_domain_remove(priv->domain);
-free_mbox:
-	mbox_free_channel(priv->mbox_chan);
 remove_genpd:
 	pm_genpd_remove(genpd);
+free_mbox:
+	mbox_free_channel(priv->mbox_chan);
 	return ret;
 }
 

-- 
2.34.1

[PATCH v3 2/2] arm64: dts: qcom: shikra: Add CPU idle states

[Sneh Mankad]

Add idle states for the CPUs as well as the whole cluster. This enables
deeper-than-WFI cpuidle.

Remove #power-domain-cells from MPM device so that the driver follows the
cpu_pm notifications path to handle RPM notification. This avoids other
probe dependencies that arise due to MPM being a power domain provider.

Signed-off-by: Sneh Mankad <sneh.mankad@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/shikra.dtsi | 93 +++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
index 4e5bc9e17c8ed8914733ac9fdc9b69128b40216e..74be82ae50d16c7dcc7bc7af4deb28794efd47e8 100644
--- a/arch/arm64/boot/dts/qcom/shikra.dtsi
+++ b/arch/arm64/boot/dts/qcom/shikra.dtsi
@@ -40,6 +40,8 @@ cpu0: cpu@0 {
 			compatible = "arm,cortex-a55";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			power-domains = <&cpu_pd0>;
+			power-domain-names = "psci";
 			next-level-cache = <&l3>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
@@ -50,6 +52,8 @@ cpu1: cpu@100 {
 			compatible = "arm,cortex-a55";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
+			power-domains = <&cpu_pd1>;
+			power-domain-names = "psci";
 			next-level-cache = <&l3>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
@@ -60,6 +64,8 @@ cpu2: cpu@200 {
 			compatible = "arm,cortex-a55";
 			reg = <0x0 0x200>;
 			enable-method = "psci";
+			power-domains = <&cpu_pd2>;
+			power-domain-names = "psci";
 			next-level-cache = <&l3>;
 			capacity-dmips-mhz = <1024>;
 			dynamic-power-coefficient = <100>;
@@ -70,6 +76,8 @@ cpu3: cpu@300 {
 			compatible = "arm,cortex-a78c";
 			reg = <0x0 0x300>;
 			enable-method = "psci";
+			power-domains = <&cpu_pd3>;
+			power-domain-names = "psci";
 			next-level-cache = <&l2_3>;
 			capacity-dmips-mhz = <1946>;
 			dynamic-power-coefficient = <489>;
@@ -105,6 +113,61 @@ core0 {
 			};
 		};
 
+		idle-states {
+			entry-method = "psci";
+
+			little_cpu_sleep_0: cpu-sleep-0-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-power-down";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <549>;
+				exit-latency-us = <901>;
+				min-residency-us = <1774>;
+				local-timer-stop;
+			};
+
+			little_cpu_sleep_1: cpu-sleep-0-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "little-pll-power-down";
+				arm,psci-suspend-param = <0x40000004>;
+				entry-latency-us = <702>;
+				exit-latency-us = <915>;
+				min-residency-us = <4001>;
+				local-timer-stop;
+			};
+
+			big_cpu_sleep_0: cpu-sleep-1-0 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-power-down";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <523>;
+				exit-latency-us = <1244>;
+				min-residency-us = <2207>;
+				local-timer-stop;
+			};
+
+			big_cpu_sleep_1: cpu-sleep-1-1 {
+				compatible = "arm,idle-state";
+				idle-state-name = "big-pll-power-down";
+				arm,psci-suspend-param = <0x40000004>;
+				entry-latency-us = <526>;
+				exit-latency-us = <1854>;
+				min-residency-us = <5555>;
+				local-timer-stop;
+			};
+		};
+
+		domain_idle_states: domain-idle-states {
+			cluster_sleep_apss_off: cluster-sleep-0 {
+				compatible = "domain-idle-state";
+				idle-state-name = "cluster-power-down";
+				arm,psci-suspend-param = <0x41000044>;
+				entry-latency-us = <2752>;
+				exit-latency-us = <3038>;
+				min-residency-us = <6118>;
+			};
+		};
+
 		l3: l3-cache {
 			compatible = "cache";
 			cache-level = <3>;
@@ -144,6 +207,35 @@ pmu-a78c {
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		cpu_pd0: power-domain-cpu0 {
+			#power-domain-cells = <0>;
+			power-domains = <&cluster_pd>;
+			domain-idle-states = <&little_cpu_sleep_0>, <&little_cpu_sleep_1>;
+		};
+
+		cpu_pd1: power-domain-cpu1 {
+			#power-domain-cells = <0>;
+			power-domains = <&cluster_pd>;
+			domain-idle-states = <&little_cpu_sleep_0>, <&little_cpu_sleep_1>;
+		};
+
+		cpu_pd2: power-domain-cpu2 {
+			#power-domain-cells = <0>;
+			power-domains = <&cluster_pd>;
+			domain-idle-states = <&little_cpu_sleep_0>, <&little_cpu_sleep_1>;
+		};
+
+		cpu_pd3: power-domain-cpu3 {
+			#power-domain-cells = <0>;
+			power-domains = <&cluster_pd>;
+			domain-idle-states = <&big_cpu_sleep_0>, <&big_cpu_sleep_1>;
+		};
+
+		cluster_pd: power-domain-cluster {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&cluster_sleep_apss_off>;
+		};
 	};
 
 	rpm: remoteproc {
@@ -217,7 +309,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 */

-- 
2.34.1

Re: [PATCH v3 2/2] arm64: dts: qcom: shikra: Add CPU idle states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] irqchip/qcom-mpm: uninitialized genpd and notifier UAF in qcom_mpm_probe()
- [High] arm64: dts: qcom: shikra: invalid lock nesting and premature MPM notification
- [Medium] irqchip/qcom-mpm: cpus_in_pm counter leak on error in mpm_cpu_pm_callback()
--

commit bc4ed1a6ee147014bad47b72745f44c888126bd9
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

arm64: dts: qcom: shikra: Add CPU idle states

This commit adds PSCI CPU and cluster idle states for Shikra and removes
#power-domain-cells from the MPM node so the driver uses CPU PM
notifications instead of acting as a power domain provider.

> diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
> index 92d22e3b6931c..aeb608fd7180a 100644
> --- a/arch/arm64/boot/dts/qcom/shikra.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra.dtsi

[ ... ]

> @@ -334,7 +426,6 @@ mpm: interrupt-controller {
>  			mboxes = <&apcs_glb 1>;
>  			interrupt-controller;
>  			#interrupt-cells = <2>;
> -			#power-domain-cells = <0>;

[Severity: High]
When #power-domain-cells is removed from the mpm node, qcom_mpm_probe() in
drivers/irqchip/irq-qcom-mpm.c takes the else branch and registers the CPU
PM notifier instead of initializing genpd:

	struct generic_pm_domain *genpd;
	...
	priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
	...
	if (of_find_property(np, "#power-domain-cells", NULL)) {
		genpd = &priv->genpd;
		...
	} else {
		priv->mpm_pm.notifier_call = mpm_cpu_pm_callback;
		ret = cpu_pm_register_notifier(&priv->mpm_pm);
		if (ret) {
			dev_err(dev, "failed to register for CPU_PM notifications: %d\n", ret);
			return ret;
		}
	}

If a subsequent step in qcom_mpm_probe() fails, the error cleanup path runs:

remove_domain:
	irq_domain_remove(priv->domain);
remove_genpd:
	pm_genpd_remove(genpd);
free_mbox:
	mbox_free_channel(priv->mbox_chan);
	return ret;

If devm_request_irq() fails, execution jumps to remove_domain and falls
through to remove_genpd, passing the uninitialized stack pointer genpd to
pm_genpd_remove().

Furthermore, if irq_find_host(), irq_domain_create_hierarchy(), or
devm_request_irq() fails after cpu_pm_register_notifier(&priv->mpm_pm)
succeeds, the error path never calls
cpu_pm_unregister_notifier(&priv->mpm_pm). When devres frees priv on probe
failure, &priv->mpm_pm remains linked in the global cpu_pm_notifier chain
and will be dereferenced after free on the next CPU idle entry. And if
cpu_pm_register_notifier() itself fails, returning directly leaks
priv->mbox_chan.

Could qcom_mpm_probe() initialize genpd to NULL, guard pm_genpd_remove(),
unregister the CPU PM notifier on error, and jump to free_mbox when
cpu_pm_register_notifier() fails before switching Shikra to this path?

[Severity: Medium]
Switching the mpm node to the mpm_cpu_pm_callback() path also exposes an
error handling issue in drivers/irqchip/irq-qcom-mpm.c when
handle_rpm_notification() fails:

static int mpm_cpu_pm_callback(struct notifier_block *nfb, unsigned long action, void *v)
{
	struct qcom_mpm_priv *priv = container_of(nfb, struct qcom_mpm_priv, mpm_pm);
	int cpus_in_pm;

	switch (action) {
	case CPU_PM_ENTER:
		cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
		...
		if (cpus_in_pm < num_online_cpus())
			return NOTIFY_OK;
		break;
	case CPU_PM_ENTER_FAILED:
	case CPU_PM_EXIT:
		atomic_dec(&priv->cpus_in_pm);
		return NOTIFY_OK;
	default:
		return NOTIFY_DONE;
	}

	if (handle_rpm_notification(priv))
		return NOTIFY_BAD;

	return NOTIFY_OK;
}

During cpu_pm_enter(), raw_notifier_call_chain_robust() invokes
notifier_call_chain_robust() in kernel/notifier.c:

	ret = notifier_call_chain(nl, val_up, v, -1, &nr);
	if (ret & NOTIFY_STOP_MASK)
		notifier_call_chain(nl, val_down, v, nr-1, NULL);

When a callback returns NOTIFY_BAD, CPU_PM_ENTER_FAILED is only sent to the
nr - 1 callbacks that preceded the failing one, and cpu_pm_enter() returns
an error so cpu_pm_exit() is not called.

If handle_rpm_notification(priv) fails (for example, if mbox_send_message()
returns -ENOBUFS), wouldn't returning NOTIFY_BAD without decrementing
priv->cpus_in_pm leave priv->cpus_in_pm permanently skewed by +1?

[Severity: High]
With #power-domain-cells removed from mpm while adding both per-CPU PSCI
idle states (little-power-down, little-pll-power-down, big-power-down,
big-pll-power-down) and a cluster idle state (cluster-power-down), the
call chain on CPU idle entry becomes:

cpu_pm_enter()
  -> cpu_pm_notify_robust()
       raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
       -> mpm_cpu_pm_callback()
            -> handle_rpm_notification()
                 -> mbox_send_message()
                      -> add_to_rbuf()
                           guard(spinlock_irqsave)(&chan->lock);

Because cpu_pm_notifier.lock is a raw_spinlock_t and chan->lock in
drivers/mailbox/mailbox.c is a non-raw spinlock_t, won't acquiring
chan->lock inside mpm_cpu_pm_callback() trigger an invalid wait context bug
under CONFIG_PROVE_RAW_LOCK_NESTING and sleep in atomic context from the
idle task on PREEMPT_RT?

In addition, cpuidle-psci calls cpu_pm_enter() for every non-WFI per-CPU
idle state (via psci_enter_idle_state()) and before
pm_runtime_put_sync_suspend() determines whether cluster_pd actually enters
cluster-power-down (in __psci_enter_domain_idle_state()). Won't
mpm_cpu_pm_callback() clear MPM_REG_STATUS and notify RPM whenever all
CPUs enter shallow per-CPU idle states, even when the cluster does not
power down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-b4-shikra_lpm_addition-v3-0-424abcdcf824@oss.qualcomm.com?part=2

Re: [PATCH v3 1/2] irqchip: irq-qcom-mpm: Register MPM with cpu_pm for RPM notification

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] irqchip/qcom-mpm: broken error handling and UAF in qcom_mpm_probe()
- [High] irqchip/qcom-mpm: cpus_in_pm not decremented on failure in mpm_cpu_pm_callback()
- [High] irqchip/qcom-mpm: invalid spinlock_t nesting in mpm_cpu_pm_callback()
- [High] irqchip: irq-qcom-mpm: cpus_in_pm underflow when probing with idle CPUs
--

commit 0b0c9281cd528ac3d6a6f2d4a69ee74beee11616
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

irqchip: irq-qcom-mpm: Register MPM with cpu_pm for RPM notification

This commit updates the Qualcomm MPM irqchip driver to register a CPU PM
notifier when the device tree node does not specify #power-domain-cells,
notifying the RPM processor when the last online CPU enters idle.

> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> index 8c575d7ce8a17..b6caea90e9f41 100644
> --- a/drivers/irqchip/irq-qcom-mpm.c
> +++ b/drivers/irqchip/irq-qcom-mpm.c

[ ... ]

> @@ -307,10 +308,52 @@ static int mpm_pd_power_off(struct generic_pm_domain *genpd)
>  		return ret;
>  
>  	mbox_client_txdone(priv->mbox_chan, 0);
> -
>  	return 0;
>  }
>  
> +static int mpm_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> +						  genpd);
> +
> +	return handle_rpm_notification(priv);
> +}
> +
> +static int mpm_cpu_pm_callback(struct notifier_block *nfb, unsigned long action, void *v)
> +{
> +	struct qcom_mpm_priv *priv = container_of(nfb, struct qcom_mpm_priv, mpm_pm);
> +	int cpus_in_pm;
> +
> +	switch (action) {
> +	case CPU_PM_ENTER:
> +		cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
> +		/*
> +		 * NOTE: comments for num_online_cpus() point out that it's
> +		 * only a snapshot so we need to be careful. It should be OK
> +		 * for us to use, though.  It's important for us not to miss
> +		 * if we're the last CPU going down so it would only be a
> +		 * problem if a CPU went offline right after we did the check
> +		 * AND that CPU was not idle AND that CPU was the last non-idle
> +		 * CPU. That can't happen. CPUs would have to come out of idle
> +		 * before the CPU could go offline.
> +		 */
> +		if (cpus_in_pm < num_online_cpus())
> +			return NOTIFY_OK;
> +		break;
> +	case CPU_PM_ENTER_FAILED:
> +	case CPU_PM_EXIT:
> +		atomic_dec(&priv->cpus_in_pm);
> +		return NOTIFY_OK;

[Severity: High]
Can priv->cpus_in_pm underflow if other CPUs are already in idle when
qcom_mpm_probe() registers this notifier?

Because MPM is no longer a power domain provider for the PSCI cluster domain,
psci_cpuidle_domain_probe() and psci_cpuidle_probe() in
drivers/cpuidle/cpuidle-psci.c can initialize cpuidle before qcom_mpm_probe()
runs (for example, when CONFIG_QCOM_MPM, CONFIG_QCOM_SMD_RPM, or
CONFIG_QCOM_APCS_IPC is built as a module or defers probe).

If one or more CPUs have already entered idle via cpu_pm_enter() when
qcom_mpm_probe() calls cpu_pm_register_notifier(&priv->mpm_pm) with
priv->cpus_in_pm initialized to 0, those idle CPUs will later wake up and
invoke cpu_pm_exit() -> mpm_cpu_pm_callback(CPU_PM_EXIT) without a matching
CPU_PM_ENTER.

That decrements priv->cpus_in_pm below zero when all CPUs are active, so
subsequent CPU_PM_ENTER transitions will never reach num_online_cpus() and
handle_rpm_notification() will never be called.

> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	if (handle_rpm_notification(priv))
> +		return NOTIFY_BAD;

[Severity: High]
If handle_rpm_notification(priv) fails here in mpm_cpu_pm_callback() in
drivers/irqchip/irq-qcom-mpm.c, does priv->cpus_in_pm remain permanently
incremented by one?

On the CPU_PM_ENTER path, priv->cpus_in_pm is incremented before calling
handle_rpm_notification(priv). In kernel/cpu_pm.c, cpu_pm_enter() calls
cpu_pm_notify_robust(), which uses raw_notifier_call_chain_robust() to send
CPU_PM_ENTER_FAILED only to the callbacks that succeeded prior to the failing
callback:

kernel/notifier.c:notifier_call_chain_robust()
    ret = nb->notifier_call(nb, val_up, v);
    if (ret & NOTIFY_STOP_MASK) {
        notifier_call_chain(nl, val_down, v, nr - 1, NULL);
        ...
    }

Because mpm_cpu_pm_callback() will not receive CPU_PM_ENTER_FAILED (and the
caller aborts idle entry without calling cpu_pm_exit()), returning NOTIFY_BAD
without calling atomic_dec(&priv->cpus_in_pm) leaves priv->cpus_in_pm skewed
by +1, causing future idle entries to trigger handle_rpm_notification() while
a CPU is still active.

[Severity: High]
Also, is it valid to call handle_rpm_notification(priv) from
mpm_cpu_pm_callback() while holding the raw spinlock in cpu_pm_notify_robust()?

In kernel/cpu_pm.c, cpu_pm_notify_robust() holds cpu_pm_notifier.lock (a
raw_spinlock_t) across the entire notifier chain:

kernel/cpu_pm.c:cpu_pm_notify_robust()
    raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
    ret = raw_notifier_call_chain_robust(&cpu_pm_notifier.chain, event_up, event_down, NULL);
    raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);

Inside mpm_cpu_pm_callback(), handle_rpm_notification() invokes
mbox_send_message() and mbox_client_txdone() in drivers/mailbox/mailbox.c,
where add_to_rbuf(), msg_submit(), and tx_tick() acquire chan->lock (a
non-raw spinlock_t). In addition, msg_submit() calls
qcom_apcs_ipc_send_data() in drivers/mailbox/qcom-apcs-ipc-mailbox.c, which
calls regmap_write() on a regmap without use_raw_spinlock set, acquiring
map->spinlock (another non-raw spinlock_t).

Acquiring a spinlock_t while holding a raw_spinlock_t violates lockdep wait-type
nesting rules under CONFIG_PROVE_RAW_LOCK_NESTING=y and sleeps in atomic idle
context on PREEMPT_RT kernels.

> +
> +	return NOTIFY_OK;
> +}
> +
>  static bool gic_hwirq_is_mapped(struct mpm_gic_map *maps, int cnt, u32 hwirq)
>  {
>  	int i;

[ ... ]

> @@ -444,6 +467,35 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
>  		return ret;
>  	}
>  
> +	if (of_find_property(np, "#power-domain-cells", NULL)) {
> +		genpd = &priv->genpd;
> +		genpd->flags = GENPD_FLAG_IRQ_SAFE;
> +		genpd->power_off = mpm_pd_power_off;
> +
> +		genpd->name = devm_kasprintf(dev, GFP_KERNEL, "%s", dev_name(dev));
> +		if (!genpd->name)
> +			return -ENOMEM;
> +
> +		ret = pm_genpd_init(genpd, NULL, false);
> +		if (ret) {
> +			dev_err(dev, "failed to init genpd: %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = of_genpd_add_provider_simple(np, genpd);
> +		if (ret) {
> +			dev_err(dev, "failed to add genpd provider: %d\n", ret);
> +			goto remove_genpd;
> +		}
> +	} else {
> +		priv->mpm_pm.notifier_call = mpm_cpu_pm_callback;
> +		ret = cpu_pm_register_notifier(&priv->mpm_pm);
> +		if (ret) {
> +			dev_err(dev, "failed to register for CPU_PM notifications: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	parent_domain = irq_find_host(parent);
>  	if (!parent_domain) {
>  		dev_err(dev, "failed to find MPM parent domain\n");
> @@ -471,10 +523,10 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
>  
>  remove_domain:
>  	irq_domain_remove(priv->domain);
> -free_mbox:
> -	mbox_free_channel(priv->mbox_chan);
>  remove_genpd:
>  	pm_genpd_remove(genpd);
> +free_mbox:
> +	mbox_free_channel(priv->mbox_chan);
>  	return ret;
>  }

[Severity: Critical]
Does the error cleanup in qcom_mpm_probe() in drivers/irqchip/irq-qcom-mpm.c
mishandle failure paths in both the genpd and cpu_pm branches?

There appear to be four related issues on probe failure:

1. In qcom_mpm_probe(), the local variable genpd is declared without
initialization:

    struct generic_pm_domain *genpd;

When #power-domain-cells is absent, the else branch is taken and genpd remains
uninitialized. If devm_request_irq() subsequently fails and jumps to
remove_domain, execution falls through to remove_genpd and calls
pm_genpd_remove(genpd) with an uninitialized pointer.

2. In the else branch, cpu_pm_register_notifier(&priv->mpm_pm) registers the
notifier block embedded in devm-allocated priv into the global cpu_pm_notifier
chain. If irq_find_host(), irq_domain_create_hierarchy(), or
devm_request_irq() fails, cpu_pm_unregister_notifier(&priv->mpm_pm) is never
called before devres frees priv, leaving a dangling notifier in the global
chain.

3. Because remove_genpd now precedes free_mbox, if #power-domain-cells is
present and irq_find_host() or irq_domain_create_hierarchy() fails, jumping to
free_mbox skips remove_genpd (and of_genpd_del_provider(np)), leaving
priv->genpd registered after priv is freed.

4. Because mbox_request_channel() is now called before the genpd and cpu_pm
setup, the direct return statements on !genpd->name, pm_genpd_init() failure,
and cpu_pm_register_notifier() failure return without calling
mbox_free_channel(priv->mbox_chan), leaking the mailbox channel with a
dangling client pointer once priv is freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-b4-shikra_lpm_addition-v3-0-424abcdcf824@oss.qualcomm.com?part=1