Hi, >-----Original Message----- >From: Felipe Contreras [mailto:felipe.contreras@xxxxxxxxx] >Sent: Tuesday, February 16, 2010 12:43 PM >To: Ameya Palande >Cc: linux-omap@xxxxxxxxxxxxxxx; felipe.contreras@xxxxxxxxx; Menon, >Nishanth; Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Guzman Lugo, >Fernando >Subject: Re: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting >framework > >On Tue, Feb 16, 2010 at 3:20 PM, Ameya Palande <ameya.palande@xxxxxxxxx> >wrote: >> DSP_RSV_OBJECT is introduced to track reserved memory accounting >information. >> This will allow us to do proper cleanup for memory reserved using >> PROC_ReserveMemory(). >> >> Signed-off-by: Ameya Palande <ameya.palande@xxxxxxxxx> >> --- >> arch/arm/plat-omap/include/dspbridge/drv.h | 13 ++++++ >> arch/arm/plat-omap/include/dspbridge/proc.h | 5 +- >> drivers/dsp/bridge/pmgr/dmm.c | 3 +- >> drivers/dsp/bridge/pmgr/wcd.c | 7 ++- >> drivers/dsp/bridge/rmgr/drv.c | 27 +++++++++--- >> drivers/dsp/bridge/rmgr/drv_interface.c | 7 ++- >> drivers/dsp/bridge/rmgr/node.c | 5 +- >> drivers/dsp/bridge/rmgr/proc.c | 58 >+++++++++++++++++++++----- >> 8 files changed, 97 insertions(+), 28 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat- >omap/include/dspbridge/drv.h >> index 3069e74..6e8187d 100644 >> --- a/arch/arm/plat-omap/include/dspbridge/drv.h >> +++ b/arch/arm/plat-omap/include/dspbridge/drv.h >> @@ -101,6 +101,15 @@ struct DMM_MAP_OBJECT { >> struct DMM_MAP_OBJECT *next; >> } ; >> >> +/* >> + * New structure (member of process context) used for accounting of DMM >> + * reserved memory information >> + */ >> +struct DMM_RSV_OBJECT { >> + struct list_head link; >> + u32 dsp_reserved_addr; >> +}; >> + >> /* New structure (member of process context) abstracts DMM resource info >*/ >> struct DSPHEAP_RES_OBJECT { >> s32 heapAllocated; /* DMM status */ >> @@ -141,6 +150,10 @@ struct PROCESS_CONTEXT{ >> /* DMM mapped memory resources */ >> struct DMM_MAP_OBJECT *dmm_map_list; >> >> + /* DMM reserved memory resources */ >> + struct list_head dmm_rsv_list; >> + spinlock_t dmm_rsv_list_lock; >> + > >Why rsv requires a spinlock, but not map? > >I guess it would be needed if somehow PROC_UnReserveMemory() was >called by user-space at the same time than bridge_close, but is that >really possible? If it is, then the same danger is present with >PROC_UnMap() unless I'm missing something. No, that's not possible. However protection is still needed for PROC_ ReserveMemory/UnReserveMemory and also for PROC_Map/UnMap. Regards, Fernando. > >> /* DSP Heap resources */ >> struct DSPHEAP_RES_OBJECT *pDSPHEAPList; >> >> diff --git a/arch/arm/plat-omap/include/dspbridge/proc.h b/arch/arm/plat- >omap/include/dspbridge/proc.h >> index 8dbdaac..558c053 100644 >> --- a/arch/arm/plat-omap/include/dspbridge/proc.h >> +++ b/arch/arm/plat-omap/include/dspbridge/proc.h >> @@ -560,7 +560,7 @@ >> * Details: >> */ >> extern DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, >> - u32 ulSize, void >**ppRsvAddr); >> + u32 ulSize, void **ppRsvAddr, struct PROCESS_CONTEXT >*pr_ctxt); >> >> /* >> * ======== PROC_UnMap ======== >> @@ -604,6 +604,7 @@ >> * Details: >> */ >> extern DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, >> - void *pRsvAddr); >> + void *pRsvAddr, struct PROCESS_CONTEXT *pr_ctxt, >> + int cleanup); >> >> #endif /* PROC_ */ >> diff --git a/drivers/dsp/bridge/pmgr/dmm.c >b/drivers/dsp/bridge/pmgr/dmm.c >> index d5a7275..72aee60 100644 >> --- a/drivers/dsp/bridge/pmgr/dmm.c >> +++ b/drivers/dsp/bridge/pmgr/dmm.c >> @@ -386,9 +386,10 @@ DSP_STATUS DMM_ReserveMemory(struct DMM_OBJECT >*hDmmMgr, u32 size, >> node->MappedSize = 0; >> /* Return the chunk's starting address */ >> *pRsvAddr = rsvAddr; >> - } else >> + } else { >> /*dSP chunk of given size is not available */ >> status = DSP_EMEMORY; >> + } >> >> SYNC_LeaveCS(pDmmObj->hDmmLock); >> GT_3trace(DMM_debugMask, GT_4CLASS, >> diff --git a/drivers/dsp/bridge/pmgr/wcd.c >b/drivers/dsp/bridge/pmgr/wcd.c >> index beea23b..747d069 100644 >> --- a/drivers/dsp/bridge/pmgr/wcd.c >> +++ b/drivers/dsp/bridge/pmgr/wcd.c >> @@ -1054,12 +1054,13 @@ u32 PROCWRAP_ReserveMemory(union Trapped_Args >*args, void *pr_ctxt) >> >> GT_0trace(WCD_debugMask, GT_ENTER, "PROCWRAP_ReserveMemory: >entered\n"); >> status = PROC_ReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, >> - args->ARGS_PROC_RSVMEM.ulSize, >&pRsvAddr); >> + args->ARGS_PROC_RSVMEM.ulSize, >&pRsvAddr, >> + pr_ctxt); >> if (DSP_SUCCEEDED(status)) { >> if (put_user(pRsvAddr, args->ARGS_PROC_RSVMEM.ppRsvAddr)) >{ >> status = DSP_EINVALIDARG; >> PROC_UnReserveMemory(args- >>ARGS_PROC_RSVMEM.hProcessor, >> - pRsvAddr); >> + pRsvAddr, pr_ctxt, 1); >> } >> } >> return status; >> @@ -1100,7 +1101,7 @@ u32 PROCWRAP_UnReserveMemory(union Trapped_Args >*args, void *pr_ctxt) >> GT_0trace(WCD_debugMask, GT_ENTER, >> "PROCWRAP_UnReserveMemory: entered\n"); >> status = PROC_UnReserveMemory(args->ARGS_PROC_UNRSVMEM.hProcessor, >> - args->ARGS_PROC_UNRSVMEM.pRsvAddr); >> + args->ARGS_PROC_UNRSVMEM.pRsvAddr, pr_ctxt, 1); >> return status; >> } >> >> diff --git a/drivers/dsp/bridge/rmgr/drv.c >b/drivers/dsp/bridge/rmgr/drv.c >> index 3dc38d0..9d5c077 100644 >> --- a/drivers/dsp/bridge/rmgr/drv.c >> +++ b/drivers/dsp/bridge/rmgr/drv.c >> @@ -277,25 +277,20 @@ DSP_STATUS DRV_ProcFreeDMMRes(HANDLE hPCtxt) >> if (DSP_FAILED(status)) >> pr_debug("%s: PROC_UnMap failed! status =" >> " 0x%xn", __func__, >status); >> - status = PROC_UnReserveMemory(pDMMRes- >>hProcessor, >> - (void *)pDMMRes->ulDSPResAddr); >> - if (DSP_FAILED(status)) >> - pr_debug("%s: PROC_UnReserveMemory >failed!" >> - " status = 0x%xn", __func__, >status); >> pDMMRes->dmmAllocated = 0; >> } >> } >> return status; >> } >> >> -/* Release all DMM resources and its context >> -* This is called from .bridge_release. */ >> +/* Release all Mapped and Reserved DMM resources */ >> DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) >> { >> struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; >> DSP_STATUS status = DSP_SOK; >> struct DMM_MAP_OBJECT *pTempDMMRes2 = NULL; >> struct DMM_MAP_OBJECT *pTempDMMRes = NULL; >> + struct DMM_RSV_OBJECT *temp, *rsv_obj; >> >> DRV_ProcFreeDMMRes(pCtxt); >> pTempDMMRes = pCtxt->dmm_map_list; >> @@ -305,6 +300,24 @@ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE >hPCtxt) >> kfree(pTempDMMRes2); >> } >> pCtxt->dmm_map_list = NULL; >> + >> + /* Free DMM reserved memory resources */ >> + spin_lock(&pCtxt->dmm_rsv_list_lock); >> + list_for_each_entry_safe(rsv_obj, temp, &pCtxt->dmm_rsv_list, >link) { >> + /* >> + * Since we have already acquired spin lock for >dmm_rsv_list, >> + * we don't want to perform cleanup in >PROC_UnReserveMemory. >> + */ > >Sure, but why did we acquire a spin lock in the first place? > >> + status = PROC_UnReserveMemory(pCtxt->hProcessor, >> + (void *)rsv_obj->dsp_reserved_addr, >pCtxt, 0); > >If this behaved similarly to the unmap code, then 'cleanup' could be >one and there wouldn't be any need to manually remove the entries. > >> + if (DSP_FAILED(status)) >> + pr_err("%s: PROC_UnReserveMemory failed!" >> + " status = 0x%xn", __func__, >status); >> + list_del(&rsv_obj->link); >> + kfree(rsv_obj); >> + } >> + spin_unlock(&pCtxt->dmm_rsv_list_lock); >> + >> return status; >> } > >Cheers. > >-- >Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
