Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 Jan 2021 18:21:20 -0600
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Siddharth Gupta <sidgup@codeaurora.org>
Cc:     agross@kernel.org, ohad@wizery.com, linux-arm-msm@vger.kernel.org,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        psodagud@codeaurora.org, rishabhb@codeaurora.org
Subject: Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob
Message-ID: <X/elgO+66ibjeL+3@builder.lan>
References: <1609968211-7579-1-git-send-email-sidgup@codeaurora.org>
 <1609968211-7579-4-git-send-email-sidgup@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1609968211-7579-4-git-send-email-sidgup@codeaurora.org>
Precedence: bulk

On Wed 06 Jan 15:23 CST 2021, Siddharth Gupta wrote:

> Since the split elf blobs will always contain the hash segment, we rely on

I think it will sounds better if we add "should" in "we should rely on..."

> the blob file to get the hash rather than assume that it will be present in
> the mdt file. This change uses the hash index to read the appropriate elf
> blob to get the hash segment.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c  |  4 ++--
>  drivers/soc/qcom/mdt_loader.c       | 38 +++++++++++++++++++++++++++----------
>  include/linux/soc/qcom/mdt_loader.h |  3 ++-
>  3 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 66106ba..74c0229 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright (C) 2016 Linaro Ltd.
>   * Copyright (C) 2014 Sony Mobile Communications AB
> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>   */
>  
>  #include <linux/clk.h>
> @@ -828,7 +828,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  	void *ptr;
>  	int ret;
>  
> -	metadata = qcom_mdt_read_metadata(fw, &size);
> +	metadata = qcom_mdt_read_metadata(qproc->dev, fw, qproc->hexagon_mdt_image, &size);
>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index c9bbd8c..6876c0b 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -103,15 +103,18 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
>   *
>   * Return: pointer to data, or ERR_PTR()
>   */
> -void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
> +			     size_t *data_len)
>  {
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_hdr *ehdr;
> -	size_t hash_offset;
> +	const struct firmware *seg_fw;
>  	size_t hash_index;
>  	size_t hash_size;
>  	size_t ehdr_size;
> +	char *fw_name;
>  	void *data;
> +	int ret;
>  
>  	ehdr = (struct elf32_hdr *)fw->data;
>  	phdrs = (struct elf32_phdr *)(ehdr + 1);
> @@ -137,14 +140,29 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	if (!data)
>  		return ERR_PTR(-ENOMEM);
>  
> -	/* Is the header and hash already packed */
> -	if (qcom_mdt_bins_are_split(fw))
> -		hash_offset = phdrs[0].p_filesz;
> -	else
> -		hash_offset = phdrs[hash_index].p_offset;
> -
> +	/* copy elf header */
>  	memcpy(data, fw->data, ehdr_size);
> -	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> +

This seems to duplicates parts of the loop in __qcom_mdt_load(), how
about breaking this out to a separate

static int mdt_load_segment(struct device *dev, const struct firmware *fw,
			    int idx, void *buf, size_t len, bool is_split)

Which either just memcpy from @fw or does the filename and loading
dance, based on @is_split?

Regards,
Bjorn

> +	if (qcom_mdt_bins_are_split(fw)) {
> +		fw_name = kstrdup(firmware, GFP_KERNEL);
> +		if (!fw_name) {
> +			kfree(data);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		snprintf(fw_name + strlen(fw_name) - 3, 4, "b%02d", hash_index);
> +
> +		ret = request_firmware_into_buf(&seg_fw, fw_name, dev, data + ehdr_size, hash_size);
> +		kfree(fw_name);
> +
> +		if (ret) {
> +			kfree(data);
> +			return ERR_PTR(ret);
> +		}
> +
> +		release_firmware(seg_fw);
> +	} else {
> +		memcpy(data + ehdr_size, fw->data + phdrs[hash_index].p_offset, hash_size);
> +	}
>  
>  	*data_len = ehdr_size + hash_size;
>  
> @@ -191,7 +209,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  		return -ENOMEM;
>  
>  	if (pas_init) {
> -		metadata = qcom_mdt_read_metadata(fw, &metadata_len);
> +		metadata = qcom_mdt_read_metadata(dev, fw, firmware, &metadata_len);
>  		if (IS_ERR(metadata)) {
>  			ret = PTR_ERR(metadata);
>  			goto out;
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index e600bae..04ba5e8 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -21,6 +21,7 @@ int qcom_mdt_load_no_init(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,
>  			  phys_addr_t *reloc_base);
> -void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len);
> +void *qcom_mdt_read_metadata(struct device *dev, const struct firmware *fw, const char *firmware,
> +			     size_t *data_len);
>  
>  #endif
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>