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 -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
