[PATCH 0/4] firmware: qcom: scm: simplify code by using classes

[PATCH 0/4] firmware: qcom: scm: simplify code by using classes

[Bartosz Golaszewski]

There's a pattern of enabling the clocks and/or setting the bandwith
limits before performing an SCM call and reverting the above after it's
node. In all places it's done unconditionally and can be simplified by
providing appropriate classes. There are also a few places where we can
use __free() for tzmem pointers.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Bartosz Golaszewski (4):
      firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler
      firmware: qcom: scm: use __free(qcom_tzmem) to simplify cleanup
      firmware: qcom: scm: introduce qcom_scm_clk class for clock management
      firmware: qcom: scm: introduce qcom_scm_bw class for bandwidth management

 drivers/firmware/qcom/qcom_scm.c         | 184 ++++++++++++-------------------
 include/linux/firmware/qcom/qcom_tzmem.h |   2 +-
 2 files changed, 73 insertions(+), 113 deletions(-)
---
base-commit: be5c93fa674f0fc3c8f359c2143abce6bbb422e6
change-id: 20260701-qcom-scm-code-shrink-c9aff845537d

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

[PATCH 1/4] firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler

[Bartosz Golaszewski]

We currently only silently skip NULL-pointers in the cleanup handler for
tzmem. It's possible that we get passed a pointer holding an ERR_PTR()
value so skip it too.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 include/linux/firmware/qcom/qcom_tzmem.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h
index 23173e0c3dddd154dd56dc3dcb56bd20ada0520a..b5520178bf6f53b86b530571a3be9f302225f022 100644
--- a/include/linux/firmware/qcom/qcom_tzmem.h
+++ b/include/linux/firmware/qcom/qcom_tzmem.h
@@ -58,7 +58,7 @@ devm_qcom_tzmem_pool_new(struct device *dev,
 void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp);
 void qcom_tzmem_free(void *ptr);
 
-DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T))
+DEFINE_FREE(qcom_tzmem, void *, if (!IS_ERR_OR_NULL(_T)) qcom_tzmem_free(_T))
 
 phys_addr_t qcom_tzmem_to_phys(void *ptr);
 

-- 
2.47.3

[PATCH 2/4] firmware: qcom: scm: use __free(qcom_tzmem) to simplify cleanup

[Bartosz Golaszewski]

Use the __free(qcom_tzmem) cleanup attribute (together with no_free_ptr()
whenever ownership is transferred) to replace open-coded
qcom_tzmem_free() calls and their associated goto labels.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c | 48 +++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 6b601a4b89dbf04322730aa22fc9590d180a383c..1684220617748e4890ecb45a1787a6798dafd5cc 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -640,10 +640,9 @@ static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_context *ctx,
 {
 	struct qcom_scm_res res;
 	phys_addr_t mdata_phys;
-	void *mdata_buf;
 	int ret;
 
-	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
+	void *mdata_buf __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
 	if (!mdata_buf)
 		return -ENOMEM;
 
@@ -652,11 +651,10 @@ static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_context *ctx,
 
 	ret = __qcom_scm_pas_init_image(ctx->pas_id, mdata_phys, &res);
 	if (ret < 0)
-		qcom_tzmem_free(mdata_buf);
-	else
-		ctx->ptr = mdata_buf;
+		return ret;
 
-	return ret ? : res.result[0];
+	ctx->ptr = no_free_ptr(mdata_buf);
+	return res.result[0];
 }
 
 /**
@@ -791,10 +789,11 @@ static void *__qcom_scm_pas_get_rsc_table(u32 pas_id, void *input_rt_tzm,
 		.owner = ARM_SMCCC_OWNER_SIP,
 	};
 	struct qcom_scm_res res;
-	void *output_rt_tzm;
 	int ret;
 
-	output_rt_tzm = qcom_tzmem_alloc(__scm->mempool, *output_rt_size, GFP_KERNEL);
+	void *output_rt_tzm __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
+								   *output_rt_size,
+								   GFP_KERNEL);
 	if (!output_rt_tzm)
 		return ERR_PTR(-ENOMEM);
 
@@ -814,20 +813,17 @@ static void *__qcom_scm_pas_get_rsc_table(u32 pas_id, void *input_rt_tzm,
 	 * be of unresonable size.
 	 */
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
-	if (!ret && res.result[2] > SZ_1G) {
-		ret = -E2BIG;
-		goto free_output_rt;
-	}
+	if (!ret && res.result[2] > SZ_1G)
+		return ERR_PTR(-E2BIG);
 
 	*output_rt_size = res.result[2];
 	if (ret && res.result[1] == RSCTABLE_BUFFER_NOT_SUFFICIENT)
-		ret = -EOVERFLOW;
+		return ERR_PTR(-EOVERFLOW);
 
-free_output_rt:
 	if (ret)
-		qcom_tzmem_free(output_rt_tzm);
+		return ERR_PTR(ret);
 
