[PATCH v2 0/2] clk: qcom: fix error path and cleanup in gdsc support

[PATCH v2 0/2] clk: qcom: fix error path and cleanup in gdsc support

[Dmitry Baryshkov]

Fix error path in gdsc_register() and cleanup code in gdsc_unregister()
to properly unwind all genpd registration calls. Both patches bear two
fixes tags, since part of the issue was present in the very first commit
adding support for GDSCs, part of the issue was added when adding
hiearchical power domains support to GDSC code.

----------------------------------------------------------------
Dmitry Baryshkov (2):
      clk: qcom: fix error_path in gdsc_register
      clk: qcom: fix domains cleanup in gdsc_unregister

 drivers/clk/qcom/gdsc.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

[PATCH v2 1/2] clk: qcom: fix error_path in gdsc_register

[Dmitry Baryshkov]

Properly handle and cleanup errors in gdsc_register() instead of just
returning an error and leaving some of resources registered/hanging in
the system.

Fixes: c2c7f0a47493 ("clk: qcom: gdsc: Add support for hierarchical power domains")
Fixes: 45dd0e55317c ("clk: qcom: Add support for GDSCs")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 51ed640e527b..241186d9d08c 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -429,7 +429,7 @@ int gdsc_register(struct gdsc_desc *desc,
 		scs[i]->rcdev = rcdev;
 		ret = gdsc_init(scs[i]);
 		if (ret)
-			return ret;
+			goto err_init;
 		data->domains[i] = &scs[i]->pd;
 	}
 
@@ -437,11 +437,35 @@ int gdsc_register(struct gdsc_desc *desc,
 	for (i = 0; i < num; i++) {
 		if (!scs[i])
 			continue;
-		if (scs[i]->parent)
-			pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
+		if (scs[i]->parent) {
+			ret = pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
+			if (ret)
+				goto err_subdomain;
+		}
 	}
 
-	return of_genpd_add_provider_onecell(dev->of_node, data);
+	ret = of_genpd_add_provider_onecell(dev->of_node, data);
+	if (!ret)
+		return 0;
+
+err_subdomain:
+	i--;
+	for (; i >= 0; i--) {
+		if (!scs[i] || !scs[i]->parent)
+			continue;
+		pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
+	}
+	i = num;
+
+err_init:
+	i--;
+	for (; i >= 0; i--) {
+		if (!scs[i])
+			continue;
+		pm_genpd_remove(&scs[i]->pd);
+	}
+
+	return ret;
 }
 
 void gdsc_unregister(struct gdsc_desc *desc)
-- 
2.30.2

[PATCH v2 2/2] clk: qcom: fix domains cleanup in gdsc_unregister

[Dmitry Baryshkov]

Properly remove registered genpds. Also remove the provider before
breaking parent/child links, so that the system is consistent at remove
time.

Fixes: c2c7f0a47493 ("clk: qcom: gdsc: Add support for hierarchical power domains")
Fixes: 45dd0e55317c ("clk: qcom: Add support for GDSCs")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 241186d9d08c..9e16f234ce6c 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -475,14 +475,26 @@ void gdsc_unregister(struct gdsc_desc *desc)
 	struct gdsc **scs = desc->scs;
 	size_t num = desc->num;
 
-	/* Remove subdomains */
+	/*
+	 * Remove provider first so that we can remove the genpds without
+	 * worrying about consumers getting them during the removal process.
+	 */
+	of_genpd_del_provider(dev->of_node);
+
+	/* Break subdomain relationship */
 	for (i = 0; i < num; i++) {
 		if (!scs[i])
 			continue;
 		if (scs[i]->parent)
 			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
 	}
-	of_genpd_del_provider(dev->of_node);
+
+	/* And finally remove domains themselves */
+	for (i = 0; i < num; i++) {
+		if (!scs[i])
+			continue;
+		pm_genpd_remove(&scs[i]->pd);
+	}
 }
 
 /*
-- 
2.30.2

Re: [PATCH v2 0/2] clk: qcom: fix error path and cleanup in gdsc support

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-06-30 15:59:50)
> Fix error path in gdsc_register() and cleanup code in gdsc_unregister()
> to properly unwind all genpd registration calls. Both patches bear two
> fixes tags, since part of the issue was present in the very first commit
> adding support for GDSCs, part of the issue was added when adding
> hiearchical power domains support to GDSC code.

Should also include the author of the patch that is being Fixed as an
FYI.

> 
> ----------------------------------------------------------------
> Dmitry Baryshkov (2):
>       clk: qcom: fix error_path in gdsc_register
>       clk: qcom: fix domains cleanup in gdsc_unregister
> 
>  drivers/clk/qcom/gdsc.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
>

Re: [PATCH v2 0/2] clk: qcom: fix error path and cleanup in gdsc support

[Stephen Boyd]

Quoting Stephen Boyd (2021-06-30 17:41:36)
> Quoting Dmitry Baryshkov (2021-06-30 15:59:50)
> > Fix error path in gdsc_register() and cleanup code in gdsc_unregister()
> > to properly unwind all genpd registration calls. Both patches bear two
> > fixes tags, since part of the issue was present in the very first commit
> > adding support for GDSCs, part of the issue was added when adding
> > hiearchical power domains support to GDSC code.
> 
> Should also include the author of the patch that is being Fixed as an
> FYI.
> 

I'm inclined to drop this series from my clk review queue unless it is
still relevant. Is this still a problem after Bjorn's fix to this file?