From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: "Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.org>
Cc: sboyd@codeaurora.org, agross@codeaurora.org,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-remoteproc@vger.kernel.org
Subject: Re: [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch
Date: Fri, 26 May 2017 12:17:47 -0700 [thread overview]
Message-ID: <20170526191747.GJ12920@tuxbook> (raw)
In-Reply-To: <a061dff7-fc09-9d03-3d8b-d72a1e84e049@codeaurora.org>
On Fri 26 May 06:19 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote:
>
>
> On 5/26/2017 7:46 AM, Bjorn Andersson wrote:
> > On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote:
> > > @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
> > > dev_err(qproc->dev,
> > > "metadata authentication failed: %d\n", ret);
> > > + /* Metadata authentication done, remove modem access */
> > > + ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size);
> > > + if (ret)
> > > + dev_err(qproc->dev,
> > > + "Failed to reclaim metadata memory, ret - %d\n", ret);
> > If this assignment fails (for any reason) we can't return the memory to
> > the free pool in Linux, because at some point in the future these pages
> > will be allocated to someone else resulting in a memory access violation
> > scenario that will be just terrible to debug.
> >
> > I do however not have a better idea at the moment than just leaking the
> > memory.
> Then shall we BUG_ON here itself?
Here we have the chance to "handle" the problem (by leaking the memory),
so it's not a catastrophic error. But perhaps better to replace the
dev_err() with a WARN(ret, "failed to reclaim memory\n"); that way this
issue will stand out in the log.
[..]
> > > @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc)
[..]
> > > }
> > > + ret = q6v5_assign_mem_to_linux(qproc,
> > > + qproc->mba_phys, qproc->mba_size);
> > > + if (ret)
> > > + dev_err(qproc->dev,
> > > + "Failed to reclaim mba memory, ret - %d\n", ret);
> > I think it's okay for symmetrical purposes to keep the memory assigned
> > to the remote until we call stop().
> Actually MBA image is transferred into internal memory of q6 and does not
> run from DDR
> that is why it is OK to release it here. let me know if you dont want to do
> that.
Leave it here, but please add a comment above this snippet saying
something like:
/*
* The MBA is transferred to internal memory of the Q6 and no longer
* need access.
*/
[..]
> > > + assign_mem_result =
> > > + q6v5_assign_mem_to_linux(qproc,
> > > + qproc->mba_phys, qproc->mba_size);
> > Shouldn't we move them even further down?
> There does not seem any restriction w.r.t. keeping it here.
> Do you think any side effect ?
No, I'm fine with this if you say that the MPSS is no longer executing
anything at this point.
Thanks,
Bjorn
next prev parent reply other threads:[~2017-05-26 19:17 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-16 18:01 [RESEND: PATCH v4 0/4] Add memory ownership switch support and enable mss rproc on msm8996 Avaneesh Kumar Dwivedi
2017-05-16 18:01 ` [RESEND: PATCH v4 1/4] firmware: scm: Add new SCM call for switching memory ownership Avaneesh Kumar Dwivedi
2017-05-26 6:03 ` Bjorn Andersson
2017-05-26 13:01 ` Dwivedi, Avaneesh Kumar (avani)
2017-05-26 19:19 ` Bjorn Andersson
2017-05-16 18:02 ` [RESEND: PATCH v4 2/4] remoteproc: qcom: refactor mss fw image loading sequence Avaneesh Kumar Dwivedi
2017-05-20 2:55 ` Sricharan R
2017-05-22 9:33 ` Dwivedi, Avaneesh Kumar (avani)
2017-05-22 10:37 ` Sricharan R
2017-05-22 13:26 ` Dwivedi, Avaneesh Kumar (avani)
2017-05-25 19:03 ` Bjorn Andersson
2017-05-26 5:00 ` Sricharan R
2017-05-25 19:13 ` Bjorn Andersson
2017-05-26 13:02 ` Dwivedi, Avaneesh Kumar (avani)
2017-05-16 18:02 ` [RESEND: PATCH v4 3/4] remoteproc: qcom: Make secure world call for mem ownership switch Avaneesh Kumar Dwivedi
2017-05-26 2:16 ` Bjorn Andersson
2017-05-26 13:19 ` Dwivedi, Avaneesh Kumar (avani)
2017-05-26 19:17 ` Bjorn Andersson [this message]
2017-05-16 18:02 ` [RESEND: PATCH v4 4/4] remoteproc: qcom: Add support for mss boot on msm8996 Avaneesh Kumar Dwivedi
2017-05-26 6:09 ` Bjorn Andersson
2017-05-26 13:20 ` Dwivedi, Avaneesh Kumar (avani)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170526191747.GJ12920@tuxbook \
--to=bjorn.andersson@linaro.org \
--cc=agross@codeaurora.org \
--cc=akdwived@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=sboyd@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).