From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3D22630F95B for ; Tue, 14 Oct 2025 08:24:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760430275; cv=none; b=dAfWNXt4NsKG9WjCTcliamHL+Kc/vn7/eypbdEiwHbw+l/umcINHiOLceUaJBLKWsZETiAqP75yIGyH5dT8NKtAK19/IoD3byrnQf69kC50osStt5/Z2T8Mn3MLkbQ6Dv6LRhDOGB4F//u8KSR1U1wFWgtOGU28N5Cc7QEfAyAY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1760430275; c=relaxed/simple; bh=omBQDlN0SNSXKstrq0O2kRGJ7zOFZ+fJkN3e0IgV1JA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=RrCy2P2FPozWUjdjRNvNy6HzqfVNeiebLTLjdPdnhVpwU0KP8dDf7vcS37DGTXHKHzSXPEW1X3PIIhaKlQgTt+VgJtZhGLH846dgLUfdXlsr5DIghz+MEv0xizu5eGnKZKNR7aRheKh+cBAg1m+jqh3h/3Qr5ePQXH61vW52CZs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=xZk8uUNb; arc=none smtp.client-ip=209.85.128.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="xZk8uUNb" Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-46e491a5b96so27583135e9.2 for ; Tue, 14 Oct 2025 01:24:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1760430271; x=1761035071; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=g1WLMyC6KZHho1k1eIZ+qKx3AxLZ35CnRGvjdbXTrhs=; b=xZk8uUNbzSr2yOOstpXtq1eHNwsSuz1u652GVVYIqCMjBSQz4i7AT3eiodhRppomfB iwaqArrGX65/YLIGHHJx90TsQOKe9LsH0azL+jYb3wFT/pzTSaWxsFleqA1aHktVzwEx +MoqVC99YfFpHQ4+3wass/C3uKra9YOnlb7ctBQ9Q7R6l4EYJM4UJyZvwToHkF9Y2Lha O8oBPXJ3+2f5I+zUbxOSSKIRT0w0Z292skf8hfEIKs1MPP6xFdngUfgxAGLG6Qoa470W OXGP66IR5gAL4AGRF197Hjishz9d3L6/CMSTr9efF6VHEDC95bI8/fSkW28qE3ZtUsDM jq9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760430271; x=1761035071; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=g1WLMyC6KZHho1k1eIZ+qKx3AxLZ35CnRGvjdbXTrhs=; b=hkEP903yhkt+44oKocmcGrm6acvFBCMvO7asxFalatsHgxVM+8gGLdmPZ8gV+SnNh8 /fzYN6vTxX93lUkYs6LF99zQCFRl/XNSBDmq3I/r7YbS8mthFWfjYInBU+zBcZTF+V1J 8YbTf4StyokHXV+j4dRKnReDAzkE1rDCBBRXmH0SYXpajHI/J5X37w5z/Wz+PtuDAB49 MVUuJXs/pobPC/ypvSaVMoH/FGxFWJmVVwusJiYzYLTuAL2nJyBjmRCBk/AWrEN5uQM4 lFIcvKkvlBx9uSuMMyqqZdn/6JlB5lbO5hSmZj3Vr3n+kg9nnfteBeGv38CnOrP+lDid oQIg== X-Forwarded-Encrypted: i=1; AJvYcCXXyfFWb9aXrufIWPd8BI3e5xj+jY2if1A2QLuCsXIT4D/OME1PlyoDyLeCWpWoFTEcX5wEYSIczfp7@vger.kernel.org X-Gm-Message-State: AOJu0YyhoU85qALuFn+Z5yNrYoShOGEQgkYDIIWbxkATD4NnmEuDIKpR iMkPbVGBiwUDa8J3s+oGfxBfdDfiEnttULTMGWTbh1pDkVzFqyg4JUnTPJVp8LOdLkU= X-Gm-Gg: ASbGncuo2uZienSYjtSl4qK81G9oZMePtdReILjo3u9a9QMq/3TLSLLFzFTOqpZciey GKjBvu6JF9tMtjkCm/wj6+4kFXuhijMHO1XbUtDXvY6hjIN5HHSrsQi6kGNcqGkJuhLvRsgQzie tIw2fhFuVw/R+3ULYElTEeeoqCJyQjUZj2DuvsDVJcoh7zFF/FS7GMe/X93ITlytR8cnQz7DzCF oYCEKRqseQWvOWk1aBnY9wDr4vCLLoDriFbeEmElqOTsk3gx/2Mh9GydaoE084CXT9YJPVNZwnj 4msN3nrIobHLfPEWNPVn5P32V0ZXMZToAEOwEuCZxU4bqjJSfaUdMLeDqLPpFlLlK52reJ4tHmM rtBPG1Dayc70HUWySOJriSL6ebyF3QkfF7GHNtsW6znkrWLfkfQ3/cNCA49rFpU9Lk8QI64/Wg+ zCpWojYffFg5Y= X-Google-Smtp-Source: AGHT+IF2nh8wX/8xS6xnvjff0xiiobDKtkMDK+CZSyPFZvqZoCQ8OqTQRSGwMzPpVRnLz4p9dqoSOg== X-Received: by 2002:a05:600c:6818:b0:46e:36fa:6b40 with SMTP id 5b1f17b1804b1-46fa9af915emr162139705e9.24.1760430270637; Tue, 14 Oct 2025 01:24:30 -0700 (PDT) Received: from [192.168.0.19] (188-141-3-146.dynamic.upc.ie. [188.141.3.146]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-426dac0ab7bsm13291015f8f.46.2025.10.14.01.24.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Oct 2025 01:24:30 -0700 (PDT) Message-ID: <27b9a906-3348-4b75-b5e4-12edad91b93f@linaro.org> Date: Tue, 14 Oct 2025 09:24:27 +0100 Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 04/13] remoteproc: pas: Replace metadata context with PAS context structure To: Mukesh Ojha , Bjorn Andersson , Mathieu Poirier , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Manivannan Sadhasivam , Konrad Dybcio Cc: linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <20251013-kvm_rprocv5-v5-0-d609ed766061@oss.qualcomm.com> <20251013-kvm_rprocv5-v5-4-d609ed766061@oss.qualcomm.com> From: Bryan O'Donoghue Content-Language: en-US In-Reply-To: <20251013-kvm_rprocv5-v5-4-d609ed766061@oss.qualcomm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 13/10/2025 11:03, Mukesh Ojha wrote: > As a superset of the existing metadata context, the PAS context > structure enables both remoteproc and non-remoteproc subsystems to > better support scenarios where the SoC runs with or without the Gunyah > hypervisor. To reflect this, relevant SCM and metadata functions are > updated to incorporate PAS context awareness. > > Signed-off-by: Mukesh Ojha > --- > drivers/firmware/qcom/qcom_scm.c | 32 ++++++++++++++++------------- > drivers/remoteproc/qcom_q6v5_pas.c | 37 ++++++++++++++++++++++++---------- > drivers/soc/qcom/mdt_loader.c | 4 ++-- > include/linux/firmware/qcom/qcom_scm.h | 4 ++-- > include/linux/soc/qcom/mdt_loader.h | 6 +++--- > 5 files changed, 51 insertions(+), 32 deletions(-) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 6d22b2ac7880..b11a21797d46 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -600,7 +600,7 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_context_init); > * and optional blob of data used for authenticating the metadata > * and the rest of the firmware > * @size: size of the metadata > - * @ctx: optional metadata context > + * @ctx: optional pas context > * > * Return: 0 on success. > * > @@ -609,8 +609,9 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_context_init); > * qcom_scm_pas_metadata_release() by the caller. > */ > int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size, > - struct qcom_scm_pas_metadata *ctx) > + struct qcom_scm_pas_context *ctx) > { > + struct qcom_scm_pas_metadata *mdt_ctx; > dma_addr_t mdata_phys; > void *mdata_buf; > int ret; > @@ -661,10 +662,11 @@ int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size, > out: > if (ret < 0 || !ctx) { > dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys); > - } else if (ctx) { > - ctx->ptr = mdata_buf; > - ctx->phys = mdata_phys; > - ctx->size = size; > + } else if (ctx && ctx->metadata) { > + mdt_ctx = ctx->metadata; > + mdt_ctx->ptr = mdata_buf; > + mdt_ctx->phys = mdata_phys; > + mdt_ctx->size = size; Suspicious looking code.. The second check for ctx is redundant as it cannot ever be false. You have if (ret < 0 || !ctx) { } else if (ctx && ctx->mdatadata) { } The old code was wrong but, that's no reason to continue to be wrong. Is it currently possible for ctx to be true but ctx->metadata to be false.. void *qcom_scm_pas_context_init(struct device *dev, u32 pas_id, phys_addr_t mem_phys, size_t mem_size) { struct qcom_scm_pas_context *ctx; ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); if (!ctx) return ERR_PTR(-ENOMEM); ctx->dev = dev; ctx->pas_id = pas_id; ctx->mem_phys = mem_phys; ctx->mem_size = mem_size; ctx->metadata = devm_kzalloc(dev, sizeof(*ctx->metadata), GFP_KERNEL); if (!ctx->metadata) return ERR_PTR(-ENOMEM); return ctx; } EXPORT_SYMBOL_GPL(qcom_scm_pas_context_init); No. Lets fix the ctx checking logic in this code as we modify the patch. > } > > return ret ? : res.result[0]; > @@ -673,18 +675,20 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image); > > /** > * qcom_scm_pas_metadata_release() - release metadata context > - * @ctx: metadata context > + * @ctx: pas context > */ > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx) > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_context *ctx) > { > - if (!ctx->ptr) > - return; > + struct qcom_scm_pas_metadata *mdt_ctx; > > - dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys); > + mdt_ctx = ctx->metadata; > + if (!mdt_ctx->ptr) > + return; > > - ctx->ptr = NULL; > - ctx->phys = 0; > - ctx->size = 0; > + dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr, mdt_ctx->phys); > + mdt_ctx->ptr = NULL; > + mdt_ctx->phys = 0; > + mdt_ctx->size = 0; > } If we have established that mdt_ctx->ptr is the fulcurm of truth for this data then setting ->phys and ->size to anything after setting ->ptr = NULL are wasted bus cycles. > EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release); > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c > index 158bcd6cc85c..e9dcab94ea0c 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > @@ -117,8 +117,8 @@ struct qcom_pas { > struct qcom_rproc_ssr ssr_subdev; > struct qcom_sysmon *sysmon; > > - struct qcom_scm_pas_metadata pas_metadata; > - struct qcom_scm_pas_metadata dtb_pas_metadata; > + struct qcom_scm_pas_context *pas_ctx; > + struct qcom_scm_pas_context *dtb_pas_ctx; > }; > > static void qcom_pas_segment_dump(struct rproc *rproc, > @@ -211,9 +211,9 @@ static int qcom_pas_unprepare(struct rproc *rproc) > * auth_and_reset() was successful, but in other cases clean it up > * here. > */ > - qcom_scm_pas_metadata_release(&pas->pas_metadata); > + qcom_scm_pas_metadata_release(pas->pas_ctx); > if (pas->dtb_pas_id) > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx); > > return 0; > } > @@ -241,7 +241,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw) > > ret = qcom_mdt_pas_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name, > pas->dtb_pas_id, pas->dtb_mem_phys, > - &pas->dtb_pas_metadata); > + pas->dtb_pas_ctx); > if (ret) > goto release_dtb_firmware; > > @@ -255,7 +255,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw) > return 0; > > release_dtb_metadata: > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx); > > release_dtb_firmware: > release_firmware(pas->dtb_firmware); > @@ -306,7 +306,7 @@ static int qcom_pas_start(struct rproc *rproc) > } > > ret = qcom_mdt_pas_init(pas->dev, pas->firmware, rproc->firmware, pas->pas_id, > - pas->mem_phys, &pas->pas_metadata); > + pas->mem_phys, pas->pas_ctx); > if (ret) > goto disable_px_supply; > > @@ -332,9 +332,9 @@ static int qcom_pas_start(struct rproc *rproc) > goto release_pas_metadata; > } > > - qcom_scm_pas_metadata_release(&pas->pas_metadata); > + qcom_scm_pas_metadata_release(pas->pas_ctx); > if (pas->dtb_pas_id) > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx); > > /* firmware is used to pass reference from qcom_pas_start(), drop it now */ > pas->firmware = NULL; > @@ -342,9 +342,9 @@ static int qcom_pas_start(struct rproc *rproc) > return 0; > > release_pas_metadata: > - qcom_scm_pas_metadata_release(&pas->pas_metadata); > + qcom_scm_pas_metadata_release(pas->pas_ctx); > if (pas->dtb_pas_id) > - qcom_scm_pas_metadata_release(&pas->dtb_pas_metadata); > + qcom_scm_pas_metadata_release(pas->dtb_pas_ctx); > disable_px_supply: > if (pas->px_supply) > regulator_disable(pas->px_supply); > @@ -779,6 +779,21 @@ static int qcom_pas_probe(struct platform_device *pdev) > } > > qcom_add_ssr_subdev(rproc, &pas->ssr_subdev, desc->ssr_name); > + > + pas->pas_ctx = qcom_scm_pas_context_init(pas->dev, pas->pas_id, pas->mem_phys, > + pas->mem_size); > + if (IS_ERR(pas->pas_ctx)) { > + ret = PTR_ERR(pas->pas_ctx); > + goto remove_ssr_sysmon; > + } > + > + pas->dtb_pas_ctx = qcom_scm_pas_context_init(pas->dev, pas->dtb_pas_id, > + pas->dtb_mem_phys, pas->dtb_mem_size); > + if (IS_ERR(pas->dtb_pas_ctx)) { > + ret = PTR_ERR(pas->dtb_pas_ctx); > + goto remove_ssr_sysmon; > + } > + > ret = rproc_add(rproc); > if (ret) > goto remove_ssr_sysmon; > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c > index a5c80d4fcc36..fe35038c5342 100644 > --- a/drivers/soc/qcom/mdt_loader.c > +++ b/drivers/soc/qcom/mdt_loader.c > @@ -234,13 +234,13 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata); > * @fw_name: name of the firmware, for construction of segment file names > * @pas_id: PAS identifier > * @mem_phys: physical address of allocated memory region > - * @ctx: PAS metadata context, to be released by caller > + * @ctx: PAS context, ctx->metadata to be released by caller > * > * Returns 0 on success, negative errno otherwise. > */ > int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw, > const char *fw_name, int pas_id, phys_addr_t mem_phys, > - struct qcom_scm_pas_metadata *ctx) > + struct qcom_scm_pas_context *ctx) > { > const struct elf32_phdr *phdrs; > const struct elf32_phdr *phdr; > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h > index 75dec515c5d2..7c58d26ede24 100644 > --- a/include/linux/firmware/qcom/qcom_scm.h > +++ b/include/linux/firmware/qcom/qcom_scm.h > @@ -83,8 +83,8 @@ struct qcom_scm_pas_context { > void *qcom_scm_pas_context_init(struct device *dev, u32 pas_id, phys_addr_t mem_phys, > size_t mem_size); > int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size, > - struct qcom_scm_pas_metadata *ctx); > -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx); > + struct qcom_scm_pas_context *ctx); > +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_context *ctx); > int qcom_scm_pas_mem_setup(u32 pas_id, phys_addr_t addr, phys_addr_t size); > int qcom_scm_pas_auth_and_reset(u32 pas_id); > int qcom_scm_pas_shutdown(u32 pas_id); > diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h > index 8ea8230579a2..07c278841816 100644 > --- a/include/linux/soc/qcom/mdt_loader.h > +++ b/include/linux/soc/qcom/mdt_loader.h > @@ -10,14 +10,14 @@ > > struct device; > struct firmware; > -struct qcom_scm_pas_metadata; > +struct qcom_scm_pas_context; > > #if IS_ENABLED(CONFIG_QCOM_MDT_LOADER) > > ssize_t qcom_mdt_get_size(const struct firmware *fw); > int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw, > const char *fw_name, int pas_id, phys_addr_t mem_phys, > - struct qcom_scm_pas_metadata *pas_metadata_ctx); > + struct qcom_scm_pas_context *pas_ctx); > int qcom_mdt_load(struct device *dev, const struct firmware *fw, > const char *fw_name, int pas_id, void *mem_region, > phys_addr_t mem_phys, size_t mem_size, > @@ -39,7 +39,7 @@ static inline ssize_t qcom_mdt_get_size(const struct firmware *fw) > > static inline int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw, > const char *fw_name, int pas_id, phys_addr_t mem_phys, > - struct qcom_scm_pas_metadata *pas_metadata_ctx) > + struct qcom_scm_pas_context *pas_ctx) > { > return -ENODEV; > } >