Messages in this thread | | | From | Siddharth Gupta <> | Subject | Re: [PATCH 3/3] soc: qcom: mdt_loader: Read hash from firmware blob | Date | Sun, 17 Jan 2021 15:03:22 -0800 |
| |
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 >>>>
| |