[PATCH v3 0/5] pmdomains: Fixes and add support for HFRP Direct

[PATCH v3 0/5] pmdomains: Fixes and add support for HFRP Direct

[AngeloGioacchino Del Regno]

Changes in v3:
 - Added new Fixes commit for possible null pointer kp during cleanup
   of internal on-check
 - Added proper error state cleanup support for SIMPLE_PWRSEQ domains

Changes in v2:
 - Fixed return value checking in power off path

This series adds support for the DirectCTL HFRPSYS power domains found
on the MT8196 SoC (the ones without HW Voter support) and also adds a
fix to respect the power domain relationships during error cleanup,
which avoids HW lockups in case probe deferrals in the specific case
of "almost fully probed" power domains (where most of them probed and
got set up but a probe deferral happened almost at the end), behavior
seen on the MT8189 SoC during bringup (but honestly I have no idea how
are the current ones working fine without this fix...!).

This was tested on MT8173, MT8186, MT8188, MT8189, MT8192, MT8195 and
also on MT8196, over months of development, both manually and over CI,
with no regressions detected.


AngeloGioacchino Del Regno (5):
  dt-bindings: power: mediatek: Add support for MT8196 direct HFRP
  pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check
  pmdomain: mediatek: Respect PD relationships during error cleanup
  pmdomain: mediatek: Add support for Direct CTL simple power sequence
  pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

 .../power/mediatek,power-controller.yaml      |   1 +
 drivers/pmdomain/mediatek/mt8196-pm-domains.h |  27 +++
 drivers/pmdomain/mediatek/mtk-pm-domains.c    | 184 ++++++++++++++----
 drivers/pmdomain/mediatek/mtk-pm-domains.h    |   1 +
 .../dt-bindings/power/mediatek,mt8196-power.h |   4 +
 5 files changed, 184 insertions(+), 33 deletions(-)

-- 
2.54.0

[PATCH v3 1/5] dt-bindings: power: mediatek: Add support for MT8196 direct HFRP

[AngeloGioacchino Del Regno]

Add support for the HFRPSYS direct control power domains in the
MT8196 SoC, controlling power for the DisplayPort and for the
Embedded DisplayPort Transmitter IPs.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../devicetree/bindings/power/mediatek,power-controller.yaml  | 1 +
 include/dt-bindings/power/mediatek,mt8196-power.h             | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index 07f046277f8a..070c6e5666dc 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -34,6 +34,7 @@ properties:
       - mediatek,mt8189-power-controller
       - mediatek,mt8192-power-controller
       - mediatek,mt8195-power-controller
+      - mediatek,mt8196-hfrp-power-controller
       - mediatek,mt8196-hwv-hfrp-power-controller
       - mediatek,mt8196-hwv-scp-power-controller
       - mediatek,mt8196-power-controller
diff --git a/include/dt-bindings/power/mediatek,mt8196-power.h b/include/dt-bindings/power/mediatek,mt8196-power.h
index 0f622a93c807..085790bf8124 100644
--- a/include/dt-bindings/power/mediatek,mt8196-power.h
+++ b/include/dt-bindings/power/mediatek,mt8196-power.h
@@ -30,6 +30,10 @@
 #define MT8196_POWER_DOMAIN_MM_PROC_DORMANT		0
 #define MT8196_POWER_DOMAIN_SSR				1
 
+/* HFRPSYS Multimedia Power Control (MMPC) - Direct Control */
+#define MT8196_POWER_DOMAIN_EDPTX			0
+#define MT8196_POWER_DOMAIN_DPTX			1
+
 /* HFRPSYS MultiMedia Power Control (MMPC) - HW Voter */
 #define MT8196_POWER_DOMAIN_VDE0			0
 #define MT8196_POWER_DOMAIN_VDE1			1
-- 
2.54.0

[PATCH v3 2/5] pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check

[AngeloGioacchino Del Regno]

Should probe fail for HW_VOTER type power domains, this driver was
unconditionally trying to perform cleanup for DIRECT_CTL domains,
but only after checking if the target domain is powered on... with
the DIRECT_CTL scpsys_domain_is_on() code again.

And there's more: the scpsys_domain_is_on() function is also being
unconditionally used in the probe path, for any power domain that
has flag MTK_SCPD_KEEP_DEFAULT_OFF!

This bug was never experienced by anyone because the HWV domains
never failed probe, and because none of those is declared with the
aforementioned flag - but it's still something critical.

In order to fix this, add a check for MTCMOS Type and, based on
that, call the correct functions for an "is on" check, and also
do the same for the cleanup path, calling the correct functions
for the "power off" action.

For the latter, since there's a call to pm_genpd_remove() right
before calling power_off, be cautious and add a variation of the
power off functions (with a _internal suffix) for those to get a
pointer to scpsys_domain instead of one to generic_pm_domain as,
even if that's still working, this is way too much fragile and
would break at some point.

Fixes: 88914db077b6 ("pmdomain: mediatek: Add support for Hardware Voter power domains")
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 40 ++++++++++++++++++----
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index e1cfd4223473..f0a6339affd7 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -393,9 +393,8 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
 	return ret;
 };
 
-static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
+static int scpsys_hwv_power_off_internal(struct scpsys_domain *pd)
 {
-	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
 	const struct scpsys_hwv_domain_data *hwv = pd->hwv_data;
 	struct scpsys *scpsys = pd->scpsys;
 	u32 val;
@@ -464,6 +463,13 @@ static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
 	return ret;
 };
 
+static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
+{
+	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
+
+	return scpsys_hwv_power_off_internal(pd);
+}
+
 static int scpsys_ctl_pwrseq_on(struct scpsys_domain *pd)
 {
 	struct scpsys *scpsys = pd->scpsys;
@@ -694,9 +700,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 	return ret;
 }
 
-static int scpsys_power_off(struct generic_pm_domain *genpd)
+static int scpsys_power_off_internal(struct scpsys_domain *pd)
 {
-	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
 	struct scpsys *scpsys = pd->scpsys;
 	bool tmp;
 	int ret;
@@ -737,6 +742,13 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
 	return 0;
 }
 
