Received-SPF: pass (google.com: domain of linux-kernel+bounces-211512-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 12 Jun 2024 15:49:09 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Ekansh Gupta <quic_ekangupt@quicinc.com>
Cc: srinivas.kandagatla@linaro.org, linux-arm-msm@vger.kernel.org, 
	gregkh@linuxfoundation.org, quic_bkumar@quicinc.com, linux-kernel@vger.kernel.org, 
	quic_chennak@quicinc.com, stable <stable@kernel.org>
Subject: Re: [PATCH v5 6/7] misc: fastrpc: Fix ownership reassignment of
 remote heap
Message-ID: <xvy4syldmqdklij4wbub6x52gexijlrgtoketd3urohxrca5g2@mmioafzpmy7s>
References: <20240611103442.27198-1-quic_ekangupt@quicinc.com>
 <20240611103442.27198-7-quic_ekangupt@quicinc.com>
 <xc2ys75plbtrenastitqafadfrtolpd3bjdqcrl3wnozpc6kdo@e6e73ousyea7>
 <7d75f6b1-60fe-4a03-a251-4675a3364148@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7d75f6b1-60fe-4a03-a251-4675a3364148@quicinc.com>

On Wed, Jun 12, 2024 at 09:54:47AM +0530, Ekansh Gupta wrote:
> 
> 
> On 6/11/2024 5:29 PM, Dmitry Baryshkov wrote:
> > On Tue, Jun 11, 2024 at 04:04:39PM +0530, Ekansh Gupta wrote:
> >> Audio PD daemon will allocate memory for audio PD dynamic loading
> > What is Audio PD daemon? Is it something running on the CPU or on the
> > DSP? Is it adsprpcd or some other daemon?
> It's adsprpcd which is going to attach to Audio PD.

Ack

> >
> >> usage when it is attaching for the first time to audio PD. As
> >> part of this, the memory ownership is moved to the VM where
> > Which VM?
> In audio PD case, it's the following VMIDs:
> QCOM_SCM_VMID_LPASS
> QCOM_SCM_VMID_ADSP_HEAP
> 
> Defined here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/firmware/qcom,scm.h?h=v6.10-rc3
> 
> These are expected to be added to fastrpc DT node as "qcom,vmids"

Ok, good.

> 
> >
> >> audio PD can use it. In case daemon process is killed without any
> >> impact to DSP audio PD, the daemon process will retry to attach to
> >> audio PD and in this case memory won't be reallocated. If the invoke
> >> fails due to any reason, as part of err_invoke, the memory ownership
> >> is getting reassigned to HLOS even when the memory was not allocated.
> >> At this time the audio PD might still be using the memory and an
> >> attemp of ownership reassignment would result in memory issue.
> > What kind of 'memory issues'? Is it even possible to reclaim the memory
> > back?
> In case when audio PD on DSP is still using the memory, the ownership should not be
> moved to HLOS. This might happen in daemon kill scenario where remote_heap is not
> allocated again, but if due to any reason if the fastrpc_internal_invoke fails, that might
> result in the ownership change of remote_heap memory.

You are describing the expected behaviour. not the observed issue.

Also, the second quesiton didn't get the answer. Is it possible to free
/reclaim the memory?

> >
> >> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> >> Cc: stable <stable@kernel.org>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >> index 1ba85c70e3ff..24dc1cba40e9 100644
> >> --- a/drivers/misc/fastrpc.c
> >> +++ b/drivers/misc/fastrpc.c
> >> @@ -1238,6 +1238,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  	struct fastrpc_phy_page pages[1];
> >>  	char *name;
> >>  	int err;
> >> +	bool scm_done = false;
> >>  	struct {
> >>  		int pgid;
> >>  		u32 namelen;
> >> @@ -1289,6 +1290,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  					fl->cctx->remote_heap->phys, fl->cctx->remote_heap->size, err);
> >>  				goto err_map;
> >>  			}
> >> +			scm_done = true;
> >>  		}
> >>  	}
> >>  
> >> @@ -1324,7 +1326,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
> >>  
> >>  	return 0;
> >>  err_invoke:
> >> -	if (fl->cctx->vmcount) {
> >> +	if (fl->cctx->vmcount && scm_done) {
> >>  		u64 src_perms = 0;
> >>  		struct qcom_scm_vmperm dst_perms;
> >>  		u32 i;
> >> -- 
> >> 2.43.0
> >>
> 

-- 
With best wishes
Dmitry