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
