[PATCH 0/2] firmware: qcom: scm: fix SMC calls on ARM32

[PATCH 0/2] firmware: qcom: scm: fix SMC calls on ARM32

[Bartosz Golaszewski]

The new TZ Mem allocator assumes the SCM driver is always probed which
apparently isn't the case on older platforms. Add a proper workaround.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (2):
      firmware: qcom: scm: fix a NULL-pointer dereference
      firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

 drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
 drivers/firmware/qcom/qcom_scm.c     |  2 +-
 2 files changed, 25 insertions(+), 5 deletions(-)
---
base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8
change-id: 20240909-tzmem-null-ptr-2a9ddd9889aa

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH 1/2] firmware: qcom: scm: fix a NULL-pointer dereference

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Some SCM calls can be invoked with __scm being NULL (the driver may not
have been and will not be probed as there's no SCM entry in device-tree).
Make sure we don't dereference a NULL pointer.

Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
Reported-by: Rudraksha Gupta <guptarud@gmail.com>
Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 10986cb11ec0..8bac4915c211 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -216,7 +216,7 @@ static DEFINE_SPINLOCK(scm_query_lock);
 
 struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void)
 {
-	return __scm->mempool;
+	return __scm ? __scm->mempool : NULL;
 }
 
 static enum qcom_scm_convention __get_convention(void)

-- 
2.43.0

[PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Older platforms don't have an actual SCM device tied into the driver
model and so there's no struct device which to use with the TZ Mem API.
We need to fall-back to kcalloc() when allocating the buffer for
additional SMC arguments on such platforms which don't even probe the SCM
driver and never create the TZMem pool.

Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
Reported-by: Rudraksha Gupta <guptarud@gmail.com>
Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/<S-Del>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 2b4c2826f572..13f72541033c 100644
--- a/drivers/firmware/qcom/qcom_scm-smc.c
+++ b/drivers/firmware/qcom/qcom_scm-smc.c
@@ -147,6 +147,15 @@ static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
 	return 0;
 }
 
+static void smc_args_free(void *ptr)
+{
+	if (qcom_scm_get_tzmem_pool())
+		qcom_tzmem_free(ptr);
+	else
+		kfree(ptr);
+}
+
+DEFINE_FREE(smc_args, void *, if (!IS_ERR_OR_NULL(_T)) smc_args_free(_T));
 
 int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		   enum qcom_scm_convention qcom_convention,
@@ -155,7 +164,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 	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;
+	void *args_virt __free(smc_args) = NULL;
 	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
 	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
 	u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
@@ -173,9 +182,20 @@ 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)) {
-		args_virt = qcom_tzmem_alloc(mempool,
-					     SCM_SMC_N_EXT_ARGS * sizeof(u64),
-					     flag);
+		/*
+		 * Older platforms don't have an entry for SCM in device-tree
+		 * and so no device is bound to the SCM driver. This means there
+		 * is no struct device for the TZ Mem API. Fall back to
+		 * kcalloc() on such platforms.
+		 */
+		if (mempool)
+			args_virt = qcom_tzmem_alloc(
+					mempool,
+					SCM_SMC_N_EXT_ARGS * sizeof(u64),
+					flag);
+		else
+			args_virt = kcalloc(SCM_SMC_N_EXT_ARGS, sizeof(u64),
+					    flag);
 		if (!args_virt)
 			return -ENOMEM;
 

-- 
2.43.0

Re: [PATCH 1/2] firmware: qcom: scm: fix a NULL-pointer dereference

[Konrad Dybcio]

On 9.09.2024 8:38 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Some SCM calls can be invoked with __scm being NULL (the driver may not
> have been and will not be probed as there's no SCM entry in device-tree).
> Make sure we don't dereference a NULL pointer.
> 
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..8bac4915c211 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -216,7 +216,7 @@ static DEFINE_SPINLOCK(scm_query_lock);
>  
>  struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void)
>  {
> -	return __scm->mempool;
> +	return __scm ? __scm->mempool : NULL;
>  }


Reviewed-by: Konrad Dybcio <konradybcio@kernel.org>

Konrad

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Konrad Dybcio]

