[PATCH 0/3] ICE: Enable runtime PM support

[PATCH 0/3] ICE: Enable runtime PM support

[Kuldeep Singh]

This series fixes ICE clock lifetime handling when ICE is modeled as a
standalone device and referenced by storage controllers through
qcom,ice.

Currently, ICE driver enables clocks during probe and keeps them
prepared after probe completes. This keeps the ICE clock voted even when
the storage controller is idle. Add runtime PM support to the ICE driver
so the standalone ICE device can enable clocks for probe-time register
access, then disable them and remain suspended while idle.

While enabling runtime PM in ice exposed couple of sequence bugs in
storage drivers(ufs and sdhci-msm) as any key programming call used to
go ice and never complaint as ice-clocks were always on.
Example, unclocked register access happened from ufs/mmc in
qcom_ice_program_key().

qcom_ice_writel(ice, 0x0, QCOM_ICE_REG_CRYPTOCFG(slot));

Fixing this issue require ice instance at early ufs/emmc probe so that
correct sequence is established and ice is ready to accept key program
calls from blk-crypto or any explicit storage operation.

Merge strategy:
Patch 1/3 and 2/3 are storage driver fixes and should be available
before patch 3/3 lands otherwise will result in bootup crash.

Validation:
Tested on shikra-evk, kodiak rb3-gen2 and sm8750-mtp.

Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
Kuldeep Singh (3):
      mmc: sdhci-msm: Enable ICE before enabling crypto support
      ufs: ufs-qcom: Enable ICE before enabling crypto support
      soc: qcom: ice: Add runtime PM support

 drivers/mmc/host/sdhci-msm.c |  21 ++-----
 drivers/soc/qcom/ice.c       | 139 ++++++++++++++++++++++++++++++++++++-------
 drivers/ufs/host/ufs-qcom.c  |  29 ++++++---
 3 files changed, 142 insertions(+), 47 deletions(-)
---
base-commit: 9a482184d910775cf648863148944945b74e2815
change-id: 20261022-qcom_ice_pm_enable-3c5d21b99e42

Best regards,
--  
Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>

[PATCH 1/3] mmc: sdhci-msm: Enable ICE before enabling crypto support

[Kuldeep Singh]

sdhci-msm currently exposes MMC crypto profile and sets MMC_CAP2_CRYPTO
before enabling ICE. This leaves a window where blk-crypto can issue
keyslot programming calls before sdhci-msm has explicitly enabled ICE.

This works today because ICE clocks remain enabled after ICE probe, but
the storage consumer should not rely on that implicit clock state. Once
ICE supports runtime PM and clocks are suspended after probe, accessing
ICE registers before qcom_ice_enable() call can fail due to unclocked
register access.

Enable ICE during the sdhci-msm crypto early init path which ensures ICE
is ready before blk-crypto can issue key programming calls.

Remove ICE enable call from CQE path, since ICE is now enabled
during SDHCI crypto initialization only.

Fixes: c7eed31e235c ("mmc: sdhci-msm: Switch to the new ICE API")
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
 drivers/mmc/host/sdhci-msm.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index aecd425bf095..85065cfae132 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1928,6 +1928,12 @@ static int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
 
 	msm_host->ice = ice;
 
+	err = qcom_ice_enable(msm_host->ice);
+	if (err) {
+		dev_err(dev, "%s: failed to enable ICE: %d\n", __func__, err);
+		return err;
+	}
+
 	/* Initialize the blk_crypto_profile */
 
 	caps.reg_val = cpu_to_le32(cqhci_readl(cq_host, CQHCI_CCAP));
@@ -1963,12 +1969,6 @@ static int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
 	return 0;
 }
 
