Audio PD daemon will allocate memory for audio PD dynamic loading
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
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.
Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
Cc: stable <[email protected]>
Signed-off-by: Ekansh Gupta <[email protected]>
---
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
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?
> 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?
> 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?
>
> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
> Cc: stable <[email protected]>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> 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
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.
>
>> 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"
>
>> 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.
>
>> Fixes: 0871561055e6 ("misc: fastrpc: Add support for audiopd")
>> Cc: stable <[email protected]>
>> Signed-off-by: Ekansh Gupta <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
> >> Signed-off-by: Ekansh Gupta <[email protected]>
> >> ---
> >> 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