-	return ret ? ERR_PTR(ret) : output_rt_tzm;
+	return no_free_ptr(output_rt_tzm);
 }
 
 /**
@@ -878,8 +874,6 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 {
 	struct resource_table empty_rsc = {};
 	size_t size = SZ_16K;
-	void *output_rt_tzm;
-	void *input_rt_tzm;
 	void *tbl_ptr;
 	int ret;
 
@@ -901,7 +895,9 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 		input_rt_size = sizeof(empty_rsc);
 	}
 
-	input_rt_tzm = qcom_tzmem_alloc(__scm->mempool, input_rt_size, GFP_KERNEL);
+	void *input_rt_tzm __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
+								  input_rt_size,
+								  GFP_KERNEL);
 	if (!input_rt_tzm) {
 		ret = -ENOMEM;
 		goto disable_scm_bw;
@@ -909,8 +905,9 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 
 	memcpy(input_rt_tzm, input_rt, input_rt_size);
 
-	output_rt_tzm = __qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt_tzm,
-						     input_rt_size, &size);
+	void *output_rt_tzm __free(qcom_tzmem) =
+		__qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt_tzm,
+					     input_rt_size, &size);
 	if (PTR_ERR(output_rt_tzm) == -EOVERFLOW)
 		/* Try again with the size requested by the TZ */
 		output_rt_tzm = __qcom_scm_pas_get_rsc_table(ctx->pas_id,
@@ -919,21 +916,16 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 							     &size);
 	if (IS_ERR(output_rt_tzm)) {
 		ret = PTR_ERR(output_rt_tzm);
-		goto free_input_rt;
+		goto disable_scm_bw;
 	}
 
 	tbl_ptr = kmemdup(output_rt_tzm, size, GFP_KERNEL);
 	if (!tbl_ptr) {
-		qcom_tzmem_free(output_rt_tzm);
 		ret = -ENOMEM;
-		goto free_input_rt;
+		goto disable_scm_bw;
 	}
 
 	*output_rt_size = size;
-	qcom_tzmem_free(output_rt_tzm);
-
-free_input_rt:
-	qcom_tzmem_free(input_rt_tzm);
 
 disable_scm_bw:
 	qcom_scm_bw_disable();

-- 
2.47.3

[PATCH 3/4] firmware: qcom: scm: introduce qcom_scm_clk class for clock management

[Bartosz Golaszewski]

Define DEFINE_CLASS(qcom_scm_clk) that calls qcom_scm_clk_enable() on
construction and automatically calls qcom_scm_clk_disable() at scope exit
*if* the enable succeeded.

This allows us to convert all call sites to using
CLASS(qcom_scm_clk, clk)() instead of the manual enable/check/disable
pattern and to remove the associated goto labels.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c | 89 +++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 55 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 1684220617748e4890ecb45a1787a6798dafd5cc..35aa9e8886b6ce8ab8eaf16c83fef7aafaef2822 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -192,6 +192,9 @@ static void qcom_scm_clk_disable(void)
 	clk_disable_unprepare(__scm->bus_clk);
 }
 
+DEFINE_CLASS(qcom_scm_clk, int, if (!_T) qcom_scm_clk_disable(),
+	     qcom_scm_clk_enable(), void)
+
 static int qcom_scm_bw_enable(void)
 {
 	int ret = 0;
@@ -511,13 +514,11 @@ static int qcom_scm_disable_sdi(void)
 	};
 	struct qcom_scm_res res;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return clk;
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 
-	qcom_scm_clk_disable();
-
 	return ret ? : res.result[0];
 }
 
@@ -616,22 +617,19 @@ static int __qcom_scm_pas_init_image(u32 pas_id, dma_addr_t mdata_phys,
 	};
 	int ret;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return clk;
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	desc.args[1] = mdata_phys;
 
 	ret = qcom_scm_call(__scm->dev, &desc, res);
 	qcom_scm_bw_disable();
 
-disable_clk:
-	qcom_scm_clk_disable();
-
 	return ret;
 }
 
@@ -758,20 +756,17 @@ int qcom_scm_pas_mem_setup(u32 pas_id, phys_addr_t addr, phys_addr_t size)
 	};
 	struct qcom_scm_res res;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return clk;
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
 
-disable_clk:
-	qcom_scm_clk_disable();
-
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL_GPL(qcom_scm_pas_mem_setup);
@@ -877,13 +872,13 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 	void *tbl_ptr;
 	int ret;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ERR_PTR(ret);
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return ERR_PTR(clk);
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		goto disable_clk;
+		return ERR_PTR(ret);
 
 	/*
 	 * TrustZone can not accept buffer as NULL value as argument hence,
@@ -930,9 +925,6 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 disable_scm_bw:
 	qcom_scm_bw_disable();
 
-disable_clk:
-	qcom_scm_clk_disable();
-
 	return ret ? ERR_PTR(ret) : tbl_ptr;
 }
 EXPORT_SYMBOL_GPL(qcom_scm_pas_get_rsc_table);
@@ -956,20 +948,17 @@ int qcom_scm_pas_auth_and_reset(u32 pas_id)
 	};
 	struct qcom_scm_res res;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return clk;
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
 
-disable_clk:
-	qcom_scm_clk_disable();
-
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL_GPL(qcom_scm_pas_auth_and_reset);
@@ -1039,20 +1028,17 @@ int qcom_scm_pas_shutdown(u32 pas_id)
 	};
 	struct qcom_scm_res res;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return clk;
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
-		goto disable_clk;
+		return ret;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
 
-disable_clk:
-	qcom_scm_clk_disable();
-
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL_GPL(qcom_scm_pas_shutdown);
@@ -1791,18 +1777,13 @@ EXPORT_SYMBOL_GPL(qcom_scm_import_ice_key);
  */
 bool qcom_scm_hdcp_available(void)
 {
-	bool avail;
-	int ret = qcom_scm_clk_enable();
+	CLASS(qcom_scm_clk, clk)();
 
-	if (ret)
-		return ret;
-
-	avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
-						QCOM_SCM_HDCP_INVOKE);
-
-	qcom_scm_clk_disable();
+	if (clk)
+		return false;
 
