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

[Bartosz Golaszewski]

On Mon, Sep 16, 2024 at 6:44 PM Stephan Gerhold
<stephan.gerhold@xxxxxxxxxx> wrote:
>
> On Wed, Sep 11, 2024 at 11:07:04AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Closes: https://lore.kernel.org/lkml/692cfe9a-8c05-4ce4-813e-82b3f310019a@xxxxxxxxx/
> > Tested-by: Rudraksha Gupta <guptarud@xxxxxxxxx>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
> >  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