[PATCH 0/3] soc: qcom: mdt_loader: General improvements

[PATCH 0/3] soc: qcom: mdt_loader: General improvements

[Siddharth Gupta]

This series of patches improves general functionality for the mdt loader.

Patch 1 adds the ability to dynamically detect hash segment location.
Patch 2 updates the logic used to identify whether the firmware is split or not.
Patch 3 updates the way the metadata is read and generated.

Siddharth Gupta (3):
  soc: qcom: mdt_loader: Allow hash at any phdr
  soc: qcom: mdt_loader: Handle split bins correctly
  soc: qcom: mdt_loader: Read hash from firmware blob

 drivers/remoteproc/qcom_q6v5_mss.c  |   4 +-
 drivers/soc/qcom/mdt_loader.c       | 110 ++++++++++++++++++++++++------------
 include/linux/soc/qcom/mdt_loader.h |   3 +-
 3 files changed, 79 insertions(+), 38 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr

[Siddharth Gupta]

The assumption that the elf program header will always have the hash
segment program header at index 1 may not hold true in all cases. This
change updates the read metadata function to find the hash program header
dynamically.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/soc/qcom/mdt_loader.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193..813216d 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -4,7 +4,7 @@
  *
  * Copyright (C) 2016 Linaro Ltd
  * Copyright (C) 2015 Sony Mobile Communications Inc
- * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
  */
 
 #include <linux/device.h>
@@ -88,6 +88,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	const struct elf32_phdr *phdrs;
 	const struct elf32_hdr *ehdr;
 	size_t hash_offset;
+	size_t hash_index;
 	size_t hash_size;
 	size_t ehdr_size;
 	void *data;
@@ -98,14 +99,19 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	if (ehdr->e_phnum < 2)
 		return ERR_PTR(-EINVAL);
 
-	if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
+	if (phdrs[0].p_type == PT_LOAD)
 		return ERR_PTR(-EINVAL);
 
-	if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
+	for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
+		if (phdrs[hash_index].p_type != PT_LOAD &&
+		   (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
+			break;
+	}
+	if (hash_index >= ehdr->e_phnum)
 		return ERR_PTR(-EINVAL);
 
 	ehdr_size = phdrs[0].p_filesz;
-	hash_size = phdrs[1].p_filesz;
+	hash_size = phdrs[hash_index].p_filesz;
 
 	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
 	if (!data)
@@ -115,7 +121,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 	if (ehdr_size + hash_size == fw->size)
 		hash_offset = phdrs[0].p_filesz;
 	else
-		hash_offset = phdrs[1].p_offset;
+		hash_offset = phdrs[hash_index].p_offset;
 
 	memcpy(data, fw->data, ehdr_size);
 	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly

[Siddharth Gupta]

It may be that the offset of the first program header lies inside the mdt's
filesize, in this case the loader would incorrectly assume that the bins
were not split. The loading would then continue on to fail for split bins.
This change updates the logic used by the mdt loader to understand whether
the firmware images are split or not. It figures this out by checking if
each program header's segment lies within the file or not.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/soc/qcom/mdt_loader.c | 60 +++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 813216d..c9bbd8c 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
 	return true;
 }
 
+static bool qcom_mdt_bins_are_split(const struct firmware *fw)
+{
+	const struct elf32_phdr *phdrs;
+	const struct elf32_hdr *ehdr;
+	uint64_t seg_start, seg_end;
+	int i;
+
+	ehdr = (struct elf32_hdr *)fw->data;
+	phdrs = (struct elf32_phdr *)(ehdr + 1);
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		seg_start = phdrs[i].p_offset;
+		seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
+		if (seg_start > fw->size || seg_end > fw->size)
+			return true;
+	}
+
+	return false;
+}
+
 /**
  * qcom_mdt_get_size() - acquire size of the memory region needed to load mdt
  * @fw:		firmware object for the mdt file
@@ -118,7 +138,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
 		return ERR_PTR(-ENOMEM);
 
 	/* Is the header and hash already packed */