-static void sdhci_msm_ice_enable(struct sdhci_msm_host *msm_host)
-{
-	if (msm_host->mmc->caps2 & MMC_CAP2_CRYPTO)
-		qcom_ice_enable(msm_host->ice);
-}
-
 static int sdhci_msm_ice_resume(struct sdhci_msm_host *msm_host)
 {
 	if (msm_host->mmc->caps2 & MMC_CAP2_CRYPTO)
@@ -2141,10 +2141,6 @@ static inline int sdhci_msm_ice_init(struct sdhci_msm_host *msm_host,
 	return 0;
 }
 
-static inline void sdhci_msm_ice_enable(struct sdhci_msm_host *msm_host)
-{
-}
-
 static inline int
 sdhci_msm_ice_resume(struct sdhci_msm_host *msm_host)
 {
@@ -2182,12 +2178,7 @@ static u32 sdhci_msm_cqe_irq(struct sdhci_host *host, u32 intmask)
 
 static void sdhci_msm_cqe_enable(struct mmc_host *mmc)
 {
-	struct sdhci_host *host = mmc_priv(mmc);
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
-
 	sdhci_cqe_enable(mmc);
-	sdhci_msm_ice_enable(msm_host);
 }
 
 static void sdhci_msm_cqe_disable(struct mmc_host *mmc, bool recovery)

-- 
2.34.1

[PATCH 2/3] ufs: ufs-qcom: Enable ICE before enabling crypto support

[Kuldeep Singh]

ufs-qcom currently exposes UFS crypto profile and sets UFSHCD_CAP_CRYPTO
before enabling ICE. This leaves a window where blk-crypto can issue
keyslot programming calls before ufs-qcom has explicitly enabled ICE.

Similar to MMC, UFS also works today as ICE clocks remain enabled after
the probe, but it should not rely on that implicit clock state. Once ICE
supports runtime PM, ICE clocks can be suspended after probe, and
accessing ICE registers before qcom_ice_enable() call can fail due to
unclocked register access in qcom_ice_program_key().

Enable ICE during ufs crypto init path to ensure ICE is ready before
blk-crypto can issue key programming calls.

Also call qcom_ice_enable() from HCE PRE_CHANGE path once crypto support
has been enabled. UFS controller reset can reset ICE state, and the UFS
core can reprogram keys during HCE start before POST_CHANGE.
Re-enabling ICE in PRE_CHANGE makes ICE ready before reset-time key
reprogramming and for any later ICE operation.

Fixes: 56541c7c4468 ("scsi: ufs: ufs-qcom: Switch to the new ICE API")
Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
 drivers/ufs/host/ufs-qcom.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index fee9b54ba5e1..7e5aa4573e0c 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -158,13 +158,15 @@ static void ufs_qcom_config_ice_allocator(struct ufs_qcom_host *host)
 	ufshcd_writel(hba, config, REG_UFS_MEM_ICE_NUM_CORE);
 }
 
-static inline void ufs_qcom_ice_enable(struct ufs_qcom_host *host)
+static const struct blk_crypto_ll_ops ufs_qcom_crypto_ops; /* forward decl */
+
+static inline int ufs_qcom_ice_enable(struct ufs_qcom_host *host)
 {
 	if (host->hba->caps & UFSHCD_CAP_CRYPTO)
-		qcom_ice_enable(host->ice);
-}
+		return qcom_ice_enable(host->ice);
 
-static const struct blk_crypto_ll_ops ufs_qcom_crypto_ops; /* forward decl */
+	return 0;
+}
 
 static int ufs_qcom_ice_init(struct ufs_qcom_host *host)
 {
@@ -188,6 +190,12 @@ static int ufs_qcom_ice_init(struct ufs_qcom_host *host)
 
 	host->ice = ice;
 
+	err = qcom_ice_enable(host->ice);
+	if (err) {
+		dev_err(dev, "Failed to enable ICE: %d\n", err);
+		return err;
+	}
+
 	/* Initialize the blk_crypto_profile */
 
 	caps.reg_val = cpu_to_le32(ufshcd_readl(hba, REG_UFS_CCAP));
@@ -318,11 +326,12 @@ static const struct blk_crypto_ll_ops ufs_qcom_crypto_ops = {
 
 #else
 
-static inline void ufs_qcom_ice_enable(struct ufs_qcom_host *host)
+static int ufs_qcom_ice_init(struct ufs_qcom_host *host)
 {
+	return 0;
 }
 
-static int ufs_qcom_ice_init(struct ufs_qcom_host *host)
+static inline int ufs_qcom_ice_enable(struct ufs_qcom_host *host)
 {
 	return 0;
 }
@@ -636,12 +645,15 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
 		 * is initialized.
 		 */
 		err = ufs_qcom_enable_lane_clks(host);
+		if (err)
+			return err;
+
+		err = ufs_qcom_ice_enable(host);
 		break;
 	case POST_CHANGE:
 		/* check if UFS PHY moved from DISABLED to HIBERN8 */
 		err = ufs_qcom_check_hibern8(hba);
 		ufs_qcom_enable_hw_clk_gating(hba);
-		ufs_qcom_ice_enable(host);
 		ufs_qcom_config_ice_allocator(host);
 		break;
 	default:
@@ -660,10 +672,9 @@ static int ufs_qcom_fw_managed_hce_enable_notify(struct ufs_hba *hba,
 	switch (status) {
 	case PRE_CHANGE:
 		ufs_qcom_select_unipro_mode(host);
-		break;
+		return ufs_qcom_ice_enable(host);
 	case POST_CHANGE:
 		ufs_qcom_enable_hw_clk_gating(hba);
-		ufs_qcom_ice_enable(host);
 		break;
 	default:
 		dev_err(hba->dev, "Invalid status %d\n", status);

-- 
2.34.1

[PATCH 3/3] soc: qcom: ice: Add runtime PM support

[Kuldeep Singh]

Ice driver currently enables its clocks during probe and keeps them
prepared/enabled even after probe is complete. On targets where ICE
exists as separate DT node and storage consumers reference it through
"qcom,ice", leaves ICE clock voted while the system is idle.

To mitigate this, add runtime PM callbacks to gate the core and iface
clocks. During probe, enable the clocks to read the ICE version/fuse
state, and then disable clocks and mark runtime PM state as suspended.

For legacy binding, keep clock ownership with the storage consumer
dev as it created the ICE instance.

Signed-off-by: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>
---
 drivers/soc/qcom/ice.c | 139 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 116 insertions(+), 23 deletions(-)

diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index e256595b9826..f9939e2d037c 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/xarray.h>
 
 #include <linux/firmware/qcom/qcom_scm.h>
@@ -113,6 +114,7 @@ struct qcom_ice {
 	bool use_hwkm;
 	bool hwkm_init_complete;
 	u8 hwkm_version;
+	bool use_pm_runtime;
 };
 
 static DEFINE_XARRAY(ice_handles);
@@ -303,42 +305,99 @@ static void qcom_ice_hwkm_init(struct qcom_ice *ice)
 
 int qcom_ice_enable(struct qcom_ice *ice)
 {
+	bool pm_acquired = false;
+	int err;
+
+	if (ice->use_pm_runtime && pm_runtime_suspended(ice->dev)) {
+		err = pm_runtime_resume_and_get(ice->dev);
+		if (err) {
+			dev_err(ice->dev, "runtime resume failed: %d\n", err);
+			return err;
+		}
+		pm_acquired = true;
+	}
+
 	qcom_ice_low_power_mode_enable(ice);
 	qcom_ice_optimization_enable(ice);
 	qcom_ice_hwkm_init(ice);
-	return qcom_ice_wait_bist_status(ice);
+
+	err = qcom_ice_wait_bist_status(ice);
+	if (err && pm_acquired) {
+		dev_err(ice->dev, "BIST failed, ret: %d\n", err);
+		pm_runtime_put_sync_suspend(ice->dev);
+	}
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(qcom_ice_enable);
 
-int qcom_ice_resume(struct qcom_ice *ice)
+static int qcom_ice_enable_clocks(struct qcom_ice *ice)
 {
-	struct device *dev = ice->dev;
 	int err;
 
 	err = clk_prepare_enable(ice->core_clk);
-	if (err) {
-		dev_err(dev, "Failed to enable core clock: %d\n", err);
+	if (err)
 		return err;
-	}
 
 	err = clk_prepare_enable(ice->iface_clk);
 	if (err) {
-		dev_err(dev, "Failed to enable iface clock: %d\n", err);
+		clk_disable_unprepare(ice->core_clk);
 		return err;
 	}
-	qcom_ice_hwkm_init(ice);
-	return qcom_ice_wait_bist_status(ice);
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(qcom_ice_resume);
 
-int qcom_ice_suspend(struct qcom_ice *ice)
+static void qcom_ice_disable_clocks(struct qcom_ice *ice)
 {
 	clk_disable_unprepare(ice->iface_clk);
 	clk_disable_unprepare(ice->core_clk);
+}
+
+static int qcom_ice_do_resume(struct qcom_ice *ice)
+{
+	int err = 0;
+
+	err = qcom_ice_enable_clocks(ice);
+	if (err)
+		return err;
+
+	qcom_ice_hwkm_init(ice);
+
+	err = qcom_ice_wait_bist_status(ice);
+	if (err) {
+		dev_err(ice->dev, "Bist failed, ret: %d\n", err);
+		qcom_ice_disable_clocks(ice);
+		ice->hwkm_init_complete = false;
+	}
+
+	return err;
+}
+
+int qcom_ice_resume(struct qcom_ice *ice)
+{
+	if (ice->use_pm_runtime)
+		return pm_runtime_resume_and_get(ice->dev);
+
+	return qcom_ice_do_resume(ice);
+}
+EXPORT_SYMBOL_GPL(qcom_ice_resume);
+
+static int qcom_ice_do_suspend(struct qcom_ice *ice)
+{
 	ice->hwkm_init_complete = false;
+	qcom_ice_disable_clocks(ice);
 
 	return 0;
 }
+
+int qcom_ice_suspend(struct qcom_ice *ice)
+{
+	if (ice->use_pm_runtime)
+		return pm_runtime_put_sync_suspend(ice->dev);
+
+	return qcom_ice_do_suspend(ice);
+}
 EXPORT_SYMBOL_GPL(qcom_ice_suspend);
 
 static unsigned int translate_hwkm_slot(struct qcom_ice *ice, unsigned int slot)
@@ -560,10 +619,11 @@ int qcom_ice_import_key(struct qcom_ice *ice,
 }
 EXPORT_SYMBOL_GPL(qcom_ice_import_key);
 
-static struct qcom_ice *qcom_ice_create(struct device *dev,
-					void __iomem *base)
+static struct qcom_ice *qcom_ice_create(struct device *dev, void __iomem *base,
+					bool use_pm_runtime)
 {
 	struct qcom_ice *engine;
+	int err;
 
 	if (!qcom_scm_is_available())
 		return ERR_PTR(-EPROBE_DEFER);
@@ -579,36 +639,54 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
 
 	engine->dev = dev;
 	engine->base = base;
+	engine->use_pm_runtime = use_pm_runtime;
 
 	/*
 	 * Legacy DT binding uses different clk names for each consumer,
 	 * so lets try those first. If none of those are a match, it means
 	 * the we only have one clock and it is part of the dedicated DT node.
-	 * Also, enable the clock before we check what HW version the driver
-	 * supports.
 	 */
-	engine->core_clk = devm_clk_get_optional_enabled(dev, "ice_core_clk");
+	engine->core_clk = devm_clk_get_optional(dev, "ice_core_clk");
 	if (!engine->core_clk)
-		engine->core_clk = devm_clk_get_optional_enabled(dev, "ice");
+		engine->core_clk = devm_clk_get_optional(dev, "ice");
 	if (!engine->core_clk)
-		engine->core_clk = devm_clk_get_optional_enabled(dev, "core");
+		engine->core_clk = devm_clk_get_optional(dev, "core");
 	if (!engine->core_clk)
-		engine->core_clk = devm_clk_get_enabled(dev, NULL);
+		engine->core_clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(engine->core_clk))
 		return ERR_CAST(engine->core_clk);
 
-	engine->iface_clk = devm_clk_get_optional_enabled(dev, "iface");
+	engine->iface_clk = devm_clk_get_optional(dev, "iface");
 	if (IS_ERR(engine->iface_clk))
 		return ERR_CAST(engine->iface_clk);
 
-	if (!qcom_ice_check_supported(engine))
+	err = qcom_ice_enable_clocks(engine);
+	if (err)
+		return ERR_PTR(err);
+
+	if (!qcom_ice_check_supported(engine)) {
+		qcom_ice_disable_clocks(engine);
 		return ERR_PTR(-EOPNOTSUPP);
+	}
+
+	if (use_pm_runtime)
+		qcom_ice_disable_clocks(engine);
 
 	dev_dbg(dev, "Registered Qualcomm Inline Crypto Engine\n");
 
 	return engine;
 }
 
+static int qcom_ice_runtime_resume(struct device *dev)
+{
+	return qcom_ice_do_resume(dev_get_drvdata(dev));
+}
+
+static int qcom_ice_runtime_suspend(struct device *dev)
+{
+	return qcom_ice_do_suspend(dev_get_drvdata(dev));
+}
+
 /**
  * of_qcom_ice_get() - get an ICE instance from a DT node
  * @dev: device pointer for the consumer device
@@ -645,7 +723,7 @@ static struct qcom_ice *of_qcom_ice_get(struct device *dev)
 			return ERR_CAST(base);
 
 		/* create ICE instance using consumer dev */
-		return qcom_ice_create(&pdev->dev, base);
+		return qcom_ice_create(&pdev->dev, base, false);
 	}
 
 	guard(mutex)(&ice_mutex);
@@ -738,6 +816,7 @@ static int qcom_ice_probe(struct platform_device *pdev)
 	unsigned long phandle = pdev->dev.of_node->phandle;
 	struct qcom_ice *engine;
 	void __iomem *base;
+	int err;
 
 	guard(mutex)(&ice_mutex);
 
@@ -749,13 +828,21 @@ static int qcom_ice_probe(struct platform_device *pdev)
 		return PTR_ERR(base);
 	}
 
-	engine = qcom_ice_create(&pdev->dev, base);
+	engine = qcom_ice_create(&pdev->dev, base, true);
 	if (IS_ERR(engine)) {
 		/* Store the error pointer for devm_of_qcom_ice_get() */
 		xa_store(&ice_handles, phandle, engine, GFP_KERNEL);
 		return PTR_ERR(engine);
 	}
 
+	platform_set_drvdata(pdev, engine);
+	pm_runtime_set_suspended(&pdev->dev);
+
+	err = devm_pm_runtime_enable(&pdev->dev);
+	if (err) {
+		xa_store(&ice_handles, phandle, ERR_PTR(err), GFP_KERNEL);
+		return err;
+	}
 	xa_store(&ice_handles, phandle, engine, GFP_KERNEL);
 
 	return 0;
@@ -775,12 +862,18 @@ static const struct of_device_id qcom_ice_of_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
 
+static const struct dev_pm_ops qcom_ice_pm_ops = {
+	RUNTIME_PM_OPS(qcom_ice_runtime_suspend, qcom_ice_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
 static struct platform_driver qcom_ice_driver = {
 	.probe	= qcom_ice_probe,
 	.remove	= qcom_ice_remove,
 	.driver = {
 		.name = "qcom-ice",
 		.of_match_table = qcom_ice_of_match_table,
+		.pm = pm_ptr(&qcom_ice_pm_ops),
 	},
 };
 

-- 
2.34.1

Re: [PATCH 3/3] soc: qcom: ice: Add runtime PM support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] soc: qcom: ice: unclocked MMIO access in qcom_ice_enable() when !CONFIG_PM
- [High] soc: qcom: ice: unbalanced runtime PM refcount in qcom_ice_enable()
- [High] soc: qcom: ice: clock and runtime PM leak on probe failure or unbind
- [High] soc: qcom: ice: broken pm_runtime return handling in qcom_ice_suspend()
- [High] soc: qcom: ice: skipped HWKM re-init and unclocked restore across system PM
--

commit e58cd3bb281ecab50bf58fdb5388a26380d5ef70
Author: Kuldeep Singh <kuldeep.singh@oss.qualcomm.com>

soc: qcom: ice: Add runtime PM support

This commit adds runtime PM support to the Qualcomm Inline Crypto Engine
(ICE) driver so that the core and iface clocks are gated when the device is
idle on platforms with a dedicated ICE devicetree node. For legacy bindings
where the ICE instance is created using the storage consumer device, manual
clock control is retained.

> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index e256595b9826f..f9939e2d037c7 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c

[ ... ]

> @@ -303,42 +305,99 @@ static void qcom_ice_hwkm_init(struct qcom_ice *ice)
>  
>  int qcom_ice_enable(struct qcom_ice *ice)
>  {
> +	bool pm_acquired = false;
> +	int err;
> +
> +	if (ice->use_pm_runtime && pm_runtime_suspended(ice->dev)) {
> +		err = pm_runtime_resume_and_get(ice->dev);
> +		if (err) {
> +			dev_err(ice->dev, "runtime resume failed: %d\n", err);
> +			return err;
> +		}
> +		pm_acquired = true;
> +	}
> +
>  	qcom_ice_low_power_mode_enable(ice);
>  	qcom_ice_optimization_enable(ice);
>  	qcom_ice_hwkm_init(ice);
> -	return qcom_ice_wait_bist_status(ice);
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err && pm_acquired) {
> +		dev_err(ice->dev, "BIST failed, ret: %d\n", err);
> +		pm_runtime_put_sync_suspend(ice->dev);
> +	}
> +
> +	return err;
>  }

[Severity: High]
In qcom_ice_enable() in drivers/soc/qcom/ice.c, pm_runtime_resume_and_get()
is gated on an unlocked check of pm_runtime_suspended(ice->dev):

include/linux/pm_runtime.h:pm_runtime_suspended() {
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

Because devm_pm_runtime_enable(&pdev->dev) is called in qcom_ice_probe()
before consumers attach, ice->dev may already be in RPM_ACTIVE (for example,
if userspace writes "on" to power/control via pm_runtime_forbid(), or after
an earlier consumer call to qcom_ice_enable()) or in RPM_SUSPENDING when
qcom_ice_enable() runs.

When pm_runtime_suspended(ice->dev) returns false:
- qcom_ice_enable() skips pm_runtime_resume_and_get(ice->dev) and does not
  acquire a runtime PM usage reference for the consumer.
- If ice->dev is concurrently in RPM_SUSPENDING, qcom_ice_enable() accesses
  registers in qcom_ice_low_power_mode_enable(),
  qcom_ice_optimization_enable(), qcom_ice_hwkm_init(), and
  qcom_ice_wait_bist_status() while qcom_ice_runtime_suspend() disables
  core_clk and iface_clk.
- If power/control is later set back to "auto" (pm_runtime_allow()),
  ice->dev can suspend and gate its clocks while the storage consumer is
  still active.
- When the consumer later calls qcom_ice_suspend() ->
  pm_runtime_put_sync_suspend(ice->dev), it decrements
  ice->dev->power.usage_count without a matching reference from
  qcom_ice_enable().

Additionally, when ice->dev is RPM_SUSPENDED on the initial call to
qcom_ice_enable(), pm_runtime_resume_and_get(ice->dev) invokes
qcom_ice_runtime_resume() -> qcom_ice_do_resume(), which runs
qcom_ice_hwkm_init() and qcom_ice_wait_bist_status() before
qcom_ice_low_power_mode_enable() and qcom_ice_optimization_enable(), and
then qcom_ice_enable() runs qcom_ice_hwkm_init() and
qcom_ice_wait_bist_status() a second time.

Can qcom_ice_enable() leave the runtime PM usage count unbalanced or race
with a concurrent runtime suspend when ice->dev is not in RPM_SUSPENDED?

>  EXPORT_SYMBOL_GPL(qcom_ice_enable);
>  
> -int qcom_ice_resume(struct qcom_ice *ice)
> +static int qcom_ice_enable_clocks(struct qcom_ice *ice)
>  {
> -	struct device *dev = ice->dev;
>  	int err;
>  
>  	err = clk_prepare_enable(ice->core_clk);
> -	if (err) {
> -		dev_err(dev, "Failed to enable core clock: %d\n", err);
> +	if (err)
>  		return err;
> -	}
>  
>  	err = clk_prepare_enable(ice->iface_clk);
>  	if (err) {
> -		dev_err(dev, "Failed to enable iface clock: %d\n", err);
> +		clk_disable_unprepare(ice->core_clk);
>  		return err;
>  	}
> -	qcom_ice_hwkm_init(ice);
> -	return qcom_ice_wait_bist_status(ice);
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL_GPL(qcom_ice_resume);
>  
> -int qcom_ice_suspend(struct qcom_ice *ice)
> +static void qcom_ice_disable_clocks(struct qcom_ice *ice)
>  {
>  	clk_disable_unprepare(ice->iface_clk);
>  	clk_disable_unprepare(ice->core_clk);
> +}
> +
> +static int qcom_ice_do_resume(struct qcom_ice *ice)
> +{
> +	int err = 0;
> +
> +	err = qcom_ice_enable_clocks(ice);
> +	if (err)
> +		return err;
> +
> +	qcom_ice_hwkm_init(ice);
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err) {
> +		dev_err(ice->dev, "Bist failed, ret: %d\n", err);
> +		qcom_ice_disable_clocks(ice);
> +		ice->hwkm_init_complete = false;
> +	}
> +
> +	return err;
> +}
> +
> +int qcom_ice_resume(struct qcom_ice *ice)
> +{
> +	if (ice->use_pm_runtime)
> +		return pm_runtime_resume_and_get(ice->dev);
> +
> +	return qcom_ice_do_resume(ice);
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_resume);
> +
> +static int qcom_ice_do_suspend(struct qcom_ice *ice)
> +{
>  	ice->hwkm_init_complete = false;
> +	qcom_ice_disable_clocks(ice);
>  
>  	return 0;
>  }
> +
> +int qcom_ice_suspend(struct qcom_ice *ice)
> +{
> +	if (ice->use_pm_runtime)
> +		return pm_runtime_put_sync_suspend(ice->dev);
> +
> +	return qcom_ice_do_suspend(ice);
> +}
>  EXPORT_SYMBOL_GPL(qcom_ice_suspend);

[Severity: High]
In qcom_ice_suspend() in drivers/soc/qcom/ice.c, the return value of
pm_runtime_put_sync_suspend(ice->dev) is returned directly to the caller,
whereas previously qcom_ice_suspend() always returned 0.

pm_runtime_put_sync_suspend() calls __pm_runtime_suspend(ice->dev,
RPM_GET_PUT) in drivers/base/power/runtime.c, which decrements
dev->power.usage_count first and can return:
- 1 if the device was already in RPM_SUSPENDED,
- -EAGAIN if another caller concurrently incremented usage_count or a resume
  is pending,
- -EBUSY if child_count is non-zero or a PM domain governor rejects suspend,
- -EPERM if a 0 us PM QoS resume latency constraint is active.

In all of these cases, ice->dev->power.usage_count has already been
decremented, but both storage callers treat any non-zero return value as a
suspend failure:

1. In drivers/mmc/host/sdhci-msm.c, sdhci_msm_runtime_suspend() disables
   msm_host->bulk_clks before calling sdhci_msm_ice_suspend():

   drivers/mmc/host/sdhci-msm.c:sdhci_msm_runtime_suspend() {
       ...
       dev_pm_opp_set_rate(dev, 0);
       clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
                                  msm_host->bulk_clks);

       return sdhci_msm_ice_suspend(msm_host);
   }

   A non-zero return causes runtime suspend to fail (and a return value of 1
   sets dev->power.runtime_error = 1 in rpm_suspend()) with SDHCI clocks left
   disabled and ice->dev's usage_count already dropped.

2. In drivers/ufs/core/ufshcd.c, __ufshcd_wl_suspend() treats a non-zero
   return from ufshcd_vops_suspend() -> ufs_qcom_suspend() ->
   qcom_ice_suspend() as an error:

   drivers/ufs/core/ufshcd.c:__ufshcd_wl_suspend() {
       ...
       ret = ufshcd_vops_suspend(hba, pm_op, POST_CHANGE);
       if (ret)
           goto set_link_active;
       ...
   }

   This aborts UFS suspend without calling qcom_ice_resume() to restore the
   dropped ICE usage_count reference, causing the next suspend to underflow
   ice->dev->power.usage_count. Similarly, ufshcd_wl_poweroff() calls
   __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM) even when UFS is already
   runtime-suspended, calling qcom_ice_suspend() a second time.

Will returning the raw result of pm_runtime_put_sync_suspend() from
qcom_ice_suspend() cause suspend regressions in sdhci-msm and ufs-qcom?

> @@ -560,10 +619,11 @@ int qcom_ice_import_key(struct qcom_ice *ice,
>  }
>  EXPORT_SYMBOL_GPL(qcom_ice_import_key);
>  
> -static struct qcom_ice *qcom_ice_create(struct device *dev,
> -					void __iomem *base)
> +static struct qcom_ice *qcom_ice_create(struct device *dev, void __iomem *base,
> +					bool use_pm_runtime)
>  {
>  	struct qcom_ice *engine;
> +	int err;
>  
>  	if (!qcom_scm_is_available())
>  		return ERR_PTR(-EPROBE_DEFER);
> @@ -579,36 +639,54 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>  
>  	engine->dev = dev;
>  	engine->base = base;
> +	engine->use_pm_runtime = use_pm_runtime;
>  
>  	/*
>  	 * Legacy DT binding uses different clk names for each consumer,
>  	 * so lets try those first. If none of those are a match, it means
>  	 * the we only have one clock and it is part of the dedicated DT node.
> -	 * Also, enable the clock before we check what HW version the driver
> -	 * supports.
>  	 */
> -	engine->core_clk = devm_clk_get_optional_enabled(dev, "ice_core_clk");
> +	engine->core_clk = devm_clk_get_optional(dev, "ice_core_clk");
>  	if (!engine->core_clk)
> -		engine->core_clk = devm_clk_get_optional_enabled(dev, "ice");
> +		engine->core_clk = devm_clk_get_optional(dev, "ice");
>  	if (!engine->core_clk)
> -		engine->core_clk = devm_clk_get_optional_enabled(dev, "core");
> +		engine->core_clk = devm_clk_get_optional(dev, "core");
>  	if (!engine->core_clk)
> -		engine->core_clk = devm_clk_get_enabled(dev, NULL);
> +		engine->core_clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(engine->core_clk))
>  		return ERR_CAST(engine->core_clk);
>  
> -	engine->iface_clk = devm_clk_get_optional_enabled(dev, "iface");
> +	engine->iface_clk = devm_clk_get_optional(dev, "iface");
>  	if (IS_ERR(engine->iface_clk))
>  		return ERR_CAST(engine->iface_clk);
>  
> -	if (!qcom_ice_check_supported(engine))
> +	err = qcom_ice_enable_clocks(engine);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	if (!qcom_ice_check_supported(engine)) {
> +		qcom_ice_disable_clocks(engine);
>  		return ERR_PTR(-EOPNOTSUPP);
> +	}

[Severity: High]
Previously, qcom_ice_create() in drivers/soc/qcom/ice.c used
devm_clk_get_optional_enabled() and devm_clk_get_enabled(), which registered
clk_disable_unprepare() with devres for teardown. Replacing them with
devm_clk_get_optional() and devm_clk_get() followed by
qcom_ice_enable_clocks(engine) leaves no devres action to disable or
unprepare engine->core_clk and engine->iface_clk:

1. For the legacy DT binding (use_pm_runtime == false), of_qcom_ice_get() ->
   qcom_ice_create(&pdev->dev, base, false) leaves core_clk and iface_clk
   prepared and enabled, and qcom_ice_put() does not disable them:

   drivers/soc/qcom/ice.c:qcom_ice_put() {
       struct platform_device *pdev = to_platform_device(ice->dev);

       if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "ice"))
           platform_device_put(pdev);
   }

   If the consumer fails probe after devm_of_qcom_ice_get() or is unbound
   (where ufshcd_remove() and sdhci_msm_remove() resume the host before
   teardown without calling qcom_ice_suspend()), devm_clk_release() calls
   clk_put() while core_clk and iface_clk remain prepared and enabled.

