[PATCH 0/3] pmdomain: rockchip: fix the probe error and deferral paths

[PATCH 0/3] pmdomain: rockchip: fix the probe error and deferral paths

[Arun Iyengar via B4 Relay]

Three fixes for what happens when a Rockchip power domain clock is not
available yet and rockchip_pm_domain_probe() defers:

1. (Igor Pecovnik) Only the clocks that were obtained are put; the
   ERR_PTR() of the failing one triggered the WARN_ON_ONCE() in
   __clk_put(), and the deferral reason was not recorded.
2. The error cleanup removes the parent/child links before the domains.
   pm_genpd_remove() refuses a domain that is still a parent, and on
   RK3588 (RKVDEC0/1 are children of both VDEC and VDPU) no ordering
   avoids that, so devm freed domains that were still on gpd_list.
3. A deferral is no longer printed as an error by the callers, once
   per probe attempt and twice for a nested domain.

Tested on an Orange Pi 5 (RK3588S) with test device trees that give a
power domain a clock which only appears about 26 s into boot, once on
PD_NPU (a top level domain) and once on PD_FEC (nested under PD_VI,
after 18 domains and their links were registered). With the series,
devices_deferred shows "failed to get clk at index 5", there is no
"failed to remove domain", list or slab corruption report (booted with
page_poison=1 init_on_free=1), no "-517" error lines, and the controller
binds later with each of the 26 domains listed once in
pm_genpd_summary. A host-side model of the probe and cleanup on the
board's DTB, deferring at each domain in turn, leaves freed domains on
gpd_list at 24 of 26 failure points without patch 2 and at none with
it. Normal boots are unchanged.

This series was developed with Claude (Anthropic) as a coding assistant:
it wrote the code of patches 2 and 3 and the commit messages, and I
reviewed and tested every change as described above. Patch 1 is Igor's
change with a rewritten commit message.

Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
Arun Iyengar (2):
      pmdomain: rockchip: remove subdomain links before the domains on error
      pmdomain: rockchip: do not report probe deferral of a domain as an error

Igor Pecovnik (1):
      pmdomain: rockchip: put only the clocks that were got on error

 drivers/pmdomain/rockchip/pm-domains.c | 73 +++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 11 deletions(-)
---
base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
change-id: 20261110-pmdomain-rockchip-3497a2e7fdf5

Best regards,
--  
Arun Iyengar <aiyengar1990@gmail.com>

[PATCH 1/3] pmdomain: rockchip: put only the clocks that were got on error

[Arun Iyengar via B4 Relay]

From: Igor Pecovnik <igor@armbian.com>

When of_clk_get() fails for clock i of a power domain,
rockchip_pm_add_one_domain() jumps to err_put_clocks, which calls
clk_bulk_put() on all pd->num_clks entries. That includes the
ERR_PTR() just stored in pd->clks[i] (clk_put() warns on it) and
the NULL entries after it (harmless, clk_put(NULL) is a no-op).

This is hit when a domain clock is not available yet and the probe is
deferred: the first deferral triggers the WARN_ON_ONCE() in __clk_put(),
and the failure is reported with dev_err() instead of being recorded as
the deferral reason shown in devices_deferred.

Put only the i clocks that were obtained, and report the failure with
dev_err_probe().

Fixes: d8b384777066 ("pmdomain: rockchip: fix clock leak on domain probe failure")
Cc: stable@vger.kernel.org
Link: https://github.com/armbian/build/commit/814ab46114eb18832e18d1f94f2d279680a308d7
Signed-off-by: Igor Pecovnik <igor@armbian.com>
[arun: rewrote the commit message for upstream]
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index 6eca21e0bb12..3b39b231ca0e 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -849,11 +849,13 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 	for (i = 0; i < pd->num_clks; i++) {
 		pd->clks[i].clk = of_clk_get(node, i);
 		if (IS_ERR(pd->clks[i].clk)) {
-			error = PTR_ERR(pd->clks[i].clk);
-			dev_err(pmu->dev,
-				"%pOFn: failed to get clk at index %d: %d\n",
-				node, i, error);
-			goto err_put_clocks;
+			error = dev_err_probe(pmu->dev,
+					      PTR_ERR(pd->clks[i].clk),
+					      "%pOFn: failed to get clk at index %d\n",
+					      node, i);
+			/* Only the clocks before index i were got */
+			clk_bulk_put(i, pd->clks);
+			return error;
 		}
 	}
 

-- 
2.55.0

[PATCH 2/3] pmdomain: rockchip: remove subdomain links before the domains on error

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

When rockchip_pm_domain_probe() fails after some domains were
registered, for example because a clock of a nested domain is not
available yet and returns -EPROBE_DEFER, rockchip_pm_domain_cleanup()
calls pm_genpd_remove() for every registered domain in domain id order.

pm_genpd_remove() refuses a domain that is still the parent of another
domain and returns -EBUSY. The cleanup walks parents before their
children in id order, and on RK3588 RKVDEC0/RKVDEC1 (ids 14 and 15) are
also children of VDPU (id 21), so no ordering of the domain array lets
every removal succeed. Each refused parent only gets the "state may be
inconsistent" message, and on return devm frees its struct
rockchip_pm_domain, the embedded generic_pm_domain included, while it
is still linked on the global gpd_list. The next registration, the
unused domain power off at late_initcall and the genpd debugfs files
then walk freed memory.

Remove the parent/child links first, walking the power domain nodes
depth first in the same way rockchip_pm_add_subdomain() added them, so
that each child no longer has subdomains of its own when its link is
removed. With no links left, and no provider or devices attached in the
probe error path, pm_genpd_remove() succeeds for every domain before
devm frees it. pm_genpd_remove_subdomain() also drops the parent's
subdomain count, which removing the child with pm_genpd_remove() alone
does not.