-	if (ehdr_size + hash_size == fw->size)
+	if (qcom_mdt_bins_are_split(fw))
 		hash_offset = phdrs[0].p_filesz;
 	else
 		hash_offset = phdrs[hash_index].p_offset;
@@ -150,6 +170,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 	void *metadata;
 	char *fw_name;
 	bool relocate = false;
+	bool is_split;
 	void *ptr;
 	int ret = 0;
 	int i;
@@ -157,6 +178,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 	if (!fw || !mem_region || !mem_phys || !mem_size)
 		return -EINVAL;
 
+	is_split = qcom_mdt_bins_are_split(fw);
 	ehdr = (struct elf32_hdr *)fw->data;
 	phdrs = (struct elf32_phdr *)(ehdr + 1);
 
@@ -238,28 +260,22 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 
 		ptr = mem_region + offset;
 
-		if (phdr->p_filesz && phdr->p_offset < fw->size) {
-			/* Firmware is large enough to be non-split */
-			if (phdr->p_offset + phdr->p_filesz > fw->size) {
-				dev_err(dev,
-					"failed to load segment %d from truncated file %s\n",
-					i, firmware);
-				ret = -EINVAL;
-				break;
+		if (phdr->p_filesz) {
+			if (!is_split) {
+				/* Firmware is large enough to be non-split */
+				memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
+			} else {
+				/* Firmware not large enough, load split-out segments */
+				snprintf(fw_name + fw_name_len - 3, 4, "b%02d", i);
+				ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
+								ptr, phdr->p_filesz);
+				if (ret) {
+					dev_err(dev, "failed to load %s\n", fw_name);
+					break;
+				}
+
+				release_firmware(seg_fw);
 			}
-
-			memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
-		} else if (phdr->p_filesz) {
-			/* Firmware not large enough, load split-out segments */
-			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
-							ptr, phdr->p_filesz);
-			if (ret) {
-				dev_err(dev, "failed to load %s\n", fw_name);
-				break;
-			}
-
-			release_firmware(seg_fw);
 		}
 
 		if (phdr->p_memsz > phdr->p_filesz)
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

Since the split elf blobs will always contain the hash segment, we 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);
+
+	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

Re: [PATCH 1/3] soc: qcom: mdt_loader: Allow hash at any phdr

[Bjorn Andersson]

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

> The assumption that the elf program header will always have the hash
> segment program header at index 1 may not hold true in all cases. This
> change updates the read metadata function to find the hash program header
> dynamically.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/soc/qcom/mdt_loader.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 24cd193..813216d 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright (C) 2016 Linaro Ltd
>   * Copyright (C) 2015 Sony Mobile Communications Inc
> - * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2012-2013, 2020 The Linux Foundation. All rights reserved.
>   */
>  
>  #include <linux/device.h>
> @@ -88,6 +88,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_hdr *ehdr;
>  	size_t hash_offset;
> +	size_t hash_index;
>  	size_t hash_size;
>  	size_t ehdr_size;
>  	void *data;
> @@ -98,14 +99,19 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	if (ehdr->e_phnum < 2)
>  		return ERR_PTR(-EINVAL);
>  
> -	if (phdrs[0].p_type == PT_LOAD || phdrs[1].p_type == PT_LOAD)
> +	if (phdrs[0].p_type == PT_LOAD)
>  		return ERR_PTR(-EINVAL);
>  
> -	if ((phdrs[1].p_flags & QCOM_MDT_TYPE_MASK) != QCOM_MDT_TYPE_HASH)
> +	for (hash_index = 1; hash_index < ehdr->e_phnum; hash_index++) {
> +		if (phdrs[hash_index].p_type != PT_LOAD &&
> +		   (phdrs[hash_index].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH)
> +			break;
> +	}
> +	if (hash_index >= ehdr->e_phnum)
>  		return ERR_PTR(-EINVAL);
>  
>  	ehdr_size = phdrs[0].p_filesz;
> -	hash_size = phdrs[1].p_filesz;
> +	hash_size = phdrs[hash_index].p_filesz;
>  
>  	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
>  	if (!data)
> @@ -115,7 +121,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  	if (ehdr_size + hash_size == fw->size)
>  		hash_offset = phdrs[0].p_filesz;
>  	else
> -		hash_offset = phdrs[1].p_offset;
> +		hash_offset = phdrs[hash_index].p_offset;
>  
>  	memcpy(data, fw->data, ehdr_size);
>  	memcpy(data + ehdr_size, fw->data + hash_offset, hash_size);
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 2/3] soc: qcom: mdt_loader: Handle split bins correctly