2. For the dedicated DT binding (use_pm_runtime == true), qcom_ice_enable()
   acquires a runtime PM reference on ice->dev via
   pm_runtime_resume_and_get(ice->dev) and enables core_clk and iface_clk.
   If a subsequent step in consumer probe fails (or in ufs-qcom if the
   second qcom_ice_enable() call in ufs_qcom_hce_enable_notify() fails BIST
   when pm_acquired is false) or the consumer is unbound, neither
   qcom_ice_put() nor the consumer remove callbacks release the runtime PM
   reference on ice->dev. Moreover, if the qcom-ice driver itself is unbound
   while active in qcom_ice_remove(), devm_pm_runtime_enable() only calls
   pm_runtime_disable() without running qcom_ice_runtime_suspend().

Does this leak the prepared/enabled core_clk and iface_clk clocks and the
ice->dev runtime PM reference on probe failure or driver unbind?

> +
> +	if (use_pm_runtime)
> +		qcom_ice_disable_clocks(engine);

[Severity: High]
CONFIG_QCOM_INLINE_CRYPTO_ENGINE does not depend on CONFIG_PM, and
qcom_ice_driver uses pm_ptr(&qcom_ice_pm_ops) so it can be built with
CONFIG_PM=n.

In qcom_ice_probe() -> qcom_ice_create(&pdev->dev, base, true),
use_pm_runtime is unconditionally set to true and qcom_ice_disable_clocks()
disables engine->core_clk and engine->iface_clk before returning.