-	return avail;
+	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
+					    QCOM_SCM_HDCP_INVOKE);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_hdcp_available);
 
@@ -1840,15 +1821,13 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
 	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
 		return -ERANGE;
 
-	ret = qcom_scm_clk_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_clk, clk)();
+	if (clk)
+		return clk;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	*resp = res.result[0];
 
-	qcom_scm_clk_disable();
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_scm_hdcp_req);

-- 
2.47.3

[PATCH 4/4] firmware: qcom: scm: introduce qcom_scm_bw class for bandwidth management

[Bartosz Golaszewski]

Define DEFINE_CLASS(qcom_scm_bw) that calls qcom_scm_bw_enable() on
construction and automatically calls qcom_scm_bw_disable() at scope exit
*if* the enable succeeded.

This allows us to convert all call sites to using
CLASS(qcom_scm_bw, bw)() instead of the manual enable/check/disable
pattern and to remove the associated goto labels in cleanup path.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c | 61 ++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 35aa9e8886b6ce8ab8eaf16c83fef7aafaef2822..6b64ed5a2b70a5ad3e57efb780df7dfa43518f91 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -227,6 +227,9 @@ static void qcom_scm_bw_disable(void)
 	mutex_unlock(&__scm->scm_bw_lock);
 }
 
+DEFINE_CLASS(qcom_scm_bw, int, if (!_T) qcom_scm_bw_disable(),
+	     qcom_scm_bw_enable(), void)
+
 enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
 static DEFINE_SPINLOCK(scm_query_lock);
 
@@ -621,14 +624,13 @@ static int __qcom_scm_pas_init_image(u32 pas_id, dma_addr_t mdata_phys,
 	if (clk)
 		return clk;
 
-	ret = qcom_scm_bw_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_bw, bw)();
+	if (bw)
+		return bw;
 
 	desc.args[1] = mdata_phys;
 
 	ret = qcom_scm_call(__scm->dev, &desc, res);
-	qcom_scm_bw_disable();
 
 	return ret;
 }
@@ -760,12 +762,11 @@ int qcom_scm_pas_mem_setup(u32 pas_id, phys_addr_t addr, phys_addr_t size)
 	if (clk)
 		return clk;
 
-	ret = qcom_scm_bw_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_bw, bw)();
+	if (bw)
+		return bw;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
-	qcom_scm_bw_disable();
 
 	return ret ? : res.result[0];
 }
@@ -870,15 +871,14 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 	struct resource_table empty_rsc = {};
 	size_t size = SZ_16K;
 	void *tbl_ptr;
-	int ret;
 
 	CLASS(qcom_scm_clk, clk)();
 	if (clk)
 		return ERR_PTR(clk);
 
