[PATCH 0/6] firmware: qcom: scm: Fixes for concurrency

[PATCH 0/6] firmware: qcom: scm: Fixes for concurrency

[Krzysztof Kozlowski]

SCM driver looks messy in terms of handling concurrency of probe.  The
driver exports interface which is guarded by global '__scm' variable
but:
1. Lacks proper read barrier (commit adding write barriers mixed up
   READ_ONCE with a read barrier).
2. Lacks barriers or checks for '__scm' in multiple places.
3. Lacks probe error cleanup.

I fixed here few visible things, but this was not tested extensively.  I
tried only SM8450.

ARM32 and SC8280xp/X1E platforms would be useful for testing as well.

All the issues here are non-urgent, IOW, they were here for some time
(v6.10-rc1 and earlier).

Best regards,
Krzysztof

---
Krzysztof Kozlowski (6):
      firmware: qcom: scm: Fix missing read barrier in qcom_scm_is_available()
      firmware: qcom: scm: Fix missing read barrier in qcom_scm_get_tzmem_pool()
      firmware: qcom: scm: Handle various probe ordering for qcom_scm_assign_mem()
      [RFC/RFT] firmware: qcom: scm: Cleanup global '__scm' on probe failures
      firmware: qcom: scm: smc: Handle missing SCM device
      firmware: qcom: scm: smc: Narrow 'mempool' variable scope

 drivers/firmware/qcom/qcom_scm-smc.c |  6 +++-
 drivers/firmware/qcom/qcom_scm.c     | 55 +++++++++++++++++++++++++-----------
 2 files changed, 44 insertions(+), 17 deletions(-)
---
base-commit: 414c97c966b69e4a6ea7b32970fa166b2f9b9ef0
change-id: 20241119-qcom-scm-missing-barriers-and-all-sort-of-srap-a25d59074882

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

[PATCH 1/6] firmware: qcom: scm: Fix missing read barrier in qcom_scm_is_available()

[Krzysztof Kozlowski]