[Bjorn Andersson]

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

> It may be that the offset of the first program header lies inside the mdt's
> filesize, in this case the loader would incorrectly assume that the bins
> were not split. The loading would then continue on to fail for split bins.
> This change updates the logic used by the mdt loader to understand whether
> the firmware images are split or not. It figures this out by checking if
> each program header's segment lies within the file or not.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/soc/qcom/mdt_loader.c | 60 +++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 813216d..c9bbd8c 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -31,6 +31,26 @@ static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
>  	return true;
>  }
>  
> +static bool qcom_mdt_bins_are_split(const struct firmware *fw)
> +{
> +	const struct elf32_phdr *phdrs;
> +	const struct elf32_hdr *ehdr;
> +	uint64_t seg_start, seg_end;
> +	int i;
> +
> +	ehdr = (struct elf32_hdr *)fw->data;
> +	phdrs = (struct elf32_phdr *)(ehdr + 1);
> +
> +	for (i = 0; i < ehdr->e_phnum; i++) {
> +		seg_start = phdrs[i].p_offset;
> +		seg_end = phdrs[i].p_offset + phdrs[i].p_filesz;
> +		if (seg_start > fw->size || seg_end > fw->size)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * qcom_mdt_get_size() - acquire size of the memory region needed to load mdt
>   * @fw:		firmware object for the mdt file
> @@ -118,7 +138,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len)
>  		return ERR_PTR(-ENOMEM);
>  
>  	/* Is the header and hash already packed */
> -	if (ehdr_size + hash_size == fw->size)
> +	if (qcom_mdt_bins_are_split(fw))
>  		hash_offset = phdrs[0].p_filesz;
>  	else
>  		hash_offset = phdrs[hash_index].p_offset;
> @@ -150,6 +170,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  	void *metadata;
>  	char *fw_name;
>  	bool relocate = false;
> +	bool is_split;
>  	void *ptr;
>  	int ret = 0;
>  	int i;
> @@ -157,6 +178,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  	if (!fw || !mem_region || !mem_phys || !mem_size)
>  		return -EINVAL;
>  
> +	is_split = qcom_mdt_bins_are_split(fw);
>  	ehdr = (struct elf32_hdr *)fw->data;
>  	phdrs = (struct elf32_phdr *)(ehdr + 1);
>  
> @@ -238,28 +260,22 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
>  
>  		ptr = mem_region + offset;
>  
> -		if (phdr->p_filesz && phdr->p_offset < fw->size) {
> -			/* Firmware is large enough to be non-split */
> -			if (phdr->p_offset + phdr->p_filesz > fw->size) {
> -				dev_err(dev,
> -					"failed to load segment %d from truncated file %s\n",
> -					i, firmware);
> -				ret = -EINVAL;
> -				break;
> +		if (phdr->p_filesz) {
> +			if (!is_split) {

In an effort to reduce the diff size and avoid adding another level of
indentation, how about making the conditionals:

		if (is_split && phdr->p_filesz) {
			memcpy();
		} else if (phdr->p_filesz) {
			...
		}

Apart from that I think this patch looks good!

Regards,
Bjorn

> +				/* Firmware is large enough to be non-split */
> +				memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> +			} else {
> +				/* Firmware not large enough, load split-out segments */
> +				snprintf(fw_name + fw_name_len - 3, 4, "b%02d", i);
> +				ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> +								ptr, phdr->p_filesz);
> +				if (ret) {
> +					dev_err(dev, "failed to load %s\n", fw_name);
> +					break;
> +				}
> +
> +				release_firmware(seg_fw);
>  			}
> -
> -			memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
> -		} else if (phdr->p_filesz) {
> -			/* Firmware not large enough, load split-out segments */
> -			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
> -			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
> -							ptr, phdr->p_filesz);
> -			if (ret) {
> -				dev_err(dev, "failed to load %s\n", fw_name);
> -				break;
> -			}
> -
> -			release_firmware(seg_fw);
>  		}
>  
>  		if (phdr->p_memsz > phdr->p_filesz)
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Bjorn Andersson]

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
> 

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
> 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..."
Sure
>
>> 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?
Since mdt_load_segment won't know the name of the firmware without a 
global variable
(which in turn will make it non-reentrant), the idea of creating such a 
function and not passing
the actual name of the firmware seemed wrong.