-	ret = qcom_scm_bw_enable();
-	if (ret)
-		return ERR_PTR(ret);
+	CLASS(qcom_scm_bw, bw)();
+	if (bw)
+		return ERR_PTR(bw);
 
 	/*
 	 * TrustZone can not accept buffer as NULL value as argument hence,
@@ -893,10 +893,8 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 	void *input_rt_tzm __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
 								  input_rt_size,
 								  GFP_KERNEL);
-	if (!input_rt_tzm) {
-		ret = -ENOMEM;
-		goto disable_scm_bw;
-	}
+	if (!input_rt_tzm)
+		return ERR_PTR(-ENOMEM);
 
 	memcpy(input_rt_tzm, input_rt, input_rt_size);
 
@@ -909,23 +907,16 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
 							     input_rt_tzm,
 							     input_rt_size,
 							     &size);
-	if (IS_ERR(output_rt_tzm)) {
-		ret = PTR_ERR(output_rt_tzm);
-		goto disable_scm_bw;
-	}
+	if (IS_ERR(output_rt_tzm))
+		return output_rt_tzm;
 
 	tbl_ptr = kmemdup(output_rt_tzm, size, GFP_KERNEL);
-	if (!tbl_ptr) {
-		ret = -ENOMEM;
-		goto disable_scm_bw;
-	}
+	if (!tbl_ptr)
+		return ERR_PTR(-ENOMEM);
 
 	*output_rt_size = size;
 
-disable_scm_bw:
-	qcom_scm_bw_disable();
-
-	return ret ? ERR_PTR(ret) : tbl_ptr;
+	return tbl_ptr;
 }
 EXPORT_SYMBOL_GPL(qcom_scm_pas_get_rsc_table);
 
@@ -952,12 +943,11 @@ int qcom_scm_pas_auth_and_reset(u32 pas_id)
 	if (clk)
 		return clk;
 
-	ret = qcom_scm_bw_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_bw, bw)();
+	if (bw)
+		return bw;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
-	qcom_scm_bw_disable();
 
 	return ret ? : res.result[0];
 }
@@ -1032,12 +1022,11 @@ int qcom_scm_pas_shutdown(u32 pas_id)
 	if (clk)
 		return clk;
 
-	ret = qcom_scm_bw_enable();
-	if (ret)
-		return ret;
+	CLASS(qcom_scm_bw, bw)();
+	if (bw)
+		return bw;
 
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
-	qcom_scm_bw_disable();
 
 	return ret ? : res.result[0];
 }

-- 
2.47.3

Re: [PATCH 1/4] firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler

[Mukesh Ojha]

On Wed, Jul 01, 2026 at 03:38:55PM +0200, Bartosz Golaszewski wrote:
> We currently only silently skip NULL-pointers in the cleanup handler for
> tzmem. It's possible that we get passed a pointer holding an ERR_PTR()
> value so skip it too.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  include/linux/firmware/qcom/qcom_tzmem.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h
> index 23173e0c3dddd154dd56dc3dcb56bd20ada0520a..b5520178bf6f53b86b530571a3be9f302225f022 100644
> --- a/include/linux/firmware/qcom/qcom_tzmem.h
> +++ b/include/linux/firmware/qcom/qcom_tzmem.h
> @@ -58,7 +58,7 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>  void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp);
>  void qcom_tzmem_free(void *ptr);
>  
> -DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T))
> +DEFINE_FREE(qcom_tzmem, void *, if (!IS_ERR_OR_NULL(_T)) qcom_tzmem_free(_T))

Is this really required? Are we trying to address an issue which
does not exist today?

>  
>  phys_addr_t qcom_tzmem_to_phys(void *ptr);
>  
> 
> -- 
> 2.47.3
> 

-- 
-Mukesh Ojha

Re: [PATCH 2/4] firmware: qcom: scm: use __free(qcom_tzmem) to simplify cleanup

[Mukesh Ojha]

On Wed, Jul 01, 2026 at 03:38:56PM +0200, Bartosz Golaszewski wrote:
> Use the __free(qcom_tzmem) cleanup attribute (together with no_free_ptr()
> whenever ownership is transferred) to replace open-coded
> qcom_tzmem_free() calls and their associated goto labels.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 48 +++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 6b601a4b89dbf04322730aa22fc9590d180a383c..1684220617748e4890ecb45a1787a6798dafd5cc 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -640,10 +640,9 @@ static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_context *ctx,
>  {
>  	struct qcom_scm_res res;
>  	phys_addr_t mdata_phys;
> -	void *mdata_buf;
>  	int ret;
>  
> -	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
> +	void *mdata_buf __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
>  	if (!mdata_buf)
>  		return -ENOMEM;
>  
> @@ -652,11 +651,10 @@ static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_context *ctx,
>  
>  	ret = __qcom_scm_pas_init_image(ctx->pas_id, mdata_phys, &res);
>  	if (ret < 0)
> -		qcom_tzmem_free(mdata_buf);
> -	else
> -		ctx->ptr = mdata_buf;
> +		return ret;
>  
> -	return ret ? : res.result[0];
> +	ctx->ptr = no_free_ptr(mdata_buf);
> +	return res.result[0];
>  }
>  
>  /**
> @@ -791,10 +789,11 @@ static void *__qcom_scm_pas_get_rsc_table(u32 pas_id, void *input_rt_tzm,
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
>  	struct qcom_scm_res res;
> -	void *output_rt_tzm;
>  	int ret;
>  
> -	output_rt_tzm = qcom_tzmem_alloc(__scm->mempool, *output_rt_size, GFP_KERNEL);
> +	void *output_rt_tzm __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
> +								   *output_rt_size,
> +								   GFP_KERNEL);
>  	if (!output_rt_tzm)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -814,20 +813,17 @@ static void *__qcom_scm_pas_get_rsc_table(u32 pas_id, void *input_rt_tzm,
>  	 * be of unresonable size.
>  	 */
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
> -	if (!ret && res.result[2] > SZ_1G) {
> -		ret = -E2BIG;
> -		goto free_output_rt;
> -	}
> +	if (!ret && res.result[2] > SZ_1G)
> +		return ERR_PTR(-E2BIG);
>  
>  	*output_rt_size = res.result[2];
>  	if (ret && res.result[1] == RSCTABLE_BUFFER_NOT_SUFFICIENT)
> -		ret = -EOVERFLOW;
> +		return ERR_PTR(-EOVERFLOW);
>  
> -free_output_rt:
>  	if (ret)
> -		qcom_tzmem_free(output_rt_tzm);
> +		return ERR_PTR(ret);
>  
> -	return ret ? ERR_PTR(ret) : output_rt_tzm;
> +	return no_free_ptr(output_rt_tzm);
>  }
>  
>  /**
> @@ -878,8 +874,6 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  {
>  	struct resource_table empty_rsc = {};
>  	size_t size = SZ_16K;
> -	void *output_rt_tzm;
> -	void *input_rt_tzm;
>  	void *tbl_ptr;
>  	int ret;
>  
> @@ -901,7 +895,9 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  		input_rt_size = sizeof(empty_rsc);
>  	}
>  
> -	input_rt_tzm = qcom_tzmem_alloc(__scm->mempool, input_rt_size, GFP_KERNEL);
> +	void *input_rt_tzm __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
> +								  input_rt_size,
> +								  GFP_KERNEL);
>  	if (!input_rt_tzm) {
>  		ret = -ENOMEM;
>  		goto disable_scm_bw;
> @@ -909,8 +905,9 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  
>  	memcpy(input_rt_tzm, input_rt, input_rt_size);
>  
> -	output_rt_tzm = __qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt_tzm,
> -						     input_rt_size, &size);
> +	void *output_rt_tzm __free(qcom_tzmem) =
> +		__qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt_tzm,
> +					     input_rt_size, &size);
>  	if (PTR_ERR(output_rt_tzm) == -EOVERFLOW)
>  		/* Try again with the size requested by the TZ */
>  		output_rt_tzm = __qcom_scm_pas_get_rsc_table(ctx->pas_id,
> @@ -919,21 +916,16 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  							     &size);
>  	if (IS_ERR(output_rt_tzm)) {
>  		ret = PTR_ERR(output_rt_tzm);
> -		goto free_input_rt;
> +		goto disable_scm_bw;
>  	}
>  
>  	tbl_ptr = kmemdup(output_rt_tzm, size, GFP_KERNEL);
>  	if (!tbl_ptr) {
> -		qcom_tzmem_free(output_rt_tzm);
>  		ret = -ENOMEM;
> -		goto free_input_rt;
> +		goto disable_scm_bw;
>  	}
>  
>  	*output_rt_size = size;
> -	qcom_tzmem_free(output_rt_tzm);
> -
> -free_input_rt:
> -	qcom_tzmem_free(input_rt_tzm);
>  
>  disable_scm_bw:
>  	qcom_scm_bw_disable();
> 
> -- 
> 2.47.3
> 

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