+static int scpsys_power_off(struct generic_pm_domain *genpd)
+{
+	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
+
+	return scpsys_power_off_internal(pd);
+}
+
 static struct
 generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
 {
@@ -884,7 +896,14 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	 * late_init time.
 	 */
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF)) {
-		if (scpsys_domain_is_on(pd))
+		bool domain_is_on;
+
+		if (scpsys->soc_data->type == SCPSYS_MTCMOS_TYPE_HW_VOTER)
+			domain_is_on = scpsys_hwv_domain_is_enable_done(pd);
+		else
+			domain_is_on = scpsys_domain_is_on(pd);
+
+		if (domain_is_on)
 			dev_warn(scpsys->dev,
 				 "%pOF: A default off power domain has been ON\n", node);
 	} else {
@@ -973,6 +992,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
 
 static void scpsys_remove_one_domain(struct scpsys_domain *pd)
 {
+	struct scpsys *scpsys = pd->scpsys;
 	int ret;
 
 	/*
@@ -984,8 +1004,14 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
 		dev_err(pd->scpsys->dev,
 			"failed to remove domain '%s' : %d - state may be inconsistent\n",
 			pd->genpd.name, ret);
-	if (scpsys_domain_is_on(pd))
-		scpsys_power_off(&pd->genpd);
+
+	if (scpsys->soc_data->type == SCPSYS_MTCMOS_TYPE_HW_VOTER) {
+		if (scpsys_hwv_domain_is_enable_done(pd))
+			scpsys_hwv_power_off_internal(pd);
+	} else {
+		if (scpsys_domain_is_on(pd))
+			scpsys_power_off_internal(pd);
+	}
 
 	clk_bulk_put(pd->num_clks, pd->clks);
 	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
-- 
2.54.0

Re: [PATCH v3 2/5] pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check

[sashiko-bot]

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

Pre-existing issues:
- [High] Sleeping in atomic context in HW_VOTER power domain callbacks
- [High] Use-After-Free during probe error cleanup due to incorrect teardown order
- [High] Unbalanced clock and regulator disable during error cleanup for domains initialized with MTK_SCPD_KEEP_DEFAULT_OFF
--

--- Patch [2]: [PATCH v3 2/5] pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check ---
commit 6f66af4cf25683016b57cc9dd8c70be44004a959
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: pmdomain: mediatek: Fix possible nullptr KP in HWV cleanup/on-check

This commit fixes a possible NULL pointer dereference in the cleanup and
on-check paths for HW_VOTER type power domains during probe failure. It checks
the MTCMOS type and calls the appropriate functions for checking status and
powering off.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index e1cfd42234734..f0a6339affd75 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -393,9 +393,8 @@ static int scpsys_hwv_power_on(struct generic_pm_domain *genpd)
>  	return ret;
>  };
>  
> -static int scpsys_hwv_power_off(struct generic_pm_domain *genpd)
> +static int scpsys_hwv_power_off_internal(struct scpsys_domain *pd)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but can calling sleeping functions
here cause a kernel panic due to scheduling while atomic?

The driver sets GENPD_FLAG_IRQ_SAFE for HWV domains in scpsys_add_one_domain().
This causes the genpd framework to acquire a spinlock and disable interrupts
before calling scpsys_hwv_power_off() and scpsys_hwv_power_on().

However, these functions then call sleeping functions like
clk_bulk_prepare_enable() and scpsys_regulator_enable(), which would trigger
a scheduling while atomic BUG.

> -	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
>  	const struct scpsys_hwv_domain_data *hwv = pd->hwv_data;
>  	struct scpsys *scpsys = pd->scpsys;

[ ... ]

> @@ -973,6 +992,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
>  
>  static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  {
> +	struct scpsys *scpsys = pd->scpsys;
>  	int ret;
>  
>  	/*
> @@ -984,8 +1004,14 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  		dev_err(pd->scpsys->dev,
>  			"failed to remove domain '%s' : %d - state may be inconsistent\n",
>  			pd->genpd.name, ret);

[Severity: High]
This is a pre-existing issue, but does ignoring this failure leave a dangling
pointer and cause a use-after-free?

During a probe error, scpsys_domain_cleanup() loops backwards by ID to tear
down domains. If the DT topology places a parent domain at a higher reg index
than its child, the loop will attempt to remove the parent first.

When this happens, pm_genpd_remove() will fail with -EBUSY because the child
is still linked (the driver doesn't explicitly unlink subdomains first).
Because the error is ignored here, devres will eventually free the
struct scpsys_domain memory while the domain is still linked in the global
gpd_list.

> -	if (scpsys_domain_is_on(pd))
> -		scpsys_power_off(&pd->genpd);
> +
> +	if (scpsys->soc_data->type == SCPSYS_MTCMOS_TYPE_HW_VOTER) {
> +		if (scpsys_hwv_domain_is_enable_done(pd))
> +			scpsys_hwv_power_off_internal(pd);
> +	} else {
> +		if (scpsys_domain_is_on(pd))
> +			scpsys_power_off_internal(pd);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but could this underflow the clock
and regulator refcounts during error cleanup?

For domains with MTK_SCPD_KEEP_DEFAULT_OFF, the driver skips calling power_on()
during initialization in scpsys_add_one_domain(). This means clocks and
regulators are never prepared or enabled by the driver.

If probe fails and the hardware was left ON by the bootloader, this check sees
the hardware is on and forcibly calls scpsys_hwv_power_off_internal() or
scpsys_power_off_internal(). This will unconditionally disable the clocks and
regulators, underflowing the refcounts for resources the driver never acquired.

>  
>  	clk_bulk_put(pd->num_clks, pd->clks);
>  	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709084635.24912-1-angelogioacchino.delregno@collabora.com?part=2

[PATCH v3 3/5] pmdomain: mediatek: Respect PD relationships during error cleanup

[AngeloGioacchino Del Regno]

In case any probe error occurs (usually, a probe deferral) the
power domains shall be cleaned up while respecting their child
to parent relationship, or the system may freeze.

In order to do that without any memory footprint impacts after
the fact, allocate a temporary array in the probe function and
use it to store the indices of the added power domains in the
correct order.

This will be used in the error cleanup path and will be freed
at the end regardless of the probe status as, when the probing
succeeds, the genpd API takes care of unregistering all PDs in
the correct order anyway.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 43 +++++++++++++++++-----
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index f0a6339affd7..8d49d9ee3cd2 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -750,7 +750,8 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
 }
 
 static struct
-generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
+generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node,
+					 u8 *domains_idx, u8 *num_domains)
 {
 	const struct scpsys_domain_data *domain_data;
 	const struct scpsys_hwv_domain_data *hwv_domain_data;
@@ -925,6 +926,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	else
 		pm_genpd_init(&pd->genpd, NULL, false);
 
+	domains_idx[(*num_domains)++] = (u8) id;
 	scpsys->domains[id] = &pd->genpd;
 
 	return scpsys->pd_data.domains[id];
@@ -936,7 +938,8 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	return ERR_PTR(ret);
 }
 
-static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *parent)
+static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *parent,
+				u8 *domains_idx, u8 *num_domains)
 {
 	struct generic_pm_domain *child_pd, *parent_pd;
 	struct device_node *child;
@@ -959,7 +962,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
 
 		parent_pd = scpsys->pd_data.domains[id];
 
-		child_pd = scpsys_add_one_domain(scpsys, child);
+		child_pd = scpsys_add_one_domain(scpsys, child, domains_idx, num_domains);
 		if (IS_ERR(child_pd)) {
 			ret = PTR_ERR(child_pd);
 			dev_err_probe(scpsys->dev, ret, "%pOF: failed to get child domain id\n",
@@ -968,7 +971,7 @@ static int scpsys_add_subdomain(struct scpsys *scpsys, struct device_node *paren
 		}
 
 		/* recursive call to add all subdomains */
-		ret = scpsys_add_subdomain(scpsys, child);
+		ret = scpsys_add_subdomain(scpsys, child, domains_idx, num_domains);
 		if (ret)
 			goto err_put_node;
 
@@ -1017,14 +1020,16 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
 	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
 }
 
-static void scpsys_domain_cleanup(struct scpsys *scpsys)
+static void scpsys_domain_cleanup(struct scpsys *scpsys, u8 *domains_idx, u8 num_probed)
 {
 	struct generic_pm_domain *genpd;
 	struct scpsys_domain *pd;
 	int i;
 
-	for (i = scpsys->pd_data.num_domains - 1; i >= 0; i--) {
-		genpd = scpsys->pd_data.domains[i];
+	for (i = num_probed - 1; i >= 0; i--) {
+		u8 pd_idx = domains_idx[i];
+
+		genpd = scpsys->pd_data.domains[pd_idx];
 		if (genpd) {
 			pd = to_scpsys_domain(genpd);
 			scpsys_remove_one_domain(pd);
@@ -1241,6 +1246,8 @@ static int scpsys_probe(struct platform_device *pdev)
 	struct device *parent;
 	struct scpsys *scpsys;
 	int num_domains, ret;
+	u8 num_added_pds = 0;
+	u8 *added_pds_idx;
 
 	soc = of_device_get_match_data(&pdev->dev);
 	if (!soc) {
@@ -1254,6 +1261,19 @@ static int scpsys_probe(struct platform_device *pdev)
 	if (!scpsys)
 		return -ENOMEM;
 
+	/*
+	 * Temporarily store the IDs of the power domains that are added as in
+	 * case of a probe deferral this can be used to correctly cleanup all
+	 * of what was added before.
+	 *
+	 * Note that this array is used only in the probe function and must be
+	 * freed at the end, regardless of whether all of the power domains were
+	 * probed successfully or any failure happened.
+	 */
+	added_pds_idx = devm_kmalloc_array(dev, num_domains, sizeof(*added_pds_idx), GFP_KERNEL);
+	if (!added_pds_idx)
+		return -ENOMEM;
+
 	scpsys->dev = dev;
 	scpsys->soc_data = soc;
 
@@ -1284,13 +1304,15 @@ static int scpsys_probe(struct platform_device *pdev)
 	for_each_available_child_of_node_scoped(np, node) {
 		struct generic_pm_domain *domain;
 
-		domain = scpsys_add_one_domain(scpsys, node);
+		domain = scpsys_add_one_domain(scpsys, node,
+					       added_pds_idx, &num_added_pds);
 		if (IS_ERR(domain)) {
 			ret = PTR_ERR(domain);
 			goto err_cleanup_domains;
 		}
 
-		ret = scpsys_add_subdomain(scpsys, node);
+		ret = scpsys_add_subdomain(scpsys, node,
+					   added_pds_idx, &num_added_pds);
 		if (ret)
 			goto err_cleanup_domains;
 	}
@@ -1306,10 +1328,11 @@ static int scpsys_probe(struct platform_device *pdev)
 		goto err_cleanup_domains;
 	}
 
+	devm_kfree(dev, added_pds_idx);
 	return 0;
 
 err_cleanup_domains:
-	scpsys_domain_cleanup(scpsys);
+	scpsys_domain_cleanup(scpsys, added_pds_idx, num_added_pds);
 	return ret;
 }
 
-- 
2.54.0

Re: [PATCH v3 3/5] pmdomain: mediatek: Respect PD relationships during error cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbalanced clock and regulator disables during error cleanup for domains with MTK_SCPD_KEEP_DEFAULT_OFF.
--

--- Patch [3]: [PATCH v3 3/5] pmdomain: mediatek: Respect PD relationships during error cleanup ---
commit 197bd2fa3c14c60d9b5f2423843880328e47fbc9
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: pmdomain: mediatek: Respect PD relationships during error cleanup

In case any probe error occurs (usually, a probe deferral) the
power domains shall be cleaned up while respecting their child
to parent relationship, or the system may freeze.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index f0a6339affd75..8d49d9ee3cd29 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c

[ ... ]

> @@ -1017,14 +1020,16 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  	clk_bulk_put(pd->num_subsys_clks, pd->subsys_clks);
>  }
>  
> -static void scpsys_domain_cleanup(struct scpsys *scpsys)
> +static void scpsys_domain_cleanup(struct scpsys *scpsys, u8 *domains_idx, u8 num_probed)
>  {
>  	struct generic_pm_domain *genpd;
>  	struct scpsys_domain *pd;
>  	int i;
>  
> -	for (i = scpsys->pd_data.num_domains - 1; i >= 0; i--) {
> -		genpd = scpsys->pd_data.domains[i];
> +	for (i = num_probed - 1; i >= 0; i--) {
> +		u8 pd_idx = domains_idx[i];
> +
> +		genpd = scpsys->pd_data.domains[pd_idx];
>  		if (genpd) {
>  			pd = to_scpsys_domain(genpd);
>  			scpsys_remove_one_domain(pd);

[Severity: High]
This is a pre-existing issue, but does the error cleanup path cause unbalanced
clock and regulator disables for default-off domains?

In scpsys_add_one_domain(), if a domain is configured with
MTK_SCPD_KEEP_DEFAULT_OFF, it intentionally skips turning on the domain,
meaning clk_bulk_prepare_enable() and scpsys_regulator_enable() are never
called by the driver during initialization.

However, if a probe deferral occurs later in scpsys_probe(), this cleanup path
calls scpsys_remove_one_domain(). If the domain was left ON by the bootloader,
scpsys_remove_one_domain() unconditionally invokes the internal power-off
functions:

drivers/pmdomain/mediatek/mtk-pm-domains.c:scpsys_remove_one_domain() {
    ...
    if (scpsys->soc_data->type == SCPSYS_MTCMOS_TYPE_HW_VOTER) {
        if (scpsys_hwv_domain_is_enable_done(pd))
            scpsys_hwv_power_off_internal(pd);
    } else {
        if (scpsys_domain_is_on(pd))
            scpsys_power_off_internal(pd);
    }
    ...
}

Since the driver never successfully enabled these resources, will these
unconditional power off calls decrement their usage counts below zero,
potentially leading to kernel warnings or system crashes from unintentionally
disabling shared clocks or regulators?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709084635.24912-1-angelogioacchino.delregno@collabora.com?part=3

[PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[AngeloGioacchino Del Regno]

Some new SoCs like MT8196, MT6991, and others, have got one
additional power controller (usually in the HFRP Multimedia
block) which needs a simplified power on/off sequence while
using Direct Control strategy.

Domains using the "simple power sequence" are not backed by
the RTFF hardware, have no Bus Protection mechanism, lacks
the ISO, PWR_ON, PWR_ON_2ND bits, and therefore get enabled
automatically after getting out of reset.

This simple power sequence is then a subset of the full one
as only needs the enablement of the specific power domain's
clock input and reset (where, again, after getting out of
reset, the ISO and PWR_ON bits are automatically internally
getting flipped) to enable or disable (power on or off).

Moreover, the simple power sequence power domains guarantee
that they always get enabled/disabled after executing the
relevant power sequence (on/off) so, differently from the
others, there is also no need to poll for a PWR_ACK.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 97 ++++++++++++++++++----
 drivers/pmdomain/mediatek/mtk-pm-domains.h |  1 +
 2 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index 8d49d9ee3cd2..632c5dcdceda 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -555,9 +555,11 @@ static int scpsys_ctl_pwrseq_on(struct scpsys_domain *pd)
 	return 0;
 }
 
-static void scpsys_ctl_pwrseq_off(struct scpsys_domain *pd)
+static int scpsys_ctl_pwrseq_off(struct scpsys_domain *pd)
 {
 	struct scpsys *scpsys = pd->scpsys;
+	bool tmp;
+	int ret;
 
 	switch (pd->data->rtff_type) {
 	case SCPSYS_RTFF_TYPE_GENERIC:
@@ -589,6 +591,41 @@ static void scpsys_ctl_pwrseq_off(struct scpsys_domain *pd)
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_2ND_BIT);
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_BIT);
+
+	/* wait until PWR_ACK = 0 */
+	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
+				 MTK_POLL_TIMEOUT);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int scpsys_simple_pwrseq_on(struct scpsys_domain *pd)
+{
+	struct scpsys *scpsys = pd->scpsys;
+
+	/* Enable subsys clock input and trigger power domain reset state */
+	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_CLK_DIS_BIT);
+	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+
+	/* Wait for the hardware to stabilize */
+	udelay(1);
+
+	/* Get out of reset: set power on */
+	regmap_set_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+
+	return 0;
+}
+
+static int scpsys_simple_pwrseq_off(struct scpsys_domain *pd)
+{
+	struct scpsys *scpsys = pd->scpsys;
+
+	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+	regmap_set_bits(scpsys->base, pd->data->ctl_offs, PWR_CLK_DIS_BIT);
+
+	return 0;
 }
 
 static int scpsys_modem_pwrseq_on(struct scpsys_domain *pd)
@@ -611,14 +648,24 @@ static int scpsys_modem_pwrseq_on(struct scpsys_domain *pd)
 	return 0;
 }
 
-static void scpsys_modem_pwrseq_off(struct scpsys_domain *pd)
+static int scpsys_modem_pwrseq_off(struct scpsys_domain *pd)
 {
 	struct scpsys *scpsys = pd->scpsys;
+	bool tmp;
+	int ret;
 
 	regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_BIT);
 
 	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SKIP_RESET_B))
 		regmap_clear_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT);
+
+	/* wait until PWR_ACK = 0 */
+	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
+				 MTK_POLL_TIMEOUT);
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 static int scpsys_power_on(struct generic_pm_domain *genpd)
@@ -641,6 +688,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
 		ret = scpsys_modem_pwrseq_on(pd);
