| Message ID | 1609968211-7579-4-git-send-email-sidgup@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: qcom: mdt_loader: General improvements | expand |
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 >
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 >>
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 > > >
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 >>>>
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
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(-)