When CONFIG_PM=n:
- pm_runtime_set_suspended() and devm_pm_runtime_enable() in
  qcom_ice_probe() are no-op stubs.
- In qcom_ice_enable(), pm_runtime_suspended(ice->dev) is a stub that always
  returns false, so qcom_ice_enable() never resumes the device or enables
  clocks before calling qcom_ice_low_power_mode_enable(),
  qcom_ice_optimization_enable(), qcom_ice_hwkm_init(), and
  qcom_ice_wait_bist_status(), performing MMIO reads and writes to ICE
  registers while core_clk and iface_clk are disabled.
- In qcom_ice_resume(), pm_runtime_resume_and_get(ice->dev) returns 0
  without invoking qcom_ice_runtime_resume(), leaving clocks disabled and
  skipping qcom_ice_do_resume().
- In qcom_ice_suspend(), pm_runtime_put_sync_suspend(ice->dev) calls the
  __pm_runtime_suspend() stub, which returns -ENOSYS.

What happens when a storage consumer calls qcom_ice_enable(),
qcom_ice_resume(), or qcom_ice_suspend() on a kernel built with CONFIG_PM=n?

[ ... ]

> @@ -775,12 +862,18 @@ static const struct of_device_id qcom_ice_of_match_table[] = {
>  };
>  MODULE_DEVICE_TABLE(of, qcom_ice_of_match_table);
>  
> +static const struct dev_pm_ops qcom_ice_pm_ops = {
> +	RUNTIME_PM_OPS(qcom_ice_runtime_suspend, qcom_ice_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};

[Severity: High]
With SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
in qcom_ice_pm_ops and qcom_ice_resume() delegating to
pm_runtime_resume_and_get(ice->dev), two system PM state mismatches can
occur when ice->use_pm_runtime is true:

1. During dpm_prepare(), device_prepare() calls
   pm_runtime_get_noresume(ice->dev), raising ice->dev->power.usage_count
   from 1 to 2 while the storage consumer is active. When the consumer
   suspends in dpm_suspend() and calls qcom_ice_suspend() ->
   pm_runtime_put_sync_suspend(ice->dev), __pm_runtime_suspend() drops
   usage_count from 2 to 1 and returns 0 without calling
   qcom_ice_runtime_suspend(), leaving ice->dev in RPM_ACTIVE.

   If system suspend aborts before pm_runtime_force_suspend(ice->dev) runs
   (or if userspace sets ice->dev's power/control to "on"), ice->dev remains
   in RPM_ACTIVE. When dpm_resume() runs ufshcd_wl_resume() ->
   ufs_qcom_resume() in drivers/ufs/host/ufs-qcom.c, UFS controller v5.0.0
   issues a synchronous hardware reset to ICE (UFS_ICE_SYNC_RST_SW in
   UFS_MEM_ICE_CFG) and then calls qcom_ice_resume(host->ice) ->
   pm_runtime_resume_and_get(ice->dev). Because ice->dev is still
   RPM_ACTIVE, pm_runtime_resume_and_get() returns 0 without running
   qcom_ice_do_resume(), skipping qcom_ice_hwkm_init() and
   qcom_ice_wait_bist_status() after the hardware reset.

2. During hibernation freeze/poweroff, after sdhci_msm_runtime_suspend()
   calls qcom_ice_suspend() -> pm_runtime_put_sync_suspend(ice->dev),
   ice->dev->power.usage_count is 1 (only the device_prepare() reference).
   When pm_runtime_force_suspend(ice->dev) runs,
   pm_runtime_need_not_resume(ice->dev) is true (usage_count <= 1), so it
   calls qcom_ice_do_suspend() (setting ice->hwkm_init_complete = false and
   disabling clocks) and sets needs_force_resume = false.

   During hibernation restore, pm_runtime_force_resume(ice->dev) leaves
   ice->dev in RPM_SUSPENDED, and sdhci_msm_restore() in
   drivers/mmc/host/sdhci-msm.c then calls sdhci_msm_ice_restore() ->
   blk_crypto_reprogram_all_keys() -> qcom_ice_program_key() while ice->dev
   is still suspended with clocks disabled and ice->hwkm_init_complete set
   to false.

Could system suspend recovery and hibernation restore leave ICE uninitialized
or unclocked when qcom_ice_resume() and qcom_ice_pm_ops rely on runtime PM
status transitions?

> +
>  static struct platform_driver qcom_ice_driver = {
>  	.probe	= qcom_ice_probe,
>  	.remove	= qcom_ice_remove,
>  	.driver = {
>  		.name = "qcom-ice",
>  		.of_match_table = qcom_ice_of_match_table,
> +		.pm = pm_ptr(&qcom_ice_pm_ops),
>  	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-qcom_ice_pm_enable-v1-0-f7a84086c651@oss.qualcomm.com?part=3