| Message ID | 20210828070202.7033-1-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] soc: qcom: mdt_loader: Drop PT_LOAD check on hash segment | expand |
Hi Shawn, On 8/28/21 9:02 AM, Shawn Guo wrote: > PT_LOAD type denotes that the segment should be loaded into the final > firmware memory region. Hash segment is not one such, because it's only > needed for PAS init and shouldn't be in the final firmware memory region. > That's why mdt_phdr_valid() explicitly reject non PT_LOAD segment and > hash segment. This actually makes the hash segment type check in > qcom_mdt_read_metadata() unnecessary and redundant. For a hash segment, > it won't be loaded into firmware memory region anyway, due to the > QCOM_MDT_TYPE_HASH check in mdt_phdr_valid(), even if it has a PT_LOAD > type for some reason (misusing or abusing?). > > Some firmware files on Sony phones are such examples, e.g WCNSS firmware > of Sony Xperia M4 Aqua phone. The type of hash segment is just PT_LOAD. > Drop the unnecessary hash segment type check in qcom_mdt_read_metadata() > to fix firmware loading failure on these phones, while hash segment is > still kept away from the final firmware memory region. > > Fixes: 498b98e93900 ("soc: qcom: mdt_loader: Support loading non-split images") > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> Thanks for sitting through that long discussion and coming to the conclusion that your patch was correct all along! > --- > Changes for v2: > - Update commit log based on the great disscussion with Marijn and Bjorn. > > drivers/soc/qcom/mdt_loader.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c > index eba7f76f9d61..6034cd8992b0 100644 > --- a/drivers/soc/qcom/mdt_loader.c > +++ b/drivers/soc/qcom/mdt_loader.c > @@ -98,7 +98,7 @@ 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) >
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index eba7f76f9d61..6034cd8992b0 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -98,7 +98,7 @@ 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)
PT_LOAD type denotes that the segment should be loaded into the final firmware memory region. Hash segment is not one such, because it's only needed for PAS init and shouldn't be in the final firmware memory region. That's why mdt_phdr_valid() explicitly reject non PT_LOAD segment and hash segment. This actually makes the hash segment type check in qcom_mdt_read_metadata() unnecessary and redundant. For a hash segment, it won't be loaded into firmware memory region anyway, due to the QCOM_MDT_TYPE_HASH check in mdt_phdr_valid(), even if it has a PT_LOAD type for some reason (misusing or abusing?). Some firmware files on Sony phones are such examples, e.g WCNSS firmware of Sony Xperia M4 Aqua phone. The type of hash segment is just PT_LOAD. Drop the unnecessary hash segment type check in qcom_mdt_read_metadata() to fix firmware loading failure on these phones, while hash segment is still kept away from the final firmware memory region. Fixes: 498b98e93900 ("soc: qcom: mdt_loader: Support loading non-split images") Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- Changes for v2: - Update commit log based on the great disscussion with Marijn and Bjorn. drivers/soc/qcom/mdt_loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)