On 9.09.2024 8:38 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Older platforms don't have an actual SCM device tied into the driver
> model and so there's no struct device which to use with the TZ Mem API.
> We need to fall-back to kcalloc() when allocating the buffer for
> additional SMC arguments on such platforms which don't even probe the SCM
> driver and never create the TZMem pool.
> 
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/<S-Del>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Considering all SoCs that would be allowed on a roller coaster by age now
are supposed not to need this patch.. perhaps this isn't a hot enough
path for static branches, but I'd say some likely() hints could be used
here..

Konrad

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Elliot Berman]

On Mon, Sep 09, 2024 at 08:38:45PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Older platforms don't have an actual SCM device tied into the driver
> model and so there's no struct device which to use with the TZ Mem API.
> We need to fall-back to kcalloc() when allocating the buffer for
> additional SMC arguments on such platforms which don't even probe the SCM
> driver and never create the TZMem pool.
> 
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/<S-Del>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 2b4c2826f572..13f72541033c 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> @@ -147,6 +147,15 @@ static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
>  	return 0;
>  }
>  
> +static void smc_args_free(void *ptr)
> +{
> +	if (qcom_scm_get_tzmem_pool())

I'm a little concerned about this check. I didn't think making SCM calls
without the SCM device probed was possible until this report. We do
worry about that in the downstream kernel. So, I'm not sure if this
scenario is currently possible in the upstream kernel.

It's possible that some driver makes SCM call in parallel to SCM device
probing. Then, it might be possible for qcom_scm_get_tzmem_pool() to
return NULL at beginning of function and then a valid pointer by the
time we're freeing the ptr.

- Elliot

> +		qcom_tzmem_free(ptr);
> +	else
> +		kfree(ptr);
> +}
> +
> +DEFINE_FREE(smc_args, void *, if (!IS_ERR_OR_NULL(_T)) smc_args_free(_T));
>  
>  int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  		   enum qcom_scm_convention qcom_convention,
> @@ -155,7 +164,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>  	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;
> +	void *args_virt __free(smc_args) = NULL;
>  	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>  	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
>  	u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
> @@ -173,9 +182,20 @@ 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)) {
> -		args_virt = qcom_tzmem_alloc(mempool,
> -					     SCM_SMC_N_EXT_ARGS * sizeof(u64),
> -					     flag);
> +		/*
> +		 * Older platforms don't have an entry for SCM in device-tree
> +		 * and so no device is bound to the SCM driver. This means there
> +		 * is no struct device for the TZ Mem API. Fall back to
> +		 * kcalloc() on such platforms.
> +		 */
> +		if (mempool)
> +			args_virt = qcom_tzmem_alloc(
> +					mempool,
> +					SCM_SMC_N_EXT_ARGS * sizeof(u64),
> +					flag);
> +		else
> +			args_virt = kcalloc(SCM_SMC_N_EXT_ARGS, sizeof(u64),
> +					    flag);
>  		if (!args_virt)
>  			return -ENOMEM;
>  
> 
> -- 
> 2.43.0
> 

Re: [PATCH 1/2] firmware: qcom: scm: fix a NULL-pointer dereference

[Rudraksha Gupta]

On 9/9/24 11:38, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Some SCM calls can be invoked with __scm being NULL (the driver may not
> have been and will not be probed as there's no SCM entry in device-tree).
> Make sure we don't dereference a NULL pointer.
>
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/firmware/qcom/qcom_scm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 10986cb11ec0..8bac4915c211 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -216,7 +216,7 @@ static DEFINE_SPINLOCK(scm_query_lock);
>   
>   struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void)
>   {
> -	return __scm->mempool;
> +	return __scm ? __scm->mempool : NULL;
>   }
>   
>   static enum qcom_scm_convention __get_convention(void)


Tested-by: Rudraksha Gupta <guptarud@gmail.com>

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Rudraksha Gupta]