Commit 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq
completion variable initialization") introduced a write barrier in probe
function to store global '__scm' variable.  It also claimed that it
added a read barrier, because as we all known barriers are paired (see
memory-barriers.txt: "Note that write barriers should normally be paired
with read or address-dependency barriers"), however it did not really
add it.

The offending commit used READ_ONCE() to access '__scm' global which is
not a barrier.

The barrier is needed so the store to '__scm' will be properly visible.
This is most likely not fatal in current driver design, because missing
read barrier would mean qcom_scm_is_available() callers will access old
value, NULL.  Driver does not support unbinding and does not correctly
handle probe failures, thus there is no risk of stale or old pointer in
'__scm' variable.

However for code correctness, readability and to be sure that we did not
mess up something in this tricky topic of SMP barriers, add a read
barrier for accessing '__scm'.  Change also comment from useless/obvious
what does barrier do, to what is expected: which other parts of the code
are involved here.

Fixes: 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq completion variable initialization")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 72bf87ddcd969834609cda2aa915b67505e93943..246d672e8f7f0e2a326a03a5af40cd434a665e67 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1867,7 +1867,8 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm)
  */
 bool qcom_scm_is_available(void)
 {
-	return !!READ_ONCE(__scm);
+	/* Paired with smp_store_release() in qcom_scm_probe */
+	return !!smp_load_acquire(&__scm);
 }
 EXPORT_SYMBOL_GPL(qcom_scm_is_available);
 
@@ -2024,7 +2025,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/* Let all above stores be available after this */
+	/* Paired with smp_load_acquire() in qcom_scm_is_available(). */
 	smp_store_release(&__scm, scm);
 
 	irq = platform_get_irq_optional(pdev, 0);

-- 
2.43.0

Re: [PATCH 1/6] firmware: qcom: scm: Fix missing read barrier in qcom_scm_is_available()

[Bartosz Golaszewski]

On Tue, Nov 19, 2024 at 7:37 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Commit 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq
> completion variable initialization") introduced a write barrier in probe
> function to store global '__scm' variable.  It also claimed that it
> added a read barrier, because as we all known barriers are paired (see
> memory-barriers.txt: "Note that write barriers should normally be paired
> with read or address-dependency barriers"), however it did not really
> add it.
>
> The offending commit used READ_ONCE() to access '__scm' global which is
> not a barrier.
>
> The barrier is needed so the store to '__scm' will be properly visible.
> This is most likely not fatal in current driver design, because missing
> read barrier would mean qcom_scm_is_available() callers will access old
> value, NULL.  Driver does not support unbinding and does not correctly
> handle probe failures, thus there is no risk of stale or old pointer in
> '__scm' variable.
>
> However for code correctness, readability and to be sure that we did not
> mess up something in this tricky topic of SMP barriers, add a read
> barrier for accessing '__scm'.  Change also comment from useless/obvious
> what does barrier do, to what is expected: which other parts of the code
> are involved here.
>
> Fixes: 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq completion variable initialization")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 72bf87ddcd969834609cda2aa915b67505e93943..246d672e8f7f0e2a326a03a5af40cd434a665e67 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1867,7 +1867,8 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm)
>   */
>  bool qcom_scm_is_available(void)
>  {
> -       return !!READ_ONCE(__scm);
> +       /* Paired with smp_store_release() in qcom_scm_probe */
> +       return !!smp_load_acquire(&__scm);
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>
> @@ -2024,7 +2025,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> -       /* Let all above stores be available after this */
> +       /* Paired with smp_load_acquire() in qcom_scm_is_available(). */
>         smp_store_release(&__scm, scm);
>
>         irq = platform_get_irq_optional(pdev, 0);
>
> --
> 2.43.0
>
>

I'm not an expert on barriers and SMP but the explanation sounds correct to me.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 2/6] firmware: qcom: scm: Fix missing read barrier in qcom_scm_get_tzmem_pool()

[Krzysztof Kozlowski]

Commit 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq
completion variable initialization") introduced a write barrier in probe
function to store global '__scm' variable.  We all known barriers are
paired (see memory-barriers.txt: "Note that write barriers should
normally be paired with read or address-dependency barriers"), therefore
accessing it from concurrent contexts requires read barrier.  Previous
commit added such barrier in qcom_scm_is_available(), so let's use that
directly.

Lack of this read barrier can result in fetching stale '__scm' variable
value, NULL, and dereferencing it.

Fixes: ca61d6836e6f ("firmware: qcom: scm: fix a NULL-pointer dereference")
Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 246d672e8f7f0e2a326a03a5af40cd434a665e67..5d91b8e22844608f35432f1ba9c08d477d4ff762 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -217,7 +217,10 @@ static DEFINE_SPINLOCK(scm_query_lock);
 
 struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void)
 {
-	return __scm ? __scm->mempool : NULL;
+	if (!qcom_scm_is_available())
+		return NULL;
+
+	return __scm->mempool;
 }
 
 static enum qcom_scm_convention __get_convention(void)

-- 
2.43.0

Re: [PATCH 2/6] firmware: qcom: scm: Fix missing read barrier in qcom_scm_get_tzmem_pool()

[Krzysztof Kozlowski]

On 19/11/2024 19:33, Krzysztof Kozlowski wrote:
> Commit 2e4955167ec5 ("firmware: qcom: scm: Fix __scm and waitq
> completion variable initialization") introduced a write barrier in probe
> function to store global '__scm' variable.  We all known barriers are
> paired (see memory-barriers.txt: "Note that write barriers should
> normally be paired with read or address-dependency barriers"), therefore
> accessing it from concurrent contexts requires read barrier.  Previous
> commit added such barrier in qcom_scm_is_available(), so let's use that
> directly.
> 
> Lack of this read barrier can result in fetching stale '__scm' variable
> value, NULL, and dereferencing it.
> 
> Fixes: ca61d6836e6f ("firmware: qcom: scm: fix a NULL-pointer dereference")
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 246d672e8f7f0e2a326a03a5af40cd434a665e67..5d91b8e22844608f35432f1ba9c08d477d4ff762 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -217,7 +217,10 @@ static DEFINE_SPINLOCK(scm_query_lock);
>  
>  struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void)
>  {
> -	return __scm ? __scm->mempool : NULL;
> +	if (!qcom_scm_is_available())
> +		return NULL;
> +
> +	return __scm->mempool;

I mentioned in commit msg that previous commit adds barrier in
qcom_scm_get_tzmem_pool(), so to be clear:
This depends on previous commit, because that barrier in
qcom_scm_is_available() solves the control dependency here, assuming the
minimal guarantee #1 ("On any given CPU, dependent memory accesses will
be issued in order, with respect to itself.")

If this is inlined by compiler it will be:

scm = READ_ONCE(__scm);
barrier()
if (scm)
	return scm->mempool;
else
	return NULL;

Which should be even safer than standard guarantee above (according to
my understanding).

Best regards,
Krzysztof

[PATCH 3/6] firmware: qcom: scm: Handle various probe ordering for qcom_scm_assign_mem()

[Krzysztof Kozlowski]

The SCM driver can defer or fail probe, or just load a bit later so
callers of qcom_scm_assign_mem() should defer if the device is not ready.

This fixes theoretical NULL pointer exception, triggered via introducing
probe deferral in SCM driver with call trace:

  qcom_tzmem_alloc+0x70/0x1ac (P)
  qcom_tzmem_alloc+0x64/0x1ac (L)
  qcom_scm_assign_mem+0x78/0x194
  qcom_rmtfs_mem_probe+0x2d4/0x38c
  platform_probe+0x68/0xc8

Fixes: d82bd359972a ("firmware: scm: Add new SCM call API for switching memory ownership")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

I am not sure about commit introducing it (Fixes tag) thus not Cc-ing
stable.
---
 drivers/firmware/qcom/qcom_scm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 5d91b8e22844608f35432f1ba9c08d477d4ff762..93212c8f20ad65ecc44804b00f4b93e3eaaf8d95 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1075,6 +1075,9 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
 	int ret, i, b;
 	u64 srcvm_bits = *srcvm;
 
+	if (!qcom_scm_is_available())
+		return -EPROBE_DEFER;
+
 	src_sz = hweight64(srcvm_bits) * sizeof(*src);
 	mem_to_map_sz = sizeof(*mem_to_map);
 	dest_sz = dest_cnt * sizeof(*destvm);

-- 
2.43.0

[PATCH RFC/RFT 4/6] firmware: qcom: scm: Cleanup global '__scm' on probe failures

[Krzysztof Kozlowski]

If SCM driver fails the probe, it should not leave global '__scm'
variable assigned, because external users of this driver will assume the
probe finished successfully.  For example TZMEM parts ('__scm->mempool')
are initialized later in the probe, but users of it (__scm_smc_call())
rely on the '__scm' variable.

This fixes theoretical NULL pointer exception, triggered via introducing
probe deferral in SCM driver with call trace:

  qcom_tzmem_alloc+0x70/0x1ac (P)
  qcom_tzmem_alloc+0x64/0x1ac (L)
  qcom_scm_assign_mem+0x78/0x194
  qcom_rmtfs_mem_probe+0x2d4/0x38c
  platform_probe+0x68/0xc8

Fixes: 40289e35ca52 ("firmware: qcom: scm: enable the TZ mem allocator")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

I am not really sure if authors wanted the cleanup at this point.

Also, I am not sure about commit introducing it (Fixes tag) thus not
Cc-ing stable.
---
 drivers/firmware/qcom/qcom_scm.c | 42 +++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 93212c8f20ad65ecc44804b00f4b93e3eaaf8d95..c6d526d055003a17a8f59471f93160bbccfc1658 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -2036,13 +2036,17 @@ static int qcom_scm_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq < 0) {
-		if (irq != -ENXIO)
-			return irq;
+		if (irq != -ENXIO) {
+			ret = irq;
+			goto err;
+		}
 	} else {
 		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
 						IRQF_ONESHOT, "qcom-scm", __scm);
-		if (ret < 0)
-			return dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
+		if (ret < 0) {
+			dev_err_probe(scm->dev, ret, "Failed to request qcom-scm irq\n");
+			goto err;
+		}
 	}
 
 	__get_convention();
@@ -2061,14 +2065,18 @@ static int qcom_scm_probe(struct platform_device *pdev)
 		qcom_scm_disable_sdi();
 
 	ret = of_reserved_mem_device_init(__scm->dev);
-	if (ret && ret != -ENODEV)
-		return dev_err_probe(__scm->dev, ret,
-				     "Failed to setup the reserved memory region for TZ mem\n");
+	if (ret && ret != -ENODEV) {
+		dev_err_probe(__scm->dev, ret,
+			      "Failed to setup the reserved memory region for TZ mem\n");
+		goto err;
+	}
 
 	ret = qcom_tzmem_enable(__scm->dev);
-	if (ret)
-		return dev_err_probe(__scm->dev, ret,
-				     "Failed to enable the TrustZone memory allocator\n");
+	if (ret) {
+		dev_err_probe(__scm->dev, ret,
+			      "Failed to enable the TrustZone memory allocator\n");
+		goto err;
+	}
 
 	memset(&pool_config, 0, sizeof(pool_config));
 	pool_config.initial_size = 0;
@@ -2076,9 +2084,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	pool_config.max_size = SZ_256K;
 
 	__scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, &pool_config);
-	if (IS_ERR(__scm->mempool))
-		return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool),
-				     "Failed to create the SCM memory pool\n");
+	if (IS_ERR(__scm->mempool)) {
+		dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool),
+			      "Failed to create the SCM memory pool\n");
+		goto err;
+	}
 
 	/*
 	 * Initialize the QSEECOM interface.
@@ -2094,6 +2104,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	WARN(ret < 0, "failed to initialize qseecom: %d\n", ret);
 
 	return 0;
+
+err:
+	/* Paired with smp_load_acquire() in qcom_scm_is_available(). */
+	smp_store_release(&__scm, 0);
+
+	return ret;
 }
 
 static void qcom_scm_shutdown(struct platform_device *pdev)

