[PATCH 0/3] clk: qcom: gdsc: three pre-existing correctness fixes

[PATCH 0/3] clk: qcom: gdsc: three pre-existing correctness fixes

[Herman van Hazendonk]

While preparing MSM8x60 support for the GDSC core (a separate series,
adding LEGACY_FOOTSWITCH and RPM_ALWAYS_ON), code review surfaced
three pre-existing correctness issues in drivers/clk/qcom/gdsc.c that
are independent of any new SoC enabling work. This series collects
those three fixes so each can be reviewed and queued on its own
schedule without blocking the MSM8x60 series.

  1/3 - gdsc_poll_status: propagate gdsc_check_status() errno

      The polling helper treats any non-zero gdsc_check_status() return
      as "reached the target state". gdsc_check_status() returns < 0
      on regmap_read() failure (e.g. parent regmap unbinding, bus
      protection, debug-tool CSR removal). The boolean test promotes
      that negative errno to "true" and the poll returns success with
      the hardware state unknown -- silently misleading
      gdsc_toggle_logic() and the gdsc_init() sync path.

  2/3 - gdsc_init: propagate gdsc_enable() failure for ALWAYS_ON

      GENPD_FLAG_ALWAYS_ON requires the domain to be ON at
      pm_genpd_init() time. When the cold readback finds an ALWAYS_ON
      GDSC powered down, gdsc_init() tries to bring it back up but
      discards the gdsc_enable() return value. If the enable fails the
      code still sets on=true and registers the domain as ALWAYS_ON,
      so genpd reports "on" while the silicon is actually off and
      consumer probes hang on dead registers.

  3/3 - gdsc_unregister: tear down per-domain genpds + reorder

      gdsc_unregister() removes the OF provider entry and tears down
      the parent/subdomain wiring, but never calls pm_genpd_remove()
      on the individual generic_pm_domain structures registered by
      gdsc_init(). The dangling entries on gpd_list cause the next
      gdsc_init() to fail with -EEXIST on deferred-probe replay,
      module reload, or OF-overlay teardown. While here, of_genpd_-
      del_provider() is moved ahead of gdsc_pm_subdomain_remove() so
      that a racing of_genpd_get_from_provider() cannot attach to a
      domain that is mid-removal.

The three changes are independent and could in principle be sent as
separate one-off patches, but they share a single file and the cover
letter makes it easier to reason about them together. Apply order is
not significant: each patch stands on its own.

Built and boot-tested on an HP TouchPad (APQ8060) in conjunction with
the MSM8x60 GDSC enabling series.

Herman van Hazendonk (3):
  clk: qcom: gdsc: propagate gdsc_check_status() errors from
    gdsc_poll_status
  clk: qcom: gdsc: propagate gdsc_enable() failure for ALWAYS_ON domains
  clk: qcom: gdsc: tear down per-domain genpds in gdsc_unregister()

 drivers/clk/qcom/gdsc.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)


base-commit: 944125b4c454b58d2fe6e35f1087a932b2050dff
--
2.43.0

[PATCH 1/3] clk: qcom: gdsc: propagate gdsc_check_status() errors from gdsc_poll_status

[Herman van Hazendonk]

gdsc_check_status() returns negative errno when the underlying
regmap_read() fails -- e.g. when a parent regmap dies during system
suspend, a CSR is removed by an HW debug tool, or the bus controller
goes into protection. gdsc_poll_status() treats the result as a plain
boolean ("is the GDSC in the requested state?"), so any negative error
return is truncated to "true" and the poll exits with success even
though the rail's real state is unknown:

	do {
		if (gdsc_check_status(sc, status))
			return 0;
	} while (ktime_us_delta(ktime_get(), start) < STATUS_POLL_TIMEOUT_US);

	if (gdsc_check_status(sc, status))
		return 0;

	return -ETIMEDOUT;

This silently misleads gdsc_toggle_logic() (which writes/un-writes
SW_COLLAPSE on the strength of the poll succeeding) and the gdsc_init()
sync path (which assumes the readback represents real silicon state).