+	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
+		ret = scpsys_simple_pwrseq_on(pd);
 	else
 		ret = scpsys_ctl_pwrseq_on(pd);
 
@@ -668,9 +717,11 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 			goto err_pwr_ack;
 	}
 
-	ret = scpsys_sram_enable(pd);
-	if (ret < 0)
-		goto err_disable_subsys_clks;
+	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
+		ret = scpsys_sram_enable(pd);
+		if (ret < 0)
+			goto err_disable_subsys_clks;
+	}
 
 	ret = scpsys_bus_protect_disable(pd, 0);
 	if (ret < 0)
@@ -688,7 +739,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 err_enable_bus_protect:
 	scpsys_bus_protect_enable(pd, 0);
 err_disable_sram:
-	scpsys_sram_disable(pd);
+	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
+		scpsys_sram_disable(pd);
 err_disable_subsys_clks:
 	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
 		clk_bulk_disable_unprepare(pd->num_subsys_clks,
@@ -703,16 +755,17 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 static int scpsys_power_off_internal(struct scpsys_domain *pd)
 {
 	struct scpsys *scpsys = pd->scpsys;
-	bool tmp;
 	int ret;
 
 	ret = scpsys_bus_protect_enable(pd, 0);
 	if (ret < 0)
 		return ret;
 
-	ret = scpsys_sram_disable(pd);
-	if (ret < 0)
-		return ret;
+	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ)) {
+		ret = scpsys_sram_disable(pd);
+		if (ret < 0)
+			return ret;
+	}
 
 	if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, MTK_SCPD_EXT_BUCK_ISO))
 		regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