Fixes: 79d12b7a7f22 ("soc: rockchip: power-domain: use pm_genpd_remove in error cleanup")
Cc: stable@vger.kernel.org
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 46 ++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index 3b39b231ca0e..d7df19e10000 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -957,12 +957,58 @@ static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
 	/* devm will free our memory */
 }
 
+static struct generic_pm_domain *
+rockchip_pm_node_to_genpd(struct rockchip_pmu *pmu, struct device_node *node)
+{
+	u32 id;
+
+	if (of_property_read_u32(node, "reg", &id) ||
+	    id >= pmu->genpd_data.num_domains)
+		return NULL;
+
+	return pmu->genpd_data.domains[id];
+}
+
+/*
+ * Undo the pm_genpd_add_subdomain() calls of rockchip_pm_add_subdomain().
+ * Links are only ever added between a DT node and its children, so walking
+ * the tree depth first and removing the links of the children before the
+ * link to their parent always finds the subdomain without subdomains of its
+ * own, as pm_genpd_remove_subdomain() requires. This also covers domains with
+ * more than one parent (RKVDEC0/RKVDEC1 on RK3588), which a walk in domain id
+ * or registration order does not.
+ */
+static void rockchip_pm_remove_subdomains(struct rockchip_pmu *pmu,
+					  struct device_node *parent)
+{
+	struct generic_pm_domain *parent_domain, *child_domain;
+
+	parent_domain = rockchip_pm_node_to_genpd(pmu, parent);
+
+	for_each_child_of_node_scoped(parent, np) {
+		rockchip_pm_remove_subdomains(pmu, np);
+
+		child_domain = rockchip_pm_node_to_genpd(pmu, np);
+		if (parent_domain && child_domain)
+			pm_genpd_remove_subdomain(parent_domain, child_domain);
+	}
+}
+
 static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
 {
 	struct generic_pm_domain *genpd;
 	struct rockchip_pm_domain *pd;
 	int i;
 
+	/*
+	 * pm_genpd_remove() refuses a domain that still has subdomains, and
+	 * devm frees the domains on return whether or not they were removed,
+	 * which would leave freed memory on the global genpd list. Remove all
+	 * parent/child links first, so every domain can be removed.
+	 */
+	for_each_available_child_of_node_scoped(pmu->dev->of_node, np)
+		rockchip_pm_remove_subdomains(pmu, np);
+
 	for (i = 0; i < pmu->genpd_data.num_domains; i++) {
 		genpd = pmu->genpd_data.domains[i];
 		if (genpd) {

-- 
2.55.0

[PATCH 3/3] pmdomain: rockchip: do not report probe deferral of a domain as an error

[Arun Iyengar via B4 Relay]

From: Arun Iyengar <aiyengar1990@gmail.com>

When a power domain clock is not available yet,
rockchip_pm_add_one_domain() records the reason with dev_err_probe() and
returns -EPROBE_DEFER, but its callers rockchip_pm_domain_probe() and
rockchip_pm_add_subdomain() still print it with dev_err():

  rockchip-pm-domain ...: failed to handle node power-domain: -517

once per probe attempt, and twice for a nested domain. On an RK3588
board whose NPU domain clock appears late this is more than 20 error
lines before the domain finally probes.

Only rockchip_pm_add_one_domain() can return -EPROBE_DEFER here, and it
already records the deferral reason, so skip the message in the callers
for -EPROBE_DEFER and keep it for real errors. dev_err_probe() is not
used in the callers because it would replace the reason recorded for
/sys/kernel/debug/devices_deferred ("failed to get clk at index 1") with
the less specific "failed to handle node power-domain".

Fixes: 7c696693a4f5 ("soc: rockchip: power-domain: Add power domain driver")
Fixes: 6be05b5ec161 ("soc: rockchip: power-domain: add support for sub-power domains")
Signed-off-by: Arun Iyengar <aiyengar1990@gmail.com>
---
 drivers/pmdomain/rockchip/pm-domains.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/pmdomain/rockchip/pm-domains.c b/drivers/pmdomain/rockchip/pm-domains.c
index d7df19e10000..70ebe5696b39 100644
--- a/drivers/pmdomain/rockchip/pm-domains.c
+++ b/drivers/pmdomain/rockchip/pm-domains.c
@@ -1050,8 +1050,9 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 
 		error = rockchip_pm_add_one_domain(pmu, np);
 		if (error) {
-			dev_err(pmu->dev, "failed to handle node %pOFn: %d\n",
-				np, error);
+			if (error != -EPROBE_DEFER)
+				dev_err(pmu->dev, "failed to handle node %pOFn: %d\n",
+					np, error);
 			return error;
 		}
 
@@ -1144,15 +1145,17 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	for_each_available_child_of_node_scoped(np, node) {
 		error = rockchip_pm_add_one_domain(pmu, node);
 		if (error) {
-			dev_err(dev, "failed to handle node %pOFn: %d\n",
-				node, error);
+			if (error != -EPROBE_DEFER)
+				dev_err(dev, "failed to handle node %pOFn: %d\n",
+					node, error);
 			goto err_out;
 		}
 
 		error = rockchip_pm_add_subdomain(pmu, node);
 		if (error < 0) {
-			dev_err(dev, "failed to handle subdomain node %pOFn: %d\n",
-				node, error);
+			if (error != -EPROBE_DEFER)
+				dev_err(dev, "failed to handle subdomain node %pOFn: %d\n",
+					node, error);
 			goto err_out;
 		}
 	}

-- 
2.55.0