| Message ID | 20200716123630.21892-2-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add modem debug features | expand |
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. > + 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. 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 >
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 >>
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) { + 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"); ret = q6v5_mpss_load(qproc); if (ret)
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(-)