@@ -725,15 +778,21 @@ static int scpsys_power_off_internal(struct scpsys_domain *pd)
 		return ret;
 
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
-		scpsys_modem_pwrseq_off(pd);
+		ret = scpsys_modem_pwrseq_off(pd);
+	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
+		ret = scpsys_simple_pwrseq_off(pd);
 	else
-		scpsys_ctl_pwrseq_off(pd);
+		ret = scpsys_ctl_pwrseq_off(pd);
+
+	if (ret < 0) {
+		/* Re-enable clocks so that next power off doesn't break the refcount */
+		int r = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
+
+		if (r)
+			dev_warn(scpsys->dev, "Could not re-enable clocks: %d\n", r);
 
-	/* wait until PWR_ACK = 0 */
-	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
-				 MTK_POLL_TIMEOUT);
-	if (ret < 0)
 		return ret;
+	}
 
 	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
 
@@ -1109,6 +1168,12 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
 		regmap[2] = NULL;
 	}
 
+	/* If no access controllers are needed, don't allocate and don't fail */
+	if (num_regmaps == 0) {
+		scpsys->bus_prot = NULL;
+		return 0;
+	}
+
 	scpsys->bus_prot = devm_kmalloc_array(dev, num_regmaps,
 					      sizeof(*scpsys->bus_prot), GFP_KERNEL);
 	if (!scpsys->bus_prot)
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index a5dca24cbc2f..092403de66fa 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -17,6 +17,7 @@
 #define MTK_SCPD_MODEM_PWRSEQ		BIT(10)
 #define MTK_SCPD_SKIP_RESET_B		BIT(11)
 #define MTK_SCPD_INFRA_PWR_CTL		BIT(12)
+#define MTK_SCPD_SIMPLE_PWRSEQ		BIT(13)
 #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data ?		\
 					 (_scpd)->data->caps & (_x) :	\
 					 (_scpd)->hwv_data->caps & (_x))
-- 
2.54.0

Re: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete error recovery in `scpsys_power_off_internal` leaves hardware paralyzed and causes clock refcount underflows.
- [High] Unbalanced lifecycle operations in `scpsys_power_on` fail to revert hardware state on error.
- [Medium] Malformed device trees lacking access controllers guarantee a NULL pointer dereference in `scpsys_bus_protect_get_regmap`.
--

--- Patch [4]: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence ---
commit 0a6955778a8abcdf284811fe9a9089e2c1cff6d6
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

