[v2,06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator
Message ID | 20230928092040.9420-7-brgl@bgdev.pl |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp3185578vqu; Thu, 28 Sep 2023 02:39:09 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF71r8W6LV8sbLPoF4kKIXWyxxsp+UwuC8zE6gjDXk2+WcREkpddk3XDcCA/tVRttgmbdXk X-Received: by 2002:a05:6808:14c5:b0:3ab:5e9e:51f1 with SMTP id f5-20020a05680814c500b003ab5e9e51f1mr813249oiw.46.1695893949543; Thu, 28 Sep 2023 02:39:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695893949; cv=none; d=google.com; s=arc-20160816; b=zCsYShuXRaP9dqYtUqXynU8OgKHEltTXTbO4LQglLqyieoRU7NEJl2fu2oO/A6DUH+ 8Xl1hRUIcRtlujKWa5K4WjaKeCxCQMtMV6EEdUgQlrkppvZfH4GzbISTZ6i+AjOoVXRW 68kF3I1C48QHgJ+DLO0Ox4xCXn7a+TXF8e1otYa6VAryupc+y0KWyCK7C5gwV4Q3c/lg VNoDJf2lxLOEBtymalR+7HKy2u39ng+hOtid39IVDU+OdzlKvmRXOZhOInK1BAIU9P46 zyVFAFpbUcoAmU8jn1Ex1dlXI7ciR6rVlv2f/cn2PQPksYo1vdHpGrjtc5jtVv91HqFw G4zg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=nxuIylgD4O5skgVzMv0zVAkT/q/ETWmaZKIykuazMpk=; fh=qciho/xsTxNUsKJZ3hehiaQZTIzl+I3IMsnaV40iMRE=; b=XpCvL9hqst6ZrJxLtiugOIhleX8B/6d7SWetaGAfju14TFQUg9WOBgM99RJDfaNQOZ fS7EQ9cVSm7tcYlGVvmgazH/x7KXpruBI5mYCHuBYoT6/W8GrS1o3UNmyFrMx3muBOdF avO7O/5RYeT5xkikNy7zwosGvOSo0t2TgDU4bRO15hgWJqJJJBJQOpNX63M+SQlZ50qa RSMcoGrCxrCKg8m64bLoSErmHF/obmLLKKrjwjWJK+qvNOFKComeMRS3TGzQm9/kWPPR p1fUFtZBHn6Q8uWMcVHKtnsJDfbz4mLleD7momhW57KtoyEf9ZXtdxklVZctwMPTg9c7 POsw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=QsZQMhMR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id bw8-20020a056a00408800b0068fb95e5401si17602872pfb.65.2023.09.28.02.39.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Sep 2023 02:39:09 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@bgdev-pl.20230601.gappssmtp.com header.s=20230601 header.b=QsZQMhMR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id D059681BDDCA; Thu, 28 Sep 2023 02:21:42 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231723AbjI1JV2 (ORCPT <rfc822;pwkd43@gmail.com> + 21 others); Thu, 28 Sep 2023 05:21:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60720 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231598AbjI1JVJ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 28 Sep 2023 05:21:09 -0400 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5D7BA8E for <linux-kernel@vger.kernel.org>; Thu, 28 Sep 2023 02:21:08 -0700 (PDT) Received: by mail-wm1-x332.google.com with SMTP id 5b1f17b1804b1-40566f8a093so96296255e9.3 for <linux-kernel@vger.kernel.org>; Thu, 28 Sep 2023 02:21:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bgdev-pl.20230601.gappssmtp.com; s=20230601; t=1695892866; x=1696497666; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=nxuIylgD4O5skgVzMv0zVAkT/q/ETWmaZKIykuazMpk=; b=QsZQMhMR3GBlsKJ7Hh7RI2aRGHO/11F+Zwd3LGwooLhEF9xpYgSdM3YZKHCuA3LTH0 pY3AuWOHLJmI3j/pyBmu5avFSn9jqc6wUDK5UmCDZ3vE6OeoC51UGWYj4lz8HHH7zqhv 1Y3FnQImB+qkBxQNrbPKvsRchdc9j+z43X9h/yVD++PpY9SX3O/f6u1OPiYpU5jnX7Jm lw74P8IZDMlNPlk73nRG896X8r4SuyqB8s1bRccr6oINpNlge11A4BVeoMfhqGBWAH+6 Kgqz1kN8uU3gDZxTA7M91vFsL15dZMKb07d1SqRCAAMyFXKoJDiDcw+PeJAeMy4Y9zAe oJbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695892866; x=1696497666; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nxuIylgD4O5skgVzMv0zVAkT/q/ETWmaZKIykuazMpk=; b=rPLl241Zxd+Pt+ZnxzUYJ2Mp/Ew1tFYTzJkMeBidpZDja8d0uGkYBcV/tDTHZHYdGQ dC0EHcD8Rhk+JhxcS+mPMlNfd6ucjsPmDWKOdSK0KW90l5C+/PvDywL9myJ872OMmggJ gkMR/Nl9AHIRWQT9cmLZiwqoPPjXxirvSzj7l6hFMnb0m3VdPiM1Z8poYdZ5PS2L+MIX eivFKxKKuvzwGm9hL+6XoMpuXll9UAa2VM8VELnuuyCckKp+NvqYxzJQd1QoDw1BDKRU JJYdd8yIHtXikjiPSa2XfISDCgRBextILhyjVySwiwNc6tbbCY98s4jqMeVGbQ7hdJKf xA8Q== X-Gm-Message-State: AOJu0Yw7MG9qKJzo4nTTSdIwuXh3PVQoyy63RQ9BFKY+ttItC2ELFOYi 8uulIwuZni8G4kx7AxbGgtglGA== X-Received: by 2002:a05:6000:11c7:b0:321:6005:8979 with SMTP id i7-20020a05600011c700b0032160058979mr787869wrx.17.1695892866569; Thu, 28 Sep 2023 02:21:06 -0700 (PDT) Received: from brgl-uxlite.home ([2a01:cb1d:334:ac00:6e4c:e3c1:5cb8:b60e]) by smtp.gmail.com with ESMTPSA id e9-20020adfe7c9000000b003197efd1e7bsm5009156wrn.114.2023.09.28.02.21.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Sep 2023 02:21:06 -0700 (PDT) From: Bartosz Golaszewski <brgl@bgdev.pl> To: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Maximilian Luz <luzmaximilian@gmail.com>, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, kernel@quicinc.com, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Subject: [PATCH v2 06/11] firmware: qcom: scm: make qcom_scm_pas_init_image() use the SCM allocator Date: Thu, 28 Sep 2023 11:20:35 +0200 Message-Id: <20230928092040.9420-7-brgl@bgdev.pl> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230928092040.9420-1-brgl@bgdev.pl> References: <20230928092040.9420-1-brgl@bgdev.pl> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 28 Sep 2023 02:21:42 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778273694241690314 X-GMAIL-MSGID: 1778273694241690314 |
Series |
arm64: qcom: add and enable SHM Bridge support
|
|
Commit Message
Bartosz Golaszewski
Sept. 28, 2023, 9:20 a.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> 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@linaro.org> --- drivers/firmware/qcom/qcom_scm.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
Comments
On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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@linaro.org> > --- > 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. > if (!mdata_buf) { > dev_err(__scm->dev, "Allocation of metadata buffer failed.\n"); > return -ENOMEM; > @@ -574,10 +568,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size, > > out: > if (ret < 0 || !ctx) { > - dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys); > + qcom_scm_mem_free(mdata_buf); > } else if (ctx) { > ctx->ptr = mdata_buf; > - ctx->phys = mdata_phys; > + ctx->phys = qcom_scm_mem_to_phys(mdata_buf); > ctx->size = size; > } > > @@ -594,7 +588,7 @@ void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx) > if (!ctx->ptr) > return; > > - dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys); > + qcom_scm_mem_free(ctx->ptr); > > ctx->ptr = NULL; > ctx->phys = 0; > -- > 2.39.2 >
On Fri, 29 Sep 2023 21:16:51 +0200, Andrew Halaney <ahalaney@redhat.com> said: > On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> >> 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@linaro.org> >> --- >> 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 >> if (!mdata_buf) { >> dev_err(__scm->dev, "Allocation of metadata buffer failed.\n"); >> return -ENOMEM; >> @@ -574,10 +568,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size, >> >> out: >> if (ret < 0 || !ctx) { >> - dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys); >> + qcom_scm_mem_free(mdata_buf); >> } else if (ctx) { >> ctx->ptr = mdata_buf; >> - ctx->phys = mdata_phys; >> + ctx->phys = qcom_scm_mem_to_phys(mdata_buf); >> ctx->size = size; >> } >> >> @@ -594,7 +588,7 @@ void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx) >> if (!ctx->ptr) >> return; >> >> - dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys); >> + qcom_scm_mem_free(ctx->ptr); >> >> ctx->ptr = NULL; >> ctx->phys = 0; >> -- >> 2.39.2 >> > >
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@redhat.com> said: > > On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: > >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >> > >> 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@linaro.org> > >> --- > >> 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): 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...
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@redhat.com> said: >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>> >>>> 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@linaro.org> >>>> --- >>>> 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.
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@redhat.com> said: > >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: > >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >>>> > >>>> 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@linaro.org> > >>>> --- > >>>> 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. Thanks, Andrew
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@redhat.com> said: > > >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: > > >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > >>>> > > >>>> 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@linaro.org> > > >>>> --- > > >>>> 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@redhat.com> # 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
On Mon, Oct 2, 2023 at 4:15 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > 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@redhat.com> said: > > > >>> On Thu, Sep 28, 2023 at 11:20:35AM +0200, Bartosz Golaszewski wrote: > > > >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > >>>> > > > >>>> 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@linaro.org> > > > >>>> --- > > > >>>> 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@redhat.com> # 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. > Is it possible that sc8280xp doesn't support SHM bridge but for some reason __qcom_scm_is_call_available() returns true when checking if it does? That's the only thing that comes to my mind ATM. Bart
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); if (!mdata_buf) { dev_err(__scm->dev, "Allocation of metadata buffer failed.\n"); return -ENOMEM; @@ -574,10 +568,10 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size, out: if (ret < 0 || !ctx) { - dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys); + qcom_scm_mem_free(mdata_buf); } else if (ctx) { ctx->ptr = mdata_buf; - ctx->phys = mdata_phys; + ctx->phys = qcom_scm_mem_to_phys(mdata_buf); ctx->size = size; } @@ -594,7 +588,7 @@ void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx) if (!ctx->ptr) return; - dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys); + qcom_scm_mem_free(ctx->ptr); ctx->ptr = NULL; ctx->phys = 0;