-- 
-Mukesh Ojha

Re: [PATCH 4/4] firmware: qcom: scm: introduce qcom_scm_bw class for bandwidth management

[Mukesh Ojha]

On Wed, Jul 01, 2026 at 03:38:58PM +0200, Bartosz Golaszewski wrote:
> Define DEFINE_CLASS(qcom_scm_bw) that calls qcom_scm_bw_enable() on
> construction and automatically calls qcom_scm_bw_disable() at scope exit
> *if* the enable succeeded.
> 
> This allows us to convert all call sites to using
> CLASS(qcom_scm_bw, bw)() instead of the manual enable/check/disable
> pattern and to remove the associated goto labels in cleanup path.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 61 ++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 35aa9e8886b6ce8ab8eaf16c83fef7aafaef2822..6b64ed5a2b70a5ad3e57efb780df7dfa43518f91 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -227,6 +227,9 @@ static void qcom_scm_bw_disable(void)
>  	mutex_unlock(&__scm->scm_bw_lock);
>  }
>  
> +DEFINE_CLASS(qcom_scm_bw, int, if (!_T) qcom_scm_bw_disable(),
> +	     qcom_scm_bw_enable(), void)
> +
>  enum qcom_scm_convention qcom_scm_convention = SMC_CONVENTION_UNKNOWN;
>  static DEFINE_SPINLOCK(scm_query_lock);
>  
> @@ -621,14 +624,13 @@ static int __qcom_scm_pas_init_image(u32 pas_id, dma_addr_t mdata_phys,
>  	if (clk)
>  		return clk;
>  
> -	ret = qcom_scm_bw_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_bw, bw)();
> +	if (bw)
> +		return bw;
>  
>  	desc.args[1] = mdata_phys;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, res);
> -	qcom_scm_bw_disable();
>  
>  	return ret;
>  }
> @@ -760,12 +762,11 @@ int qcom_scm_pas_mem_setup(u32 pas_id, phys_addr_t addr, phys_addr_t size)
>  	if (clk)
>  		return clk;
>  
> -	ret = qcom_scm_bw_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_bw, bw)();
> +	if (bw)
> +		return bw;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
> -	qcom_scm_bw_disable();
>  
>  	return ret ? : res.result[0];
>  }
> @@ -870,15 +871,14 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  	struct resource_table empty_rsc = {};
>  	size_t size = SZ_16K;
>  	void *tbl_ptr;
> -	int ret;
>  
>  	CLASS(qcom_scm_clk, clk)();
>  	if (clk)
>  		return ERR_PTR(clk);
>  
> -	ret = qcom_scm_bw_enable();
> -	if (ret)
> -		return ERR_PTR(ret);
> +	CLASS(qcom_scm_bw, bw)();
> +	if (bw)
> +		return ERR_PTR(bw);
>  
>  	/*
>  	 * TrustZone can not accept buffer as NULL value as argument hence,
> @@ -893,10 +893,8 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  	void *input_rt_tzm __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
>  								  input_rt_size,
>  								  GFP_KERNEL);
> -	if (!input_rt_tzm) {
> -		ret = -ENOMEM;
> -		goto disable_scm_bw;
> -	}
> +	if (!input_rt_tzm)
> +		return ERR_PTR(-ENOMEM);
>  
>  	memcpy(input_rt_tzm, input_rt, input_rt_size);
>  
> @@ -909,23 +907,16 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  							     input_rt_tzm,
>  							     input_rt_size,
>  							     &size);
> -	if (IS_ERR(output_rt_tzm)) {
> -		ret = PTR_ERR(output_rt_tzm);
> -		goto disable_scm_bw;
> -	}
> +	if (IS_ERR(output_rt_tzm))
> +		return output_rt_tzm;
>  
>  	tbl_ptr = kmemdup(output_rt_tzm, size, GFP_KERNEL);
> -	if (!tbl_ptr) {
> -		ret = -ENOMEM;
> -		goto disable_scm_bw;
> -	}
> +	if (!tbl_ptr)
> +		return ERR_PTR(-ENOMEM);
>  
>  	*output_rt_size = size;
>  
> -disable_scm_bw:
> -	qcom_scm_bw_disable();
> -
> -	return ret ? ERR_PTR(ret) : tbl_ptr;
> +	return tbl_ptr;
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_pas_get_rsc_table);
>  
> @@ -952,12 +943,11 @@ int qcom_scm_pas_auth_and_reset(u32 pas_id)
>  	if (clk)
>  		return clk;
>  
> -	ret = qcom_scm_bw_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_bw, bw)();
> +	if (bw)
> +		return bw;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
> -	qcom_scm_bw_disable();
>  
>  	return ret ? : res.result[0];
>  }
> @@ -1032,12 +1022,11 @@ int qcom_scm_pas_shutdown(u32 pas_id)
>  	if (clk)
>  		return clk;
>  
> -	ret = qcom_scm_bw_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_bw, bw)();
> +	if (bw)
> +		return bw;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
> -	qcom_scm_bw_disable();
>  
>  	return ret ? : res.result[0];
>  }

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

-- 
-Mukesh Ojha

Re: [PATCH 3/4] firmware: qcom: scm: introduce qcom_scm_clk class for clock management

[Mukesh Ojha]

On Wed, Jul 01, 2026 at 03:38:57PM +0200, Bartosz Golaszewski wrote:
> Define DEFINE_CLASS(qcom_scm_clk) that calls qcom_scm_clk_enable() on
> construction and automatically calls qcom_scm_clk_disable() at scope exit
> *if* the enable succeeded.
> 
> This allows us to convert all call sites to using
> CLASS(qcom_scm_clk, clk)() instead of the manual enable/check/disable
> pattern and to remove the associated goto labels.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 89 +++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 1684220617748e4890ecb45a1787a6798dafd5cc..35aa9e8886b6ce8ab8eaf16c83fef7aafaef2822 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -192,6 +192,9 @@ static void qcom_scm_clk_disable(void)
>  	clk_disable_unprepare(__scm->bus_clk);
>  }
>  
> +DEFINE_CLASS(qcom_scm_clk, int, if (!_T) qcom_scm_clk_disable(),
> +	     qcom_scm_clk_enable(), void)
> +
>  static int qcom_scm_bw_enable(void)
>  {
>  	int ret = 0;
> @@ -511,13 +514,11 @@ static int qcom_scm_disable_sdi(void)
>  	};
>  	struct qcom_scm_res res;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return clk;
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  
> -	qcom_scm_clk_disable();
> -
>  	return ret ? : res.result[0];
>  }
>  
> @@ -616,22 +617,19 @@ static int __qcom_scm_pas_init_image(u32 pas_id, dma_addr_t mdata_phys,
>  	};
>  	int ret;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return clk;
>  
>  	ret = qcom_scm_bw_enable();
>  	if (ret)
> -		goto disable_clk;
> +		return ret;
>  
>  	desc.args[1] = mdata_phys;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, res);
>  	qcom_scm_bw_disable();
>  
> -disable_clk:
> -	qcom_scm_clk_disable();
> -
>  	return ret;
>  }
>  
> @@ -758,20 +756,17 @@ int qcom_scm_pas_mem_setup(u32 pas_id, phys_addr_t addr, phys_addr_t size)
>  	};
>  	struct qcom_scm_res res;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return clk;
>  
>  	ret = qcom_scm_bw_enable();
>  	if (ret)
> -		goto disable_clk;
> +		return ret;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	qcom_scm_bw_disable();
>  
> -disable_clk:
> -	qcom_scm_clk_disable();
> -
>  	return ret ? : res.result[0];
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_pas_mem_setup);
> @@ -877,13 +872,13 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  	void *tbl_ptr;
>  	int ret;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ERR_PTR(ret);
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return ERR_PTR(clk);
>  
>  	ret = qcom_scm_bw_enable();
>  	if (ret)
> -		goto disable_clk;
> +		return ERR_PTR(ret);
>  
>  	/*
>  	 * TrustZone can not accept buffer as NULL value as argument hence,
> @@ -930,9 +925,6 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  disable_scm_bw:
>  	qcom_scm_bw_disable();
>  
> -disable_clk:
> -	qcom_scm_clk_disable();
> -
>  	return ret ? ERR_PTR(ret) : tbl_ptr;
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_pas_get_rsc_table);
> @@ -956,20 +948,17 @@ int qcom_scm_pas_auth_and_reset(u32 pas_id)
>  	};
>  	struct qcom_scm_res res;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return clk;
>  
>  	ret = qcom_scm_bw_enable();
>  	if (ret)
> -		goto disable_clk;
> +		return ret;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	qcom_scm_bw_disable();
>  
> -disable_clk:
> -	qcom_scm_clk_disable();
> -
>  	return ret ? : res.result[0];
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_pas_auth_and_reset);
> @@ -1039,20 +1028,17 @@ int qcom_scm_pas_shutdown(u32 pas_id)
>  	};
>  	struct qcom_scm_res res;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return clk;
>  
>  	ret = qcom_scm_bw_enable();
>  	if (ret)
> -		goto disable_clk;
> +		return ret;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	qcom_scm_bw_disable();
>  
> -disable_clk:
> -	qcom_scm_clk_disable();
> -
>  	return ret ? : res.result[0];
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_pas_shutdown);
> @@ -1791,18 +1777,13 @@ EXPORT_SYMBOL_GPL(qcom_scm_import_ice_key);
>   */
>  bool qcom_scm_hdcp_available(void)
>  {
> -	bool avail;
> -	int ret = qcom_scm_clk_enable();
> +	CLASS(qcom_scm_clk, clk)();
>  
> -	if (ret)
> -		return ret;
> -
> -	avail = __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
> -						QCOM_SCM_HDCP_INVOKE);
> -
> -	qcom_scm_clk_disable();
> +	if (clk)
> +		return false;
>  
> -	return avail;
> +	return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_HDCP,
> +					    QCOM_SCM_HDCP_INVOKE);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_hdcp_available);
>  
> @@ -1840,15 +1821,13 @@ int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp)
>  	if (req_cnt > QCOM_SCM_HDCP_MAX_REQ_CNT)
>  		return -ERANGE;
>  
> -	ret = qcom_scm_clk_enable();
> -	if (ret)
> -		return ret;
> +	CLASS(qcom_scm_clk, clk)();
> +	if (clk)
> +		return clk;
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	*resp = res.result[0];
>  
> -	qcom_scm_clk_disable();
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_hdcp_req);
>

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