-- 
2.43.0

Re: [PATCH RFC/RFT 4/6] firmware: qcom: scm: Cleanup global '__scm' on probe failures

[Krzysztof Kozlowski]

On 19/11/2024 19:33, Krzysztof Kozlowski wrote:
>  	/*
>  	 * Initialize the QSEECOM interface.
> @@ -2094,6 +2104,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	WARN(ret < 0, "failed to initialize qseecom: %d\n", ret);
>  
>  	return 0;
> +
> +err:
> +	/* Paired with smp_load_acquire() in qcom_scm_is_available(). */
> +	smp_store_release(&__scm, 0);
Heh, I should store there NULL, obviously.

Best regards,
Krzysztof

[PATCH 5/6] firmware: qcom: scm: smc: Handle missing SCM device

[Krzysztof Kozlowski]

Commit ca61d6836e6f ("firmware: qcom: scm: fix a NULL-pointer
dereference") makes it explicit that qcom_scm_get_tzmem_pool() can
return NULL, therefore its users should handle this.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm-smc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 2b4c2826f57251f25a1bc37c3b467dde28e1268b..3f10b23ec941b558e1d91761011776bb5c9d11b5 100644
--- a/drivers/firmware/qcom/qcom_scm-smc.c
+++ b/drivers/firmware/qcom/qcom_scm-smc.c
@@ -173,6 +173,9 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];
 
 	if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
