Re: [PATCH 1/3] remoteproc: qcom_q6v5_mss: Add modem debug policy support

[Sibi Sankar]

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 <si...@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.