-- 
-Mukesh Ojha

Re: [PATCH 1/4] firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler

[Bartosz Golaszewski]

On Thu, 2 Jul 2026 10:10:26 +0200, Mukesh Ojha
<mukesh.ojha@oss.qualcomm.com> said:
> On Wed, Jul 01, 2026 at 03:38:55PM +0200, Bartosz Golaszewski wrote:
>> We currently only silently skip NULL-pointers in the cleanup handler for
>> tzmem. It's possible that we get passed a pointer holding an ERR_PTR()
>> value so skip it too.
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>> ---
>>  include/linux/firmware/qcom/qcom_tzmem.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h
>> index 23173e0c3dddd154dd56dc3dcb56bd20ada0520a..b5520178bf6f53b86b530571a3be9f302225f022 100644
>> --- a/include/linux/firmware/qcom/qcom_tzmem.h
>> +++ b/include/linux/firmware/qcom/qcom_tzmem.h
>> @@ -58,7 +58,7 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>>  void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp);
>>  void qcom_tzmem_free(void *ptr);
>>
>> -DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T))
>> +DEFINE_FREE(qcom_tzmem, void *, if (!IS_ERR_OR_NULL(_T)) qcom_tzmem_free(_T))
>
> Is this really required? Are we trying to address an issue which
> does not exist today?
>

