Messages in this thread | | | Date | Fri, 17 Jul 2020 10:37:31 +0530 | From | Sibi Sankar <> | Subject | Re: [PATCH 1/3] remoteproc: qcom_q6v5_mss: Add modem debug policy support |
| |
Hey Bjorn, Thanks for taking time to review the series.
On 2020-07-17 10:11, Bjorn Andersson wrote: > On Thu 16 Jul 05:36 PDT 2020, Sibi Sankar wrote: > >> Add modem debug policy support which will enable coredumps and live >> debug support when the msadp firmware is present on secure devices. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/qcom_q6v5_mss.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c >> b/drivers/remoteproc/qcom_q6v5_mss.c >> index 13c6d5a72a831..95e21ed607cb9 100644 >> --- a/drivers/remoteproc/qcom_q6v5_mss.c >> +++ b/drivers/remoteproc/qcom_q6v5_mss.c >> @@ -187,6 +187,7 @@ struct q6v5 { >> phys_addr_t mba_phys; >> void *mba_region; >> size_t mba_size; >> + size_t dp_size; >> >> phys_addr_t mpss_phys; >> phys_addr_t mpss_reloc; >> @@ -406,6 +407,13 @@ static int q6v5_xfer_mem_ownership(struct q6v5 >> *qproc, int *current_perm, >> static int q6v5_load(struct rproc *rproc, const struct firmware *fw) >> { >> struct q6v5 *qproc = rproc->priv; >> + const struct firmware *dp_fw; >> + >> + if (!request_firmware(&dp_fw, "msadp", qproc->dev) && fw->size <= >> SZ_1M) { > > Can we change this to a request_firmware_direct() to avoid the fact > that > as written here devices lacking this file will pause here for 60 > seconds > waiting for userspace to assist in loading it (which at least none of > my > systems do). > > I also think that while it's nice to check that fw->size <= SZ_1M, to > avoid overwriting the tail of it, you should check that SZ_1M + > dp_fw->size < mba_size. To ensure that the memcpy doesn't go out of > bounds.
Sure I'll get ^^ done in the next re-spin.
> >> + memcpy(qproc->mba_region + SZ_1M, dp_fw->data, dp_fw->size); >> + qproc->dp_size = dp_fw->size; >> + release_firmware(dp_fw); >> + } >> >> memcpy(qproc->mba_region, fw->data, fw->size); >> >> @@ -896,6 +904,10 @@ static int q6v5_mba_load(struct q6v5 *qproc) >> } >> >> writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); >> + if (qproc->dp_size) { >> + writel(qproc->mba_phys + SZ_1M, qproc->rmb_base + >> RMB_PMI_CODE_START_REG); >> + writel(qproc->dp_size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); >> + } >> >> ret = q6v5proc_reset(qproc); >> if (ret) >> @@ -1258,7 +1270,8 @@ static int q6v5_start(struct rproc *rproc) >> if (ret) >> return ret; >> >> - dev_info(qproc->dev, "MBA booted, loading mpss\n"); >> + dev_info(qproc->dev, "MBA booted, debug policy %s, loading mpss\n", >> + qproc->dp_size ? "enabled" : "disabled"); > > "MBA booted with%s debug policy, loading mpss\n", qproc->dp_size ? "" : > "out" > > Please.
Sure I'll use your template instead.
> > Regards, > Bjorn > >> >> ret = q6v5_mpss_load(qproc); >> if (ret) >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project >>
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
| |