pmdomain: mediatek: Add support for Direct CTL simple power sequence

Some new SoCs like MT8196, MT6991, and others, have got one
additional power controller (usually in the HFRP Multimedia
block) which needs a simplified power on/off sequence while
using Direct Control strategy.

Domains using the "simple power sequence" are not backed by
the RTFF hardware, have no Bus Protection mechanism, lacks
the ISO, PWR_ON, PWR_ON_2ND bits, and therefore get enabled
automatically after getting out of reset.

This simple power sequence is then a subset of the full one
as only needs the enablement of the specific power domain's
clock input and reset (where, again, after getting out of
reset, the ISO and PWR_ON bits are automatically internally
getting flipped) to enable or disable (power on or off).

Moreover, the simple power sequence power domains guarantee
that they always get enabled/disabled after executing the
relevant power sequence (on/off) so, differently from the
others, there is also no need to poll for a PWR_ACK.

> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index 8d49d9ee3cd29..632c5dcdcedaf 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
[ ... ]
> @@ -688,7 +739,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>  err_enable_bus_protect:
>  	scpsys_bus_protect_enable(pd, 0);
>  err_disable_sram:
> -	scpsys_sram_disable(pd);
> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
> +		scpsys_sram_disable(pd);
>  err_disable_subsys_clks:
>  	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
>  		clk_bulk_disable_unprepare(pd->num_subsys_clks,
>  					   pd->subsys_clks);

[Severity: High]
This is a pre-existing issue, but does this error path fail to revert
the hardware state if an error occurs?

If scpsys_bus_protect_disable() or clk_bulk_prepare_enable() fails after
*_pwrseq_on() has successfully executed, the error path jumps to err_pwr_ack.

This path disables the domain's regulator and clocks but fails to execute
*_pwrseq_off() or revert ext_buck_iso:

err_pwr_ack:
	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
err_reg:
	scpsys_regulator_disable(pd->supply);
	return ret;

Could this sever the supply while leaving the domain's internal logic active,
resulting in hardware lockup and state machine desynchronization?

[ ... ]
> @@ -725,15 +778,21 @@ static int scpsys_power_off_internal(struct scpsys_domain *pd)
>  		return ret;
>  
>  	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
> -		scpsys_modem_pwrseq_off(pd);
> +		ret = scpsys_modem_pwrseq_off(pd);
> +	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
> +		ret = scpsys_simple_pwrseq_off(pd);
>  	else
> -		scpsys_ctl_pwrseq_off(pd);
> +		ret = scpsys_ctl_pwrseq_off(pd);
> +
> +	if (ret < 0) {
> +		/* Re-enable clocks so that next power off doesn't break the refcount */
> +		int r = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
> +
> +		if (r)
> +			dev_warn(scpsys->dev, "Could not re-enable clocks: %d\n", r);
>  
> -	/* wait until PWR_ACK = 0 */
> -	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
> -				 MTK_POLL_TIMEOUT);
> -	if (ret < 0)
>  		return ret;
> +	}
>  
>  	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);

[Severity: High]
This isn't a bug introduced by this patch, but does the error recovery in
scpsys_power_off_internal() leave the hardware in a paralyzed state?

If pwrseq_off fails, this block restores subsys_clks but leaves the domain's
SRAM disabled, isolation applied, and bus protection disabled, while
returning an error to genpd.

Because an error is returned, genpd treats the domain as still ON, allowing
active consumers to interact with a paralyzed hardware domain.

Additionally, if scpsys_bus_protect_enable() fails earlier in this function:

	ret = scpsys_bus_protect_enable(pd, BUS_PROT_IGNORE_SUBCLK);
	if (ret < 0)
		return ret;

It returns early and bypasses clock recovery entirely, causing a permanent
clock refcount underflow on the next failure.

Can we ensure consistent state on power off errors?

[ ... ]
> @@ -1109,6 +1168,12 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>  		regmap[2] = NULL;
>  	}
>  
> +	/* If no access controllers are needed, don't allocate and don't fail */
> +	if (num_regmaps == 0) {
> +		scpsys->bus_prot = NULL;
> +		return 0;
> +	}
> +
>  	scpsys->bus_prot = devm_kmalloc_array(dev, num_regmaps,
>  					      sizeof(*scpsys->bus_prot), GFP_KERNEL);

[Severity: Medium]
This is a pre-existing issue, but does this result in a NULL pointer
dereference if the device tree is malformed?

If an older SoC's device tree is missing required access controllers,
num_regmaps becomes 0. The probe function succeeds and explicitly sets
scpsys->bus_prot = NULL.

When the domain later attempts to power on, scpsys_bus_protect_enable() sees
that the hardcoded SoC domain data still requests bus protection and
unconditionally dereferences the NULL array in
scpsys_bus_protect_get_regmap():

	unsigned short block_idx = scpsys->bus_prot_index[bpd->bus_prot_block];
	return scpsys->bus_prot[block_idx];

Should we ensure scpsys->bus_prot is not dereferenced when NULL, or perhaps
fail the probe if required controllers are missing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709084635.24912-1-angelogioacchino.delregno@collabora.com?part=4

Re: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence

[AngeloGioacchino Del Regno]

On 7/9/26 10:58, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incomplete error recovery in `scpsys_power_off_internal` leaves hardware paralyzed and causes clock refcount underflows.
> - [High] Unbalanced lifecycle operations in `scpsys_power_on` fail to revert hardware state on error.

Unfortunately there's no way of fixing those two briefly; the hardware may randomly
lockup if changes are reverted at that point.

I'm not even sure if there's any way at all of fixing this, but research will
be done a bit later. Not relevant for this series.

> - [Medium] Malformed device trees lacking access controllers guarantee a NULL pointer dereference in `scpsys_bus_protect_get_regmap`.

No, that's not true - only HFRPSYS HWV lack access controllers, and will use
different ops, so that's not going to create any NULL pointer dereference.