It becomes needed in patch 2/4 where in qcom_scm_pas_get_rsc_table() the
pointer returned by _qcom_scm_pas_get_rsc_table() can contain ERR_PTR() so
this patch preempts a possible problem.

Bart

Re: [PATCH 2/4] firmware: qcom: scm: use __free(qcom_tzmem) to simplify cleanup

[Konrad Dybcio]

On 7/1/26 3:38 PM, Bartosz Golaszewski wrote:
> Use the __free(qcom_tzmem) cleanup attribute (together with no_free_ptr()
> whenever ownership is transferred) to replace open-coded
> qcom_tzmem_free() calls and their associated goto labels.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 3/4] firmware: qcom: scm: introduce qcom_scm_clk class for clock management

[Konrad Dybcio]

On 7/1/26 3:38 PM, Bartosz Golaszewski wrote:
> Define DEFINE_CLASS(qcom_scm_clk) that calls qcom_scm_clk_enable() on
> construction and automatically calls qcom_scm_clk_disable() at scope exit
> *if* the enable succeeded.
> 
> This allows us to convert all call sites to using
> CLASS(qcom_scm_clk, clk)() instead of the manual enable/check/disable
> pattern and to remove the associated goto labels.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 4/4] firmware: qcom: scm: introduce qcom_scm_bw class for bandwidth management

[Konrad Dybcio]

On 7/1/26 3:38 PM, Bartosz Golaszewski wrote:
> Define DEFINE_CLASS(qcom_scm_bw) that calls qcom_scm_bw_enable() on
> construction and automatically calls qcom_scm_bw_disable() at scope exit
> *if* the enable succeeded.
> 
> This allows us to convert all call sites to using
> CLASS(qcom_scm_bw, bw)() instead of the manual enable/check/disable
> pattern and to remove the associated goto labels in cleanup path.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 1/4] firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler

[Konrad Dybcio]

