Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <248ebc1b-d077-e424-8f36-6c143b0ed697@gmail.com>
Date:   Wed, 24 May 2023 13:17:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH 01/13] firmware: qcom: scm: Introduce pas_metadata context
To:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>
Cc:     Rob Herring <robh+dt@kernel.org>, linux-arm-msm@vger.kernel.org,
        linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Bjorn Andersson <andersson@kernel.org>
References: <20220128025513.97188-1-bjorn.andersson@linaro.org>
 <20220128025513.97188-2-bjorn.andersson@linaro.org>
Content-Language: en-US
From:   Robert Marko <robimarko@gmail.com>
In-Reply-To: <20220128025513.97188-2-bjorn.andersson@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 28. 01. 2022. 03:55, Bjorn Andersson wrote:
> Starting with Qualcomm SM8450, some new security enhancements has been
> done in the secure world, which results in the requirement to keep the
> metadata segment accessible by the secure world from init_image() until
> auth_and_reset().
>
> Introduce a "PAS metadata context" object that can be passed to
> init_image() for tracking the mapped memory and a related release
> function for client drivers to release the mapping once either
> auth_and_reset() has been invoked or in error handling paths on the way
> there.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Hi Bjorn,
I know a lot of time has passed since this patch, but this patch breaks 
qcom_scm_pas_auth_and_reset SCM call.
I have had MDT loader and SCM changes from this series reverted for a 
long time but was unable to
find the exact culprit nor how to fix it.
I even ported the TZ log driver to see if anything is obvious there but 
nope, calling qcom_scm_pas_auth_and_reset
after this patch is just failling which is then causing Q6 WCSS on 
IPQ8074 to fail probing so WLAN doesnt work.

Since I am out of ideas, I was hoping that you maybe have an idea what 
could be the issue, I can provide the TZ log
with all of the SCM calls when it works and when not if required.

Regards,
Robert

> ---
>   drivers/firmware/qcom_scm.c   | 39 ++++++++++++++++++++++++++++++-----
>   drivers/soc/qcom/mdt_loader.c |  2 +-
>   include/linux/qcom_scm.h      | 10 ++++++++-
>   3 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 7db8066b19fd..3218d13cbf83 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -435,10 +435,16 @@ static void qcom_scm_set_download_mode(bool enable)
>    *		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
>    *
> - * Returns 0 on success.
> + * Return: 0 on success.
> + *
> + * Upon successful return, the PAS metadata context (@ctx) will be used to
> + * track the metadata allocation, this needs to be released by invoking
> + * qcom_scm_pas_metadata_release() by the caller.
>    */
> -int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
> +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;
>   	void *mdata_buf;
> @@ -467,7 +473,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>   
>   	ret = qcom_scm_clk_enable();
>   	if (ret)
> -		goto free_metadata;
> +		goto out;
>   
>   	desc.args[1] = mdata_phys;
>   
> @@ -475,13 +481,36 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>   
>   	qcom_scm_clk_disable();
>   
> -free_metadata:
> -	dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> +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;
> +	}
>   
>   	return ret ? : res.result[0];
>   }
>   EXPORT_SYMBOL(qcom_scm_pas_init_image);
>   
> +/**
> + * qcom_scm_pas_metadata_release() - release metadata context
> + * @ctx:	metadata context
> + */
> +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);
> +
> +	ctx->ptr = NULL;
> +	ctx->phys = 0;
> +	ctx->size = 0;
> +}
> +EXPORT_SYMBOL(qcom_scm_pas_metadata_release);
> +
>   /**
>    * qcom_scm_pas_mem_setup() - Prepare the memory related to a given peripheral
>    *			      for firmware loading
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 72fc2b539213..b00586db5391 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -171,7 +171,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>   			goto out;
>   		}
>   
> -		ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len);
> +		ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, NULL);
>   
>   		kfree(metadata);
>   		if (ret) {
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 81cad9e1e412..4d8371410b05 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -68,8 +68,16 @@ extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
>   extern void qcom_scm_cpu_power_down(u32 flags);
>   extern int qcom_scm_set_remote_state(u32 state, u32 id);
>   
> +struct qcom_scm_pas_metadata {
> +	void *ptr;
> +	dma_addr_t phys;
> +	ssize_t size;
> +};
> +
>   extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
> -				   size_t size);
> +				   size_t size,
> +				   struct qcom_scm_pas_metadata *ctx);
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
>   extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
>   				  phys_addr_t size);
>   extern int qcom_scm_pas_auth_and_reset(u32 peripheral);