Latch the return value, propagate negative errno immediately, and only
treat a strictly-positive value as "reached the target state". Make the
same change in the post-timeout final check so a regmap that comes back
after the deadline does not silently degrade to -ETIMEDOUT.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/gdsc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 95aa07120245..b9b47f584f6d 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -103,14 +103,21 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
 static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
 {
 	ktime_t start;
+	int ret;
 
 	start = ktime_get();
 	do {
-		if (gdsc_check_status(sc, status))
+		ret = gdsc_check_status(sc, status);
+		if (ret < 0)
+			return ret;
+		if (ret)
 			return 0;
 	} while (ktime_us_delta(ktime_get(), start) < STATUS_POLL_TIMEOUT_US);
 
-	if (gdsc_check_status(sc, status))
+	ret = gdsc_check_status(sc, status);
+	if (ret < 0)
+		return ret;
+	if (ret)
 		return 0;
 
 	return -ETIMEDOUT;
-- 
2.43.0

[PATCH 2/3] clk: qcom: gdsc: propagate gdsc_enable() failure for ALWAYS_ON domains

[Herman van Hazendonk]

GENPD_FLAG_ALWAYS_ON requires the underlying domain to be on at
genpd_init() time -- the framework will refuse to register the domain
otherwise. When the cold readback in gdsc_init() finds an ALWAYS_ON
GDSC powered down, the driver tries to bring it back up:

	} else if (sc->flags & ALWAYS_ON) {
		/* If ALWAYS_ON GDSCs are not ON, turn them ON */
		gdsc_enable(&sc->pd);
		on = true;
	}

but discards the return value: if gdsc_enable() fails (regmap write
error, the long-form sequence's status poll times out, or the
HW_CTRL hand-off errors) the code still sets on=true and falls
through to pm_genpd_init(..., !on) -- which then registers the
domain in the ON state and sets GENPD_FLAG_ALWAYS_ON, even though
the silicon is actually off. Subsequent consumer probes will see
genpd report "on" while accessing dead registers and hang or read
garbage.

Catch the failure and surface it: returning the error from
gdsc_init() makes the provider probe fail with the underlying errno,
which propagates to consumers as -EPROBE_DEFER (or fatal if the
hardware really is broken) rather than silently lying about the
rail state.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/gdsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index b9b47f584f6d..a80a489763ed 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -481,7 +481,9 @@ static int gdsc_init(struct gdsc *sc)
 
 	} else if (sc->flags & ALWAYS_ON) {
 		/* If ALWAYS_ON GDSCs are not ON, turn them ON */
-		gdsc_enable(&sc->pd);
+		ret = gdsc_enable(&sc->pd);
+		if (ret)
+			return ret;
 		on = true;
 	}
 
-- 
2.43.0

[PATCH 3/3] clk: qcom: gdsc: tear down per-domain genpds in gdsc_unregister()

[Herman van Hazendonk]

gdsc_unregister() removes the OF provider entry and tears down the
parent/subdomain wiring, but never calls pm_genpd_remove() on the
individual generic_pm_domain structures registered by gdsc_init():

	void gdsc_unregister(struct gdsc_desc *desc)
	{
		struct device *dev = desc->dev;
		size_t num = desc->num;

		gdsc_pm_subdomain_remove(desc, num);
		of_genpd_del_provider(dev->of_node);
	}

That leaves dangling entries on the global gpd_list. After a provider
unbind/rebind cycle (deferred-probe replay during early boot, real
module unload of a clk driver that owns GDSCs, or an OF-overlay tear-
down) the next gdsc_init() will end up trying to re-register a name
that is still in the list and pm_genpd_init() returns -EEXIST.

While we are here, flip the order so the consumer-facing OF provider
entry is the first thing removed -- otherwise a fresh
of_genpd_get_from_provider() call racing with the teardown could
attach to a domain that is mid-removal.

Iterate the scs[] array and pm_genpd_remove() each registered domain
after the subdomain links are torn down. The regulators stay devm-
managed (devm_regulator_get_optional() in gdsc_register()), so the
release happens automatically when the underlying device is unbound;
just the genpd accounting needs to be undone explicitly.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/clk/qcom/gdsc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a80a489763ed..71826ccbd9bd 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -645,10 +645,18 @@ int gdsc_register(struct gdsc_desc *desc,
 void gdsc_unregister(struct gdsc_desc *desc)
 {
 	struct device *dev = desc->dev;
+	struct gdsc **scs = desc->scs;
 	size_t num = desc->num;
+	int i;
 
-	gdsc_pm_subdomain_remove(desc, num);
 	of_genpd_del_provider(dev->of_node);
+	gdsc_pm_subdomain_remove(desc, num);
+
+	for (i = 0; i < num; i++) {
+		if (!scs[i])
+			continue;
+		pm_genpd_remove(&scs[i]->pd);
+	}
 }
 
 /*
-- 
2.43.0