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

Commit Message

Bartosz Golaszewski Sept. 11, 2024, 9:07 a.m. UTC

From: Bartosz Golaszewski <[e-mail]>

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 <[e-mail]>
Closes: https://lore.kernel.org/lkml/[e-mail]/
Tested-by: Rudraksha Gupta <[e-mail]>
Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
 drivers/firmware/qcom/qcom_scm-smc.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Comments

Stephan Gerhold Sept. 16, 2024, 4:44 p.m. UTC | #1

On Wed, Sep 11, 2024 at 11:07:04AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> 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 <[e-mail]>
> Closes: https://lore.kernel.org/lkml/[e-mail]/
> Tested-by: Rudraksha Gupta <[e-mail]>
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
>  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..88652c38c9a0 100644
> --- a/drivers/firmware/qcom/qcom_scm-smc.c
> +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> [...]
> @@ -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);

I'm afraid this won't work. For kcalloc, we would need to flush the
cache since it returns cached memory. In v6.10 this was done using the
dma_map_single() call that you removed when moving to the tzmem
allocator.

Actually, taking only the first patch in this series should be enough to
fix the crash Rudraksha reported. None of the older platforms should
ever reach into this if statement. I think the rough story is:

 1. The crash Rudraksha reported happens in qcom_scm_set_cold_boot_addr()
    during SMP CPU core boot-up. That code runs very early, AFAIK even
    before the device model is initialized. There is no way to get
    a device pointer at that point. Even if you add the scm node to DT.

 2. AFAIK all the ARM32 platforms without PSCI support implement the
    legacy calling convention (see qcom_scm-legacy.c). They will only
    reach qcom_scm-smc.c once during convention detection (see
    __get_convention()). This is a SCM call with just a single argument
    that won't go inside the if (unlikely(arglen > SCM_SMC_N_REG_ARGS)).
    And qcom_scm-legacy.c does not use the tzmem allocator (yet?).

 3. qcom_scm-legacy.c does use the device pointer for dma_map_single(),
    so it already needs a scm node in the DT. I suspect MSM8960 does not
    hit an error there only because it does not have enough functionality
    enabled to actually reach a non-atomic SCM call. This means: Whoever
    adds that functionality should also add the scm node in the DT.

It would be good to add explicit checks for the device pointer where
needed, instead of crashing. But other than that I think we should be
good with just the first patch of this series?

Thanks,
Stephan

Rudraksha Gupta Sept. 17, 2024, 6:28 a.m. UTC | #2

> Actually, taking only the first patch in this series should be enough to
> fix the crash Rudraksha reported. None of the older platforms should
> ever reach into this if statement. I think the rough story is:

Yep, just tested this right now, and the first patch alone seems to do 
the trick!

Bartosz Golaszewski Sept. 17, 2024, 10:17 a.m. UTC | #3

On Mon, Sep 16, 2024 at 6:44 PM Stephan Gerhold
<[e-mail]> wrote:
>
> On Wed, Sep 11, 2024 at 11:07:04AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > 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 <[e-mail]>
> > Closes: https://lore.kernel.org/lkml/[e-mail]/
> > Tested-by: Rudraksha Gupta <[e-mail]>
> > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> > ---
> >  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..88652c38c9a0 100644
> > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > [...]
> > @@ -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);
>
> I'm afraid this won't work. For kcalloc, we would need to flush the
> cache since it returns cached memory. In v6.10 this was done using the
> dma_map_single() call that you removed when moving to the tzmem
> allocator.
>

Indeed, I missed this but it's not very hard to re-add here.

> Actually, taking only the first patch in this series should be enough to
> fix the crash Rudraksha reported. None of the older platforms should
> ever reach into this if statement. I think the rough story is:
>
>  1. The crash Rudraksha reported happens in qcom_scm_set_cold_boot_addr()
>     during SMP CPU core boot-up. That code runs very early, AFAIK even
>     before the device model is initialized. There is no way to get
>     a device pointer at that point. Even if you add the scm node to DT.
>
>  2. AFAIK all the ARM32 platforms without PSCI support implement the
>     legacy calling convention (see qcom_scm-legacy.c). They will only
>     reach qcom_scm-smc.c once during convention detection (see
>     __get_convention()). This is a SCM call with just a single argument
>     that won't go inside the if (unlikely(arglen > SCM_SMC_N_REG_ARGS)).
>     And qcom_scm-legacy.c does not use the tzmem allocator (yet?).
>

No and I didn't plan to add it. Let me know if I should?

>  3. qcom_scm-legacy.c does use the device pointer for dma_map_single(),
>     so it already needs a scm node in the DT. I suspect MSM8960 does not
>     hit an error there only because it does not have enough functionality
>     enabled to actually reach a non-atomic SCM call. This means: Whoever
>     adds that functionality should also add the scm node in the DT.
>
> It would be good to add explicit checks for the device pointer where
> needed, instead of crashing. But other than that I think we should be
> good with just the first patch of this series?
>

Makes sense to me and with the Tested tag from Rudraksha I guess we
can drop this one.

Bart

Stephan Gerhold Sept. 18, 2024, 8:18 a.m. UTC | #4

On Tue, Sep 17, 2024 at 12:17:19PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 16, 2024 at 6:44 PM Stephan Gerhold
> <[e-mail]> wrote:
> > On Wed, Sep 11, 2024 at 11:07:04AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Closes: https://lore.kernel.org/lkml/[e-mail]/
> > > Tested-by: Rudraksha Gupta <[e-mail]>
> > > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> > > ---
> > >  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..88652c38c9a0 100644
> > > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > > [...]
> > > @@ -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);
> >
> > I'm afraid this won't work. For kcalloc, we would need to flush the
> > cache since it returns cached memory. In v6.10 this was done using the
> > dma_map_single() call that you removed when moving to the tzmem
> > allocator.
> >
> 
> Indeed, I missed this but it's not very hard to re-add here.
> 
> > Actually, taking only the first patch in this series should be enough to
> > fix the crash Rudraksha reported. None of the older platforms should
> > ever reach into this if statement. I think the rough story is:
> >
> >  1. The crash Rudraksha reported happens in qcom_scm_set_cold_boot_addr()
> >     during SMP CPU core boot-up. That code runs very early, AFAIK even
> >     before the device model is initialized. There is no way to get
> >     a device pointer at that point. Even if you add the scm node to DT.
> >
> >  2. AFAIK all the ARM32 platforms without PSCI support implement the
> >     legacy calling convention (see qcom_scm-legacy.c). They will only
> >     reach qcom_scm-smc.c once during convention detection (see
> >     __get_convention()). This is a SCM call with just a single argument
> >     that won't go inside the if (unlikely(arglen > SCM_SMC_N_REG_ARGS)).
> >     And qcom_scm-legacy.c does not use the tzmem allocator (yet?).
> >
> 
> No and I didn't plan to add it. Let me know if I should?
> 

I'm not sure if there is any advantage aside from slightly more
consistent code. None of these old platforms will have support for the
SHM bridge. We would need to test the changes carefully to make sure
there are no regressions. It's probably easier (and safer) to just leave
that code as-is.

Thanks,
Stephan

Patch

diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
index 2b4c2826f572..88652c38c9a0 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 (likely(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;