> --
> 
> --- Patch [4]: [PATCH v3 4/5] pmdomain: mediatek: Add support for Direct CTL simple power sequence ---
> commit 0a6955778a8abcdf284811fe9a9089e2c1cff6d6
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> pmdomain: mediatek: Add support for Direct CTL simple power sequence
> 
> Some new SoCs like MT8196, MT6991, and others, have got one
> additional power controller (usually in the HFRP Multimedia
> block) which needs a simplified power on/off sequence while
> using Direct Control strategy.
> 
> Domains using the "simple power sequence" are not backed by
> the RTFF hardware, have no Bus Protection mechanism, lacks
> the ISO, PWR_ON, PWR_ON_2ND bits, and therefore get enabled
> automatically after getting out of reset.
> 
> This simple power sequence is then a subset of the full one
> as only needs the enablement of the specific power domain's
> clock input and reset (where, again, after getting out of
> reset, the ISO and PWR_ON bits are automatically internally
> getting flipped) to enable or disable (power on or off).
> 
> Moreover, the simple power sequence power domains guarantee
> that they always get enabled/disabled after executing the
> relevant power sequence (on/off) so, differently from the
> others, there is also no need to poll for a PWR_ACK.
> 
>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> index 8d49d9ee3cd29..632c5dcdcedaf 100644
>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> [ ... ]
>> @@ -688,7 +739,8 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>   err_enable_bus_protect:
>>   	scpsys_bus_protect_enable(pd, 0);
>>   err_disable_sram:
>> -	scpsys_sram_disable(pd);
>> +	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>> +		scpsys_sram_disable(pd);
>>   err_disable_subsys_clks:
>>   	if (!MTK_SCPD_CAPS(pd, MTK_SCPD_STRICT_BUS_PROTECTION))
>>   		clk_bulk_disable_unprepare(pd->num_subsys_clks,
>>   					   pd->subsys_clks);
> 
> [Severity: High]
> This is a pre-existing issue, but does this error path fail to revert
> the hardware state if an error occurs?
> 
> If scpsys_bus_protect_disable() or clk_bulk_prepare_enable() fails after
> *_pwrseq_on() has successfully executed, the error path jumps to err_pwr_ack.
> 
> This path disables the domain's regulator and clocks but fails to execute
> *_pwrseq_off() or revert ext_buck_iso:
> 
> err_pwr_ack:
> 	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> err_reg:
> 	scpsys_regulator_disable(pd->supply);
> 	return ret;
> 
> Could this sever the supply while leaving the domain's internal logic active,
> resulting in hardware lockup and state machine desynchronization?
> 
> [ ... ]
>> @@ -725,15 +778,21 @@ static int scpsys_power_off_internal(struct scpsys_domain *pd)
>>   		return ret;
>>   
>>   	if (MTK_SCPD_CAPS(pd, MTK_SCPD_MODEM_PWRSEQ))
>> -		scpsys_modem_pwrseq_off(pd);
>> +		ret = scpsys_modem_pwrseq_off(pd);
>> +	else if (MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>> +		ret = scpsys_simple_pwrseq_off(pd);
>>   	else
>> -		scpsys_ctl_pwrseq_off(pd);
>> +		ret = scpsys_ctl_pwrseq_off(pd);
>> +
>> +	if (ret < 0) {
>> +		/* Re-enable clocks so that next power off doesn't break the refcount */
>> +		int r = clk_bulk_prepare_enable(pd->num_subsys_clks, pd->subsys_clks);
>> +
>> +		if (r)
>> +			dev_warn(scpsys->dev, "Could not re-enable clocks: %d\n", r);
>>   
>> -	/* wait until PWR_ACK = 0 */
>> -	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
>> -				 MTK_POLL_TIMEOUT);
>> -	if (ret < 0)
>>   		return ret;
>> +	}
>>   
>>   	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the error recovery in
> scpsys_power_off_internal() leave the hardware in a paralyzed state?
> 
> If pwrseq_off fails, this block restores subsys_clks but leaves the domain's
> SRAM disabled, isolation applied, and bus protection disabled, while
> returning an error to genpd.
> 
> Because an error is returned, genpd treats the domain as still ON, allowing
> active consumers to interact with a paralyzed hardware domain.
> 
> Additionally, if scpsys_bus_protect_enable() fails earlier in this function:
> 
> 	ret = scpsys_bus_protect_enable(pd, BUS_PROT_IGNORE_SUBCLK);
> 	if (ret < 0)
> 		return ret;
> 
> It returns early and bypasses clock recovery entirely, causing a permanent
> clock refcount underflow on the next failure.
> 
> Can we ensure consistent state on power off errors?
> 
> [ ... ]
>> @@ -1109,6 +1168,12 @@ static int scpsys_get_bus_protection_legacy(struct device *dev, struct scpsys *s
>>   		regmap[2] = NULL;
>>   	}
>>   
>> +	/* If no access controllers are needed, don't allocate and don't fail */
>> +	if (num_regmaps == 0) {
>> +		scpsys->bus_prot = NULL;
>> +		return 0;
>> +	}
>> +
>>   	scpsys->bus_prot = devm_kmalloc_array(dev, num_regmaps,
>>   					      sizeof(*scpsys->bus_prot), GFP_KERNEL);
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this result in a NULL pointer
> dereference if the device tree is malformed?
> 
> If an older SoC's device tree is missing required access controllers,
> num_regmaps becomes 0. The probe function succeeds and explicitly sets
> scpsys->bus_prot = NULL.
> 
> When the domain later attempts to power on, scpsys_bus_protect_enable() sees
> that the hardcoded SoC domain data still requests bus protection and
> unconditionally dereferences the NULL array in
> scpsys_bus_protect_get_regmap():
> 
> 	unsigned short block_idx = scpsys->bus_prot_index[bpd->bus_prot_block];
> 	return scpsys->bus_prot[block_idx];
> 
> Should we ensure scpsys->bus_prot is not dereferenced when NULL, or perhaps
> fail the probe if required controllers are missing?
> 

[PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[AngeloGioacchino Del Regno]

Add support for the power domains provided by the HFRPSYS Power
Controller of the MT8196 SoC.
Those control power to the eDP and DP Transmitter IPs.

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pmdomain/mediatek/mt8196-pm-domains.h | 27 +++++++++++++++++++
 drivers/pmdomain/mediatek/mtk-pm-domains.c    |  6 ++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/pmdomain/mediatek/mt8196-pm-domains.h b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
index 2e4b28720659..d704c9fa9337 100644
--- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
@@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
 	},
 };
 