On 9/9/24 11:38, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Older platforms don't have an actual SCM device tied into the driver
> model and so there's no struct device which to use with the TZ Mem API.
> We need to fall-back to kcalloc() when allocating the buffer for
> additional SMC arguments on such platforms which don't even probe the SCM
> driver and never create the TZMem pool.
>
> Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/<S-Del>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> index 2b4c2826f572..13f72541033c 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> @@ -147,6 +147,15 @@ static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
>   	return 0;
>   }
>   
> +static void smc_args_free(void *ptr)
> +{
> +	if (qcom_scm_get_tzmem_pool())
> +		qcom_tzmem_free(ptr);
> +	else
> +		kfree(ptr);
> +}
> +
> +DEFINE_FREE(smc_args, void *, if (!IS_ERR_OR_NULL(_T)) smc_args_free(_T));
>   
>   int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>   		   enum qcom_scm_convention qcom_convention,
> @@ -155,7 +164,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
>   	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;
> +	void *args_virt __free(smc_args) = NULL;
>   	gfp_t flag = atomic ? GFP_ATOMIC : GFP_KERNEL;
>   	u32 smccc_call_type = atomic ? ARM_SMCCC_FAST_CALL : ARM_SMCCC_STD_CALL;
>   	u32 qcom_smccc_convention = (qcom_convention == SMC_CONVENTION_ARM_32) ?
> @@ -173,9 +182,20 @@ 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)) {
> -		args_virt = qcom_tzmem_alloc(mempool,
> -					     SCM_SMC_N_EXT_ARGS * sizeof(u64),
> -					     flag);
> +		/*
> +		 * Older platforms don't have an entry for SCM in device-tree
> +		 * and so no device is bound to the SCM driver. This means there
> +		 * is no struct device for the TZ Mem API. Fall back to
> +		 * kcalloc() on such platforms.
> +		 */
> +		if (mempool)
> +			args_virt = qcom_tzmem_alloc(
> +					mempool,
> +					SCM_SMC_N_EXT_ARGS * sizeof(u64),
> +					flag);
> +		else
> +			args_virt = kcalloc(SCM_SMC_N_EXT_ARGS, sizeof(u64),
> +					    flag);
>   		if (!args_virt)
>   			return -ENOMEM;
>   
>

Tested-by: Rudraksha Gupta <guptarud@gmail.com>

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Bartosz Golaszewski]

On Mon, Sep 9, 2024 at 11:04 PM Elliot Berman <quic_eberman@quicinc.com> wrote:
>
> On Mon, Sep 09, 2024 at 08:38:45PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Older platforms don't have an actual SCM device tied into the driver
> > model and so there's no struct device which to use with the TZ Mem API.
> > We need to fall-back to kcalloc() when allocating the buffer for
> > additional SMC arguments on such platforms which don't even probe the SCM
> > driver and never create the TZMem pool.
> >
> > Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> > Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> > Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/<S-Del>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > index 2b4c2826f572..13f72541033c 100644
> > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > @@ -147,6 +147,15 @@ static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
> >       return 0;
> >  }
> >
> > +static void smc_args_free(void *ptr)
> > +{
> > +     if (qcom_scm_get_tzmem_pool())
>
> I'm a little concerned about this check. I didn't think making SCM calls
> without the SCM device probed was possible until this report. We do
> worry about that in the downstream kernel. So, I'm not sure if this
> scenario is currently possible in the upstream kernel.

I didn't know about this either. And I think this should be fixed.

That being said: qcom-msm8960.dtsi doesn't have any SCM node and
there's no such compatible in the driver so it looks real.

Also: some of the calls seem to be ready for this and they check
whether __scm is set and use a NULL struct device pointer if not.

>
> It's possible that some driver makes SCM call in parallel to SCM device
> probing. Then, it might be possible for qcom_scm_get_tzmem_pool() to
> return NULL at beginning of function and then a valid pointer by the
> time we're freeing the ptr.
>

I thought the SCM module is initialized at subsys_initcall() exactly
for that reason? Because if the above can happen then we have more
problems than just that. What if we enable SHM bridge during probe?
I'm not even sure what would happen to SCM calls in progress that were
passed regular buffers before.

I think the SCM driver should be improved and not allow calls until it's probed.

Bart

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Dmitry Baryshkov]

