[PATCH] PM: clk: Report clock enable failures from pm_clk_resume()

[Abel Vesa]

pm_clk_resume() logs a clock that fails to start but always returns
success, so the runtime resume of a device using the PM clock helpers
succeeds without its clocks, and its driver goes on and accesses the
hardware.

This happens to the devices in the Qualcomm LPASS, whose clocks are
provided by the ADSP: after the ADSP is gone, like right after a
hibernation image is restored, a sound card opened by user space
resumes the LPASS LPI pin controller without clocks, and the access to
its registers then brings the whole machine down:

  qcom-prm gprsvc:service:2:2: CMD timeout for [100100f] opcode
  qcom-sm8550-lpass-lpi-pinctrl 6e80000.pinctrl: __pm_clk_enable: failed to enable clk 00000000b90a5151, error -110

Stop at the first clock that fails to start, stop the ones started
before it, and return the error, so that the resume fails and the
device stays suspended with its clocks off.

The callers of pm_clk_resume() check what it returns, including genpd
and pm_clk_runtime_resume(), except the MMP2 audio clock controller,
which keeps ignoring it. A device with a clock that fails to start no
longer resumes though, even if it used to work without that clock.

Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxxxxxxxx>
---
pm_clk_resume() always returns success, even when a clock fails to
start, so a device using the PM clock helpers resumes without its clocks
and its driver goes on accessing the hardware.

I hit this on the Qualcomm Hamoa CRD while working on hibernation: the LPASS
devices get their clocks from the ADSP, and when the ADSP is gone, like
right after a hibernation image is restored, a sound card opened by user
space resumes the LPASS LPI pin controller without clocks and the whole
machine resets on the first register access. With this patch, the
resume fails, the open fails, and the machine stays up until the ADSP is
restarted.

The LPASS macro drivers already check what pm_clk_resume() returns, but
never saw an error so far.

Prepared with help from an LLM-based assistant.
---
 drivers/base/power/clock_ops.c | 56 ++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 3b8ba27802a8..a978eb3d5479 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -143,8 +143,10 @@ static void pm_clk_op_unlock(struct pm_subsys_data *psd, unsigned long *flags)
  * __pm_clk_enable - Enable a clock, reporting any errors
  * @dev: The device for the given clock
  * @ce: PM clock entry corresponding to the clock.
+ *
+ * Return: 0 on success or the error from enabling the clock.
  */
-static inline void __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce)
+static inline int __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce)
 {
 	int ret;
 
@@ -156,13 +158,29 @@ static inline void __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce
 		ret = clk_enable(ce->clk);
 		break;
 	default:
-		return;
+		return 0;
 	}
 	if (!ret)
 		ce->status = PCE_STATUS_ENABLED;
 	else
 		dev_err(dev, "%s: failed to enable clk %p, error %d\n",
 			__func__, ce->clk, ret);
+
+	return ret;
+}
+
+static inline void __pm_clk_disable(struct pm_clock_entry *ce)
+{
+	if (ce->status != PCE_STATUS_ENABLED)
+		return;
+
+	if (ce->enabled_when_prepared) {
+		clk_disable_unprepare(ce->clk);
+		ce->status = PCE_STATUS_ACQUIRED;
+	} else {
+		clk_disable(ce->clk);
+		ce->status = PCE_STATUS_PREPARED;
+	}
 }
 
 /**
@@ -483,17 +501,8 @@ int pm_clk_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	list_for_each_entry_reverse(ce, &psd->clock_list, node) {
-		if (ce->status == PCE_STATUS_ENABLED) {
-			if (ce->enabled_when_prepared) {
-				clk_disable_unprepare(ce->clk);
-				ce->status = PCE_STATUS_ACQUIRED;
-			} else {
-				clk_disable(ce->clk);
-				ce->status = PCE_STATUS_PREPARED;
-			}
-		}
-	}
+	list_for_each_entry_reverse(ce, &psd->clock_list, node)
+		__pm_clk_disable(ce);
 
 	pm_clk_op_unlock(psd, &flags);
 
@@ -504,6 +513,9 @@ EXPORT_SYMBOL_GPL(pm_clk_suspend);
 /**
  * pm_clk_resume - Enable clocks in a device's PM clock list.
  * @dev: Device to enable the clocks for.
+ *
+ * Return: 0 on success or a negative error code. If a clock fails to start,
+ * the clocks started before it are stopped again.
  */
 int pm_clk_resume(struct device *dev)
 {
@@ -521,12 +533,24 @@ int pm_clk_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	list_for_each_entry(ce, &psd->clock_list, node)
-		__pm_clk_enable(dev, ce);
+	list_for_each_entry(ce, &psd->clock_list, node) {
+		ret = __pm_clk_enable(dev, ce);
+		if (ret)
+			break;
+	}
+
+	/*
+	 * A device whose clocks are not running cannot be accessed. Do not
+	 * pretend it resumed, and leave its clocks off as it stays suspended.
+	 */
+	if (ret) {
+		list_for_each_entry_continue_reverse(ce, &psd->clock_list, node)
+			__pm_clk_disable(ce);
+	}
 
 	pm_clk_op_unlock(psd, &flags);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(pm_clk_resume);
 

---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
change-id: 20261029-pm-clk-resume-report-errors-b5d538eb27de

Best regards,
--  

Abel