If we want to pass the firmware name in this function the code size will 
be more or equal to
what we started with. If that is not a problem I can make the changes.

Thanks,
Sid
>
> 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
>>

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Bjorn Andersson]

On Wed 13 Jan 17:01 CST 2021, Siddharth Gupta wrote:

> 
> On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
> > 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..."
> Sure
> > 
> > > 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?
> Since mdt_load_segment won't know the name of the firmware without a global
> variable
> (which in turn will make it non-reentrant), the idea of creating such a
> function and not passing
> the actual name of the firmware seemed wrong.
> 

Wouldn't you be able to pass "firmware" as an argument to the
load_segment function?

> If we want to pass the firmware name in this function the code size will be
> more or equal to
> what we started with. If that is not a problem I can make the changes.
> 

Perhaps I'm missing something here, I do expect that you would end with
code similar to the hunk you add here. But in doing so we should be able
to reuse that in the __qcom_mdt_load(). Or am I too optimistic?

(In particular I'm not fond of the fw_name dance and doing it twice is
worse)

Regards,
Bjorn

> Thanks,
> Sid
> > 
> > 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
> > > 

Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob

[Siddharth Gupta]

On 1/14/2021 9:46 AM, Bjorn Andersson wrote:
> On Wed 13 Jan 17:01 CST 2021, Siddharth Gupta wrote:
>
>> On 1/7/2021 4:21 PM, Bjorn Andersson wrote:
>>> 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..."
>> Sure
>>>> 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?
>> Since mdt_load_segment won't know the name of the firmware without a global
>> variable
>> (which in turn will make it non-reentrant), the idea of creating such a
>> function and not passing
>> the actual name of the firmware seemed wrong.
>>
> Wouldn't you be able to pass "firmware" as an argument to the
> load_segment function?
We can, which is what I guess you meant in the first place. What I thought
was that if we are creating something like mdt_load_segments passing
"firmware.mdt" with the index instead of "firmware.b<index>" seemed
kind of wrong, but I guess that is just a matter of the naming convention.
>> If we want to pass the firmware name in this function the code size will be
>> more or equal to
>> what we started with. If that is not a problem I can make the changes.
>>
> Perhaps I'm missing something here, I do expect that you would end with
> code similar to the hunk you add here. But in doing so we should be able
> to reuse that in the __qcom_mdt_load(). Or am I too optimistic?
>
> (In particular I'm not fond of the fw_name dance and doing it twice is
> worse)
If I am creating the firmware name inside this function then we should 
definitely
see a code reduction. I'll make the changes and push the new patchset soon.

Thanks,
Sid
>
> Regards,
> Bjorn
>
>> Thanks,
>> Sid
>>> 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
>>>>