+		if (!mempool)
+			return -EINVAL;
+
 		args_virt = qcom_tzmem_alloc(mempool,
 					     SCM_SMC_N_EXT_ARGS * sizeof(u64),
 					     flag);

-- 
2.43.0

[PATCH 6/6] firmware: qcom: scm: smc: Narrow 'mempool' variable scope

[Krzysztof Kozlowski]

Only part of the __scm_smc_call() function uses 'mempool' variable, so
narrow the scope to make it more readable.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm-smc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 3f10b23ec941b558e1d91761011776bb5c9d11b5..574930729ddd72d98013770da97cc018a52554ff 100644
--- a/drivers/firmware/qcom/qcom_scm-smc.c
+++ b/drivers/firmware/qcom/qcom_scm-smc.c
@@ -152,7 +152,6 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		   enum qcom_scm_convention qcom_convention,
 		   struct qcom_scm_res *res, bool atomic)
 {
-	struct qcom_tzmem_pool *mempool = qcom_scm_get_tzmem_pool();
 	int arglen = desc->arginfo & 0xf;
 	int i, ret;
 	void *args_virt __free(qcom_tzmem) = NULL;
@@ -173,6 +172,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];
 
 	if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
+		struct qcom_tzmem_pool *mempool = qcom_scm_get_tzmem_pool();
+
 		if (!mempool)
 			return -EINVAL;
 

