On Mon, Oct 02, 2023 at 08:24:09AM -0500, Andrew Halaney wrote:
> On Fri, Sep 29, 2023 at 03:48:37PM -0700, Elliot Berman wrote:
> > Hi Andrew,
> >
> > On 9/29/2023 1:44 PM, Andrew Halaney wrote:
> > > On Fri, Sep 29, 2023 at 12:22:16PM -0700, Bartosz Golaszewski wrote:
> > >> On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@xxxxxxxxxx> said:
> > >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote:
> > >>>> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >>>>
> > >>>> Let's use the new SCM memory allocator to obtain a buffer for this call
> > >>>> instead of using dma_alloc_coherent().
> > >>>>
> > >>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >>>> ---
> > >>>> drivers/firmware/qcom/qcom_scm.c | 16 +++++-----------
> > >>>> 1 file changed, 5 insertions(+), 11 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > >>>> index 02a773ba1383..c0eb81069847 100644
> > >>>> --- a/drivers/firmware/qcom/qcom_scm.c
> > >>>> +++ b/drivers/firmware/qcom/qcom_scm.c
> > >>>> @@ -532,7 +532,7 @@ static void qcom_scm_set_download_mode(bool enable)
> > >>>> int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > >>>> struct qcom_scm_pas_metadata *ctx)
> > >>>> {
> > >>>> - dma_addr_t mdata_phys;
> > >>>> + phys_addr_t mdata_phys;
> > >>>
> > >>>> void *mdata_buf;
> > >>>> int ret;
> > >>>> struct qcom_scm_desc desc = {
> > >>>> @@ -544,13 +544,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> > >>>> };
> > >>>> struct qcom_scm_res res;
> > >>>>
> > >>>> - /*
> > >>>> - * During the scm call memory protection will be enabled for the meta
> > >>>> - * data blob, so make sure it's physically contiguous, 4K aligned and
> > >>>> - * non-cachable to avoid XPU violations.
> > >>>> - */
> > >>>> - mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
> > >>>> - GFP_KERNEL);
> > >>>> + mdata_buf = qcom_scm_mem_alloc(size, GFP_KERNEL);
> > >>>
> > >>> mdata_phys is never initialized now, and its what's being shoved into
> > >>> desc.args[1] later, which I believe is what triggered the -EINVAL
> > >>> with qcom_scm_call() that I reported in my cover letter reply this
> > >>> morning.
> > >>>
> > >>> Prior with the DMA API that would have been the device view of the buffer.
> > >>>
> > >>
> > >> Gah! Thanks for finding this.
> > >>
> > >> Can you try the following diff?
> > >>
> > >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > >> index 794388c3212f..b0d4ea237034 100644
> > >> --- a/drivers/firmware/qcom/qcom_scm.c
> > >> +++ b/drivers/firmware/qcom/qcom_scm.c
> > >> @@ -556,6 +556,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> > >> void *metadata, size_t size,
> > >> dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
> > >> return -ENOMEM;
> > >> }
> > >> + mdata_phys = qcom_scm_mem_to_phys(mdata_buf);
> > >> memcpy(mdata_buf, metadata, size);
> > >>
> > >> ret = qcom_scm_clk_enable();
> > >> @@ -578,7 +579,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const
> > >> void *metadata, size_t size,
> > >> qcom_scm_mem_free(mdata_buf);
> > >> } else if (ctx) {
> > >> ctx->ptr = mdata_buf;
> > >> - ctx->phys = qcom_scm_mem_to_phys(mdata_buf);
> > >> + ctx->phys = mdata_phys;
> > >> ctx->size = size;
> > >> }
> > >>
> > >> Bart
> > >>
> > >
> > > For some reason that I can't explain that is still not working. It seems
> > > the SMC call is returning !0 and then we return -EINVAL from there
> > > with qcom_scm_remap_error().
> > >
> > > Here's a really crummy diff of what I hacked in during lunch to debug (don't
> > > judge my primitive debug skills):
> > >
> >
> > I don't know what you're talking about :-)
> >
> > > diff --git a/drivers/firmware/qcom/qcom_scm-smc.c b/drivers/firmware/qcom/qcom_scm-smc.c
> > > index 0d5554df1321..56eab0ae5f3a 100644
> > > --- a/drivers/firmware/qcom/qcom_scm-smc.c
> > > +++ b/drivers/firmware/qcom/qcom_scm-smc.c
> > > @@ -162,6 +162,8 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > struct arm_smccc_res smc_res;
> > > struct arm_smccc_args smc = {0};
> > >
> > > + dev_err(dev, "%s: %d: We are in this function\n", __func__, __LINE__);
> > > +
> > > smc.args[0] = ARM_SMCCC_CALL_VAL(
> > > smccc_call_type,
> > > qcom_smccc_convention,
> > > @@ -174,6 +176,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > if (unlikely(arglen > SCM_SMC_N_REG_ARGS)) {
> > > alloc_len = SCM_SMC_N_EXT_ARGS * sizeof(u64);
> > > args_virt = qcom_scm_mem_alloc(PAGE_ALIGN(alloc_len), flag);
> > > + dev_err(dev, "%s: %d: Hit the unlikely case!\n", __func__, __LINE__);
> > >
> > > if (!args_virt)
> > > return -ENOMEM;
> > > @@ -197,6 +200,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > >
> > > /* ret error check follows after args_virt cleanup*/
> > > ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
> > > + dev_err(dev, "%s: %d: ret: %d\n", __func__, __LINE__, ret);
> > >
> > > if (ret)
> > > return ret;
> > > @@ -205,8 +209,10 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
> > > res->result[0] = smc_res.a1;
> > > res->result[1] = smc_res.a2;
> > > res->result[2] = smc_res.a3;
> > > + dev_err(dev, "%s: %d: 0: %llu, 1: %llu: 2: %llu\n", __func__, __LINE__, res->result[0], res->result[1], res->result[2]);
> > > }
> > >
> > > + dev_err(dev, "%s: %d: smc_res.a0: %lu\n", __func__, __LINE__, smc_res.a0);
> > > return (long)smc_res.a0 ? qcom_scm_remap_error(smc_res.a0) : 0;
> > >
> > >
> > > And that all spams dmesg successfully for most cases, but the
> > > pas_init_image calls log this out:
> > >
> > > [ 16.362965] remoteproc remoteproc1: powering up 1b300000.remoteproc
> > > [ 16.364897] remoteproc remoteproc1: Booting fw image qcom/sc8280xp/LENOVO/21BX/qccdsp8280.mbn, size 3575808
> > > [ 16.365009] qcom_scm firmware:scm: __scm_smc_call: 165: We are in this function
> > > [ 16.365251] qcom_scm firmware:scm: __scm_smc_call: 203: ret: 0
> > > [ 16.365256] qcom_scm firmware:scm: __scm_smc_call: 212: 0: 0, 1: 0: 2: 0
> > > [ 16.365261] qcom_scm firmware:scm: __scm_smc_call: 215: smc_res.a0: 4291821558
> > >
> > > At the moment I am unsure why...
> > >
> > Does the issue appear right after taking patch 6 or does it only appear after taking
> > the whole series? If it's just to this patch, then maybe something wrong with
> > the refactor: shm bridge isn't enabled at this point in the series.
> >
>
> I've only been testing the series as a whole on top of a 6.6 based
> branch, I'm going to try and test some more today to see if just the
> allocator bits (but not the SHM bridge enablement) works alright for
> me.
>
After testing a little more with the fix Bart sent above,
the allocator refactor seems to work well.
With "firmware: qcom: scm: enable SHM bridge" applied is when I see the
errors I pointed out above. All prior patches cause no problems on boot
for me.
For patches 1-9 (with the fix sent here for patch 6) please feel free
to add:
Tested-by: Andrew Halaney <ahalaney@xxxxxxxxxx> # sc8280xp-lenovo-thinkpad-x13s
If anyone has suggestions for debugging why the switch to using
SHM bridge is breaking firmware loading for me, I'm willing to give
that a try.
Thanks,
Andrew