+static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
+	[MT8196_POWER_DOMAIN_EDPTX] = {
+		.name = "edp-tx",
+		.sta_mask = MT8196_PWR_ACK,
+		.sta2nd_mask = MT8196_PWR_ACK_2ND,
+		.ctl_offs = 0x74,
+		.pwr_sta_offs = 0x74,
+		.pwr_sta2nd_offs = 0x74,
+		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
+	},
+	[MT8196_POWER_DOMAIN_DPTX] = {
+		.name = "dp-tx",
+		.sta_mask = MT8196_PWR_ACK,
+		.sta2nd_mask = MT8196_PWR_ACK_2ND,
+		.ctl_offs = 0x78,
+		.pwr_sta_offs = 0x78,
+		.pwr_sta2nd_offs = 0x78,
+		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
+	},
+};
+
 static const struct scpsys_soc_data mt8196_scpsys_data = {
 	.domains_data = scpsys_domain_data_mt8196,
 	.num_domains = ARRAY_SIZE(scpsys_domain_data_mt8196),
@@ -616,6 +637,12 @@ static const struct scpsys_soc_data mt8196_scpsys_hwv_data = {
 	.type = SCPSYS_MTCMOS_TYPE_HW_VOTER,
 };
 
+static const struct scpsys_soc_data mt8196_hfrpsys_data = {
+	.domains_data = hfrpsys_domain_data_mt8196,
+	.num_domains = ARRAY_SIZE(hfrpsys_domain_data_mt8196),
+	.type = SCPSYS_MTCMOS_TYPE_DIRECT_CTL,
+};
+
 static const struct scpsys_soc_data mt8196_hfrpsys_hwv_data = {
 	.hwv_domains_data = hfrpsys_hwv_domain_data_mt8196,
 	.num_hwv_domains = ARRAY_SIZE(hfrpsys_hwv_domain_data_mt8196),
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index 632c5dcdceda..8309a4b46afb 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -1071,7 +1071,7 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
 		if (scpsys_hwv_domain_is_enable_done(pd))
 			scpsys_hwv_power_off_internal(pd);
 	} else {
-		if (scpsys_domain_is_on(pd))
+		if (scpsys_domain_is_on(pd) || MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
 			scpsys_power_off_internal(pd);
 	}
 
@@ -1288,6 +1288,10 @@ static const struct of_device_id scpsys_of_match[] = {
 		.compatible = "mediatek,mt8196-power-controller",
 		.data = &mt8196_scpsys_data,
 	},
+	{
+		.compatible = "mediatek,mt8196-hfrp-power-controller",
+		.data = &mt8196_hfrpsys_data,
+	},
 	{
 		.compatible = "mediatek,mt8196-hwv-hfrp-power-controller",
 		.data = &mt8196_hfrpsys_hwv_data,
-- 
2.54.0

Re: [PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[sashiko-bot]

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

New issues:
- [Medium] Unconditional power-off of SIMPLE_PWRSEQ domains in error cleanup, masking potentially incorrect status mask configurations.

Pre-existing issues:
- [Critical] Missing error handling for pm_genpd_init() causes kernel panic via NULL pointer dereference during error cleanup.
- [Critical] Missing subdomain unlinking during probe error cleanup leaves freed genpd objects in the global gpd_list, causing Use-After-Free.
- [High] HW_VOTER domains invoke sleepable clock and regulator APIs while holding a spinlock with IRQs disabled.
- [High] Missing GENPD_FLAG_NO_STAY_ON or sync_state callback causes regulator-supplied domains to stay on indefinitely, leading to hardware malfunction on regulator cleanup.
- [High] Unbalanced resource cleanup on the error path for KEEP_DEFAULT_OFF domains left on by the bootloader.
--

--- Patch [5]: [PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains ---
commit 5fec9c9afff1947a88f0033701c6c01f031caa0d
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

Add support for the power domains provided by the HFRPSYS Power Controller of
the MT8196 SoC. Those control power to the eDP and DP Transmitter IPs.

> --- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> +++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
> @@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
>  	},
>  };
>  
> +static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
> +	[MT8196_POWER_DOMAIN_EDPTX] = {
> +		.name = "edp-tx",
> +		.sta_mask = MT8196_PWR_ACK,
> +		.sta2nd_mask = MT8196_PWR_ACK_2ND,
> +		.ctl_offs = 0x74,
> +		.pwr_sta_offs = 0x74,
> +		.pwr_sta2nd_offs = 0x74,
> +		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
> +	},

[ ... ]

> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -1071,7 +1071,7 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>  		if (scpsys_hwv_domain_is_enable_done(pd))
>  			scpsys_hwv_power_off_internal(pd);

[Severity: High]
This isn't a bug introduced by this patch, but do HW_VOTER domains invoke
sleepable clock and regulator APIs while holding a spinlock with IRQs
disabled?

Since HW_VOTER domains are registered with GENPD_FLAG_IRQ_SAFE, the genpd core
guarantees their power_on and power_off callbacks run with interrupts
disabled. However, scpsys_hwv_power_on() and scpsys_hwv_power_off()
unconditionally call clk_bulk_prepare_enable() and scpsys_regulator_enable(),
which take sleepable mutexes.

Can this result in a scheduling while atomic BUG or kernel panic?

>  	} else {
> -		if (scpsys_domain_is_on(pd))
> +		if (scpsys_domain_is_on(pd) || MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>  			scpsys_power_off_internal(pd);
>  	}

[Severity: Medium]
Will this unconditionally force power off for SIMPLE_PWRSEQ domains on the
error path?

If a SIMPLE_PWRSEQ domain is ever configured with MTK_SCPD_KEEP_DEFAULT_OFF
and probe fails, it appears the cleanup path unconditionally powers it off
regardless of whether it was active. Could this lead to disabling clocks that
were never enabled, resulting in a clock underflow WARN_ON?

Is it possible this acts as a workaround that masks the fact that
scpsys_domain_is_on() fails to report the true state for these domains due
to their status mask configurations (like MT8196_PWR_ACK for edp-tx)?

[Severity: Critical]
This is a pre-existing issue, but does scpsys_add_one_domain() ignore the
return value of pm_genpd_init()?

If pm_genpd_init() fails, it seems genpd->gpd_list_node remains 0-initialized.
On a subsequent probe failure, the error cleanup loop calls pm_genpd_remove()
which executes list_del(&genpd->gpd_list_node).

Could this dereference a NULL pointer and cause a kernel panic since the node
was never initialized?

[Severity: Critical]
This is a pre-existing issue, but does scpsys_domain_cleanup() iterate
backwards calling scpsys_remove_one_domain() without breaking parent-child
links first?

When this happens, pm_genpd_remove() returns -EBUSY and aborts without
unlinking the domain from gpd_list. Since the driver ignores this error, devm
may free the still-linked genpd objects.

Could background processes like genpd_power_off_unused later iterate gpd_list
and trigger a use-after-free when accessing the freed memory?

[Severity: High]
This is a pre-existing issue, but is there unbalanced resource cleanup on
the error path for KEEP_DEFAULT_OFF domains left on by the bootloader?

If such a domain is left physically ON, the driver warns but skips OS resource
preparation, leaving the enable_count at 0. If probe fails later, this path in
scpsys_remove_one_domain() evaluates scpsys_domain_is_on() as true and calls
scpsys_power_off_internal().

Could this unconditionally disable resources the OS never enabled, triggering
a clock/regulator underflow WARN_ON?

[ ... ]

> @@ -1288,6 +1288,10 @@ static const struct of_device_id scpsys_of_match[] = {
>  		.compatible = "mediatek,mt8196-power-controller",
>  		.data = &mt8196_scpsys_data,
>  	},
> +	{
> +		.compatible = "mediatek,mt8196-hfrp-power-controller",
> +		.data = &mt8196_hfrpsys_data,
> +	},

[Severity: High]
This is a pre-existing issue, but is the platform driver missing the
.sync_state callback required to clear the stay_on flag for default-ON
domains?

Since the driver initializes default-on domains with is_off = false, the genpd
core sets genpd->stay_on = true. Without a .sync_state callback or
GENPD_FLAG_NO_STAY_ON, stay_on is never cleared, and the framework will refuse
to power off unused domains.

When regulator_late_cleanup() later forces unused regulators off, could this
silently cut physical power to domains the kernel still considers logically
active and cause a hardware malfunction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709084635.24912-1-angelogioacchino.delregno@collabora.com?part=5

Re: [PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains

[AngeloGioacchino Del Regno]

On 7/9/26 11:00, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Unconditional power-off of SIMPLE_PWRSEQ domains in error cleanup, masking potentially incorrect status mask configurations.
> 

That's not an issue. It's done on purpose.

> Pre-existing issues:
> - [Critical] Missing error handling for pm_genpd_init() causes kernel panic via NULL pointer dereference during error cleanup.
> - [Critical] Missing subdomain unlinking during probe error cleanup leaves freed genpd objects in the global gpd_list, causing Use-After-Free.
> - [High] HW_VOTER domains invoke sleepable clock and regulator APIs while holding a spinlock with IRQs disabled.
> - [High] Missing GENPD_FLAG_NO_STAY_ON or sync_state callback causes regulator-supplied domains to stay on indefinitely, leading to hardware malfunction on regulator cleanup.
> - [High] Unbalanced resource cleanup on the error path for KEEP_DEFAULT_OFF domains left on by the bootloader.

I have already fixed whatever I could, can't do the rest in this cycle; maybe in
september I can.

Also, this shall not block this series from being picked (as it also contains
important fixes inside), because those are all pre-existing issues.

> --
> 
> --- Patch [5]: [PATCH v3 5/5] pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains ---
> commit 5fec9c9afff1947a88f0033701c6c01f031caa0d
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> pmdomain: mediatek: Add support for MT8196 HFRP DirectCTL domains
> 
> Add support for the power domains provided by the HFRPSYS Power Controller of
> the MT8196 SoC. Those control power to the eDP and DP Transmitter IPs.
> 
>> --- a/drivers/pmdomain/mediatek/mt8196-pm-domains.h
>> +++ b/drivers/pmdomain/mediatek/mt8196-pm-domains.h
>> @@ -602,6 +602,27 @@ static const struct scpsys_hwv_domain_data hfrpsys_hwv_domain_data_mt8196[] = {
>>   	},
>>   };
>>   
>> +static const struct scpsys_domain_data hfrpsys_domain_data_mt8196[] = {
>> +	[MT8196_POWER_DOMAIN_EDPTX] = {
>> +		.name = "edp-tx",
>> +		.sta_mask = MT8196_PWR_ACK,
>> +		.sta2nd_mask = MT8196_PWR_ACK_2ND,
>> +		.ctl_offs = 0x74,
>> +		.pwr_sta_offs = 0x74,
>> +		.pwr_sta2nd_offs = 0x74,
>> +		.caps = MTK_SCPD_SIMPLE_PWRSEQ,
>> +	},
> 
> [ ... ]
> 
>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> @@ -1071,7 +1071,7 @@ static void scpsys_remove_one_domain(struct scpsys_domain *pd)
>>   		if (scpsys_hwv_domain_is_enable_done(pd))
>>   			scpsys_hwv_power_off_internal(pd);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but do HW_VOTER domains invoke
> sleepable clock and regulator APIs while holding a spinlock with IRQs
> disabled?
> 
> Since HW_VOTER domains are registered with GENPD_FLAG_IRQ_SAFE, the genpd core
> guarantees their power_on and power_off callbacks run with interrupts
> disabled. However, scpsys_hwv_power_on() and scpsys_hwv_power_off()
> unconditionally call clk_bulk_prepare_enable() and scpsys_regulator_enable(),
> which take sleepable mutexes.
> 
> Can this result in a scheduling while atomic BUG or kernel panic?
> 
>>   	} else {
>> -		if (scpsys_domain_is_on(pd))
>> +		if (scpsys_domain_is_on(pd) || MTK_SCPD_CAPS(pd, MTK_SCPD_SIMPLE_PWRSEQ))
>>   			scpsys_power_off_internal(pd);
>>   	}
> 
> [Severity: Medium]
> Will this unconditionally force power off for SIMPLE_PWRSEQ domains on the
> error path?
> 
> If a SIMPLE_PWRSEQ domain is ever configured with MTK_SCPD_KEEP_DEFAULT_OFF
> and probe fails, it appears the cleanup path unconditionally powers it off
> regardless of whether it was active. Could this lead to disabling clocks that
> were never enabled, resulting in a clock underflow WARN_ON?
> 
> Is it possible this acts as a workaround that masks the fact that
> scpsys_domain_is_on() fails to report the true state for these domains due
> to their status mask configurations (like MT8196_PWR_ACK for edp-tx)?
> 
> [Severity: Critical]
> This is a pre-existing issue, but does scpsys_add_one_domain() ignore the
> return value of pm_genpd_init()?
> 
> If pm_genpd_init() fails, it seems genpd->gpd_list_node remains 0-initialized.
> On a subsequent probe failure, the error cleanup loop calls pm_genpd_remove()
> which executes list_del(&genpd->gpd_list_node).
> 
> Could this dereference a NULL pointer and cause a kernel panic since the node
> was never initialized?
> 
> [Severity: Critical]
> This is a pre-existing issue, but does scpsys_domain_cleanup() iterate
> backwards calling scpsys_remove_one_domain() without breaking parent-child
> links first?
> 
> When this happens, pm_genpd_remove() returns -EBUSY and aborts without
> unlinking the domain from gpd_list. Since the driver ignores this error, devm
> may free the still-linked genpd objects.
> 
> Could background processes like genpd_power_off_unused later iterate gpd_list
> and trigger a use-after-free when accessing the freed memory?
> 
> [Severity: High]
> This is a pre-existing issue, but is there unbalanced resource cleanup on
> the error path for KEEP_DEFAULT_OFF domains left on by the bootloader?
> 
> If such a domain is left physically ON, the driver warns but skips OS resource
> preparation, leaving the enable_count at 0. If probe fails later, this path in
> scpsys_remove_one_domain() evaluates scpsys_domain_is_on() as true and calls
> scpsys_power_off_internal().
> 
> Could this unconditionally disable resources the OS never enabled, triggering
> a clock/regulator underflow WARN_ON?
> 
> [ ... ]
> 
>> @@ -1288,6 +1288,10 @@ static const struct of_device_id scpsys_of_match[] = {
>>   		.compatible = "mediatek,mt8196-power-controller",
>>   		.data = &mt8196_scpsys_data,
>>   	},
>> +	{
>> +		.compatible = "mediatek,mt8196-hfrp-power-controller",
>> +		.data = &mt8196_hfrpsys_data,
>> +	},
> 
> [Severity: High]
> This is a pre-existing issue, but is the platform driver missing the
> .sync_state callback required to clear the stay_on flag for default-ON
> domains?
> 
> Since the driver initializes default-on domains with is_off = false, the genpd
> core sets genpd->stay_on = true. Without a .sync_state callback or
> GENPD_FLAG_NO_STAY_ON, stay_on is never cleared, and the framework will refuse
> to power off unused domains.
> 
> When regulator_late_cleanup() later forces unused regulators off, could this
> silently cut physical power to domains the kernel still considers logically
> active and cause a hardware malfunction?
>