On 7/2/26 10:10 AM, Mukesh Ojha wrote:
> On Wed, Jul 01, 2026 at 03:38:55PM +0200, Bartosz Golaszewski wrote:
>> We currently only silently skip NULL-pointers in the cleanup handler for
>> tzmem. It's possible that we get passed a pointer holding an ERR_PTR()
>> value so skip it too.
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>> ---
>>  include/linux/firmware/qcom/qcom_tzmem.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h
>> index 23173e0c3dddd154dd56dc3dcb56bd20ada0520a..b5520178bf6f53b86b530571a3be9f302225f022 100644
>> --- a/include/linux/firmware/qcom/qcom_tzmem.h
>> +++ b/include/linux/firmware/qcom/qcom_tzmem.h
>> @@ -58,7 +58,7 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>>  void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp);
>>  void qcom_tzmem_free(void *ptr);
>>  
>> -DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T))
>> +DEFINE_FREE(qcom_tzmem, void *, if (!IS_ERR_OR_NULL(_T)) qcom_tzmem_free(_T))
> 
> Is this really required? Are we trying to address an issue which
> does not exist today?

It doesn't, I was scratching my head too, but in patch 2 there's:

void *output_rt_tzm __free(qcom_tzmem) =
		__qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt_tzm,
					     input_rt_size, &size);

and __qcom_scm_pas_get_rsc_table() does throw ERR_PTRs

Konrad

Re: [PATCH 1/4] firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler

[Mukesh Ojha]

On Thu, Jul 02, 2026 at 04:47:51AM -0500, Bartosz Golaszewski wrote:
> On Thu, 2 Jul 2026 10:10:26 +0200, Mukesh Ojha
> <mukesh.ojha@oss.qualcomm.com> said:
> > On Wed, Jul 01, 2026 at 03:38:55PM +0200, Bartosz Golaszewski wrote:
> >> We currently only silently skip NULL-pointers in the cleanup handler for
> >> tzmem. It's possible that we get passed a pointer holding an ERR_PTR()
> >> value so skip it too.
> >>
> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> >> ---
> >>  include/linux/firmware/qcom/qcom_tzmem.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/firmware/qcom/qcom_tzmem.h b/include/linux/firmware/qcom/qcom_tzmem.h
> >> index 23173e0c3dddd154dd56dc3dcb56bd20ada0520a..b5520178bf6f53b86b530571a3be9f302225f022 100644
> >> --- a/include/linux/firmware/qcom/qcom_tzmem.h
> >> +++ b/include/linux/firmware/qcom/qcom_tzmem.h
> >> @@ -58,7 +58,7 @@ devm_qcom_tzmem_pool_new(struct device *dev,
> >>  void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp);
> >>  void qcom_tzmem_free(void *ptr);
> >>
> >> -DEFINE_FREE(qcom_tzmem, void *, if (_T) qcom_tzmem_free(_T))
> >> +DEFINE_FREE(qcom_tzmem, void *, if (!IS_ERR_OR_NULL(_T)) qcom_tzmem_free(_T))
> >
> > Is this really required? Are we trying to address an issue which
> > does not exist today?
> >
> 
> It becomes needed in patch 2/4 where in qcom_scm_pas_get_rsc_table() the
> pointer returned by _qcom_scm_pas_get_rsc_table() can contain ERR_PTR() so
> this patch preempts a possible problem.
> 

Yes, I see now. It was easy to get ignored in this cleanup.

Reviewed-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

-- 
-Mukesh Ojha

Re: [PATCH 1/4] firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler

[Konrad Dybcio]

On 7/1/26 3:38 PM, Bartosz Golaszewski wrote:
> We currently only silently skip NULL-pointers in the cleanup handler for
> tzmem. It's possible that we get passed a pointer holding an ERR_PTR()
> value so skip it too.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 0/4] firmware: qcom: scm: simplify code by using classes

[Bjorn Andersson]

On Wed, Jul 01, 2026 at 03:38:54PM +0200, Bartosz Golaszewski wrote:
> There's a pattern of enabling the clocks and/or setting the bandwith
> limits before performing an SCM call and reverting the above after it's
> node. In all places it's done unconditionally and can be simplified by
> providing appropriate classes. There are also a few places where we can
> use __free() for tzmem pointers.
> 

This ended up conflicting with the PAS changes, can you please rebase?

Regards,
Bjorn

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
> Bartosz Golaszewski (4):
>       firmware: qcom: tzmem: guard against IS_ERR() in the cleanup handler
>       firmware: qcom: scm: use __free(qcom_tzmem) to simplify cleanup
>       firmware: qcom: scm: introduce qcom_scm_clk class for clock management
>       firmware: qcom: scm: introduce qcom_scm_bw class for bandwidth management
> 
>  drivers/firmware/qcom/qcom_scm.c         | 184 ++++++++++++-------------------
>  include/linux/firmware/qcom/qcom_tzmem.h |   2 +-
>  2 files changed, 73 insertions(+), 113 deletions(-)
> ---
> base-commit: be5c93fa674f0fc3c8f359c2143abce6bbb422e6
> change-id: 20260701-qcom-scm-code-shrink-c9aff845537d
> 
> Best regards,
> -- 
> Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>