-- 
2.43.0

Re: [PATCH 6/6] firmware: qcom: scm: smc: Narrow 'mempool' variable scope

[Bartosz Golaszewski]

On Tue, Nov 19, 2024 at 7:38 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Only part of the __scm_smc_call() function uses 'mempool' variable, so
> narrow the scope to make it more readable.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm-smc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 3f10b23ec941b558e1d91761011776bb5c9d11b5..574930729ddd72d98013770da97cc018a52554ff 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> @@ -152,7 +152,6 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>                    enum qcom_scm_convention qcom_convention,
>                    struct qcom_scm_res *res, bool atomic)
>  {
> -       struct qcom_tzmem_pool *mempool = qcom_scm_get_tzmem_pool();
>         int arglen = desc->arginfo & 0xf;
>         int i, ret;
>         void *args_virt __free(qcom_tzmem) = NULL;
> @@ -173,6 +172,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>                 smc.args[i + SCM_SMC_FIRST_REG_IDX] = desc->args[i];
>
>         if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> +               struct qcom_tzmem_pool *mempool = qcom_scm_get_tzmem_pool();
> +
>                 if (!mempool)
>                         return -EINVAL;
>
>
> --
> 2.43.0
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 0/6] firmware: qcom: scm: Fixes for concurrency

[Dmitry Baryshkov]

On Tue, Nov 19, 2024 at 07:33:16PM +0100, Krzysztof Kozlowski wrote:
> SCM driver looks messy in terms of handling concurrency of probe.  The
> driver exports interface which is guarded by global '__scm' variable
> but:
> 1. Lacks proper read barrier (commit adding write barriers mixed up
>    READ_ONCE with a read barrier).
> 2. Lacks barriers or checks for '__scm' in multiple places.
> 3. Lacks probe error cleanup.
> 
> I fixed here few visible things, but this was not tested extensively.  I
> tried only SM8450.
> 
> ARM32 and SC8280xp/X1E platforms would be useful for testing as well.

ARM32 devices are present in the lab.

> 
> All the issues here are non-urgent, IOW, they were here for some time
> (v6.10-rc1 and earlier).
> 
> Best regards,
> Krzysztof
> 
> ---
> Krzysztof Kozlowski (6):
>       firmware: qcom: scm: Fix missing read barrier in qcom_scm_is_available()
>       firmware: qcom: scm: Fix missing read barrier in qcom_scm_get_tzmem_pool()
>       firmware: qcom: scm: Handle various probe ordering for qcom_scm_assign_mem()
>       [RFC/RFT] firmware: qcom: scm: Cleanup global '__scm' on probe failures
>       firmware: qcom: scm: smc: Handle missing SCM device
>       firmware: qcom: scm: smc: Narrow 'mempool' variable scope
> 
>  drivers/firmware/qcom/qcom_scm-smc.c |  6 +++-
>  drivers/firmware/qcom/qcom_scm.c     | 55 +++++++++++++++++++++++++-----------
>  2 files changed, 44 insertions(+), 17 deletions(-)
> ---
> base-commit: 414c97c966b69e4a6ea7b32970fa166b2f9b9ef0
> change-id: 20241119-qcom-scm-missing-barriers-and-all-sort-of-srap-a25d59074882
> 
> Best regards,
> -- 
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 

-- 
With best wishes
Dmitry