On Tue, 10 Sept 2024 at 00:04, Elliot Berman <quic_eberman@quicinc.com> wrote:
>
> On Mon, Sep 09, 2024 at 08:38:45PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Older platforms don't have an actual SCM device tied into the driver
> > model and so there's no struct device which to use with the TZ Mem API.
> > We need to fall-back to kcalloc() when allocating the buffer for
> > additional SMC arguments on such platforms which don't even probe the SCM
> > driver and never create the TZMem pool.
> >
> > Fixes: 449d0d84bcd8 ("firmware: qcom: scm: smc: switch to using the SCM allocator")
> > Reported-by: Rudraksha Gupta <guptarud@gmail.com>
> > Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@gmail.com/<S-Del>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > index 2b4c2826f572..13f72541033c 100644
> > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > @@ -147,6 +147,15 @@ static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
> >       return 0;
> >  }
> >
> > +static void smc_args_free(void *ptr)
> > +{
> > +     if (qcom_scm_get_tzmem_pool())
>
> I'm a little concerned about this check. I didn't think making SCM calls
> without the SCM device probed was possible until this report. We do
> worry about that in the downstream kernel. So, I'm not sure if this
> scenario is currently possible in the upstream kernel.

MSM8960 and MSM8660 don't have SCM devices. For MSM8960 it should be
trivial to get it, c&p from apq8064 should. For MSM8660 it might be a
bit harder. But even if we add such nodes, we shouldn't break existing
DT files.

> It's possible that some driver makes SCM call in parallel to SCM device
> probing. Then, it might be possible for qcom_scm_get_tzmem_pool() to
> return NULL at beginning of function and then a valid pointer by the
> time we're freeing the ptr.


-- 
With best wishes
Dmitry

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Bartosz Golaszewski]

On Tue, Sep 10, 2024 at 1:06 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> >
> > I'm a little concerned about this check. I didn't think making SCM calls
> > without the SCM device probed was possible until this report. We do
> > worry about that in the downstream kernel. So, I'm not sure if this
> > scenario is currently possible in the upstream kernel.
>
> MSM8960 and MSM8660 don't have SCM devices. For MSM8960 it should be
> trivial to get it, c&p from apq8064 should. For MSM8660 it might be a
> bit harder. But even if we add such nodes, we shouldn't break existing
> DT files.
>

I'm wondering about how to approach an eventual refactoring and I'm
thinking that for platforms that are known to have DTs out there
without the node, we could exceptionally instantiate the SCM device
when the module is loaded? And then modify the driver to require the
provider to have an actual struct device attached.

Bart

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Dmitry Baryshkov]

On Wed, Sep 11, 2024 at 01:41:58PM GMT, Bartosz Golaszewski wrote:
> On Tue, Sep 10, 2024 at 1:06 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > >
> > > I'm a little concerned about this check. I didn't think making SCM calls
> > > without the SCM device probed was possible until this report. We do
> > > worry about that in the downstream kernel. So, I'm not sure if this
> > > scenario is currently possible in the upstream kernel.
> >
> > MSM8960 and MSM8660 don't have SCM devices. For MSM8960 it should be
> > trivial to get it, c&p from apq8064 should. For MSM8660 it might be a
> > bit harder. But even if we add such nodes, we shouldn't break existing
> > DT files.
> >
> 
> I'm wondering about how to approach an eventual refactoring and I'm
> thinking that for platforms that are known to have DTs out there
> without the node, we could exceptionally instantiate the SCM device
> when the module is loaded? And then modify the driver to require the
> provider to have an actual struct device attached.

Might make sense. We should be able to test in on APQ8060 = MSM8660.

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Rudraksha Gupta]

> I'm wondering about how to approach an eventual refactoring and I'm
> thinking that for platforms that are known to have DTs out there
> without the node, we could exceptionally instantiate the SCM device
> when the module is loaded? And then modify the driver to require the
> provider to have an actual struct device attached.


I'm happy to help test these changes if you'd like!

Re: [PATCH 2/2] firmware: qcom: scm: fall back to kcalloc() for no SCM device bound

[Bartosz Golaszewski]

On Wed, Sep 11, 2024 at 8:01 PM Rudraksha Gupta <guptarud@gmail.com> wrote:
>
> > I'm wondering about how to approach an eventual refactoring and I'm
> > thinking that for platforms that are known to have DTs out there
> > without the node, we could exceptionally instantiate the SCM device
> > when the module is loaded? And then modify the driver to require the
> > provider to have an actual struct device attached.
>
>
> I'm happy to help test these changes if you'd like!
>

Thanks! In any case, this series should still be merged to not break
existing users (even if the kcalloc() fallback will be removed once we
do the refactoring).

Bart