| Message ID | 1526556740-25494-2-git-send-email-vgarodia@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 17, 2018 at 05:02:17PM +0530, Vikash Garodia wrote: > In order to invoke scm calls, ensure that the platform > has the required support to invoke the scm calls in > secure world. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/soc/qcom/mdt_loader.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c > index 17b314d..db55d53 100644 > --- a/drivers/soc/qcom/mdt_loader.c > +++ b/drivers/soc/qcom/mdt_loader.c > @@ -121,10 +121,12 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw, > if (!fw_name) > return -ENOMEM; > > - ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); > - if (ret) { > - dev_err(dev, "invalid firmware metadata\n"); > - goto out; > + if (qcom_scm_is_available()) { > + ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); > + if (ret) { > + dev_err(dev, "invalid firmware metadata\n"); > + goto out; > + } > } > > for (i = 0; i < ehdr->e_phnum; i++) { > @@ -144,10 +146,13 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw, > } > > if (relocate) { > - ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr); > - if (ret) { > - dev_err(dev, "unable to setup relocation\n"); > - goto out; > + if (qcom_scm_is_available()) { > + ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, > + max_addr - min_addr); > + if (ret) { > + dev_err(dev, "unable to setup relocation\n"); > + goto out; > + } > } > As far as I can tell you can make it all the way through the function with 'ret' uninitialized if qcom_scm_is_available() returns false which is a bug, but I'm confused why you would even bother loading the firmware even if you didn't have SCM. Jordan
On Thu 17 May 04:32 PDT 2018, Vikash Garodia wrote: > In order to invoke scm calls, ensure that the platform > has the required support to invoke the scm calls in > secure world. > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/soc/qcom/mdt_loader.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c > index 17b314d..db55d53 100644 > --- a/drivers/soc/qcom/mdt_loader.c > +++ b/drivers/soc/qcom/mdt_loader.c > @@ -121,10 +121,12 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw, > if (!fw_name) > return -ENOMEM; > > - ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); > - if (ret) { > - dev_err(dev, "invalid firmware metadata\n"); > - goto out; > + if (qcom_scm_is_available()) { qcom_scm_is_available() tells you if the qcom_scm driver has been probed, not if your platform implements PAS. Please add a DT property to tell the driver if it should require PAS or not (the absence of such property should indicate PAS is required, for backwards compatibility purposes). For the MDT loader we need to merge the following patch to make this work: https://patchwork.kernel.org/patch/10397889/ Regards, Bjorn
Hi Bjorn, On 2018-05-18 10:58, Bjorn Andersson wrote: > On Thu 17 May 04:32 PDT 2018, Vikash Garodia wrote: > >> In order to invoke scm calls, ensure that the platform >> has the required support to invoke the scm calls in >> secure world. >> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >> --- >> drivers/soc/qcom/mdt_loader.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/soc/qcom/mdt_loader.c >> b/drivers/soc/qcom/mdt_loader.c >> index 17b314d..db55d53 100644 >> --- a/drivers/soc/qcom/mdt_loader.c >> +++ b/drivers/soc/qcom/mdt_loader.c >> @@ -121,10 +121,12 @@ int qcom_mdt_load(struct device *dev, const >> struct firmware *fw, >> if (!fw_name) >> return -ENOMEM; >> >> - ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); >> - if (ret) { >> - dev_err(dev, "invalid firmware metadata\n"); >> - goto out; >> + if (qcom_scm_is_available()) { > > qcom_scm_is_available() tells you if the qcom_scm driver has been > probed, not if your platform implements PAS. > > Please add a DT property to tell the driver if it should require PAS or > not (the absence of such property should indicate PAS is required, for > backwards compatibility purposes). For the MDT loader we need to merge > the following patch to make this work: > > https://patchwork.kernel.org/patch/10397889/ Thanks for your inputs. I have already added a child node in video DT node to tell the driver if PAS is not needed. I will drop this patch as use https://patchwork.kernel.org/patch/10397889 and update the driver to call new api qcom_mdt_load_no_init. > Regards, > Bjorn
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 17b314d..db55d53 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -121,10 +121,12 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw, if (!fw_name) return -ENOMEM; - ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); - if (ret) { - dev_err(dev, "invalid firmware metadata\n"); - goto out; + if (qcom_scm_is_available()) { + ret = qcom_scm_pas_init_image(pas_id, fw->data, fw->size); + if (ret) { + dev_err(dev, "invalid firmware metadata\n"); + goto out; + } } for (i = 0; i < ehdr->e_phnum; i++) { @@ -144,10 +146,13 @@ int qcom_mdt_load(struct device *dev, const struct firmware *fw, } if (relocate) { - ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, max_addr - min_addr); - if (ret) { - dev_err(dev, "unable to setup relocation\n"); - goto out; + if (qcom_scm_is_available()) { + ret = qcom_scm_pas_mem_setup(pas_id, mem_phys, + max_addr - min_addr); + if (ret) { + dev_err(dev, "unable to setup relocation\n"); + goto out; + } } /*
In order to invoke scm calls, ensure that the platform has the required support to invoke the scm calls in secure world. Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> --- drivers/soc/qcom/mdt_loader.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)