[PATCH 1/3] DSPBRIDGE: Cleanup bridge_release and remove DSP_Close

[PATCH 1/3] DSPBRIDGE: Cleanup bridge_release and remove DSP_Close

[Ameya Palande]

Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 arch/arm/plat-omap/include/dspbridge/dspdrv.h |   17 ----------------
 drivers/dsp/bridge/rmgr/drv_interface.c       |   19 ++---------------
 drivers/dsp/bridge/rmgr/dspdrv.c              |   26 -------------------------
 3 files changed, 3 insertions(+), 59 deletions(-)

diff --git a/arch/arm/plat-omap/include/dspbridge/dspdrv.h b/arch/arm/plat-omap/include/dspbridge/dspdrv.h
index f500ffb..3d04967 100644
--- a/arch/arm/plat-omap/include/dspbridge/dspdrv.h
+++ b/arch/arm/plat-omap/include/dspbridge/dspdrv.h
@@ -50,23 +50,6 @@
 #define MAX_DEV     10		/* Max support of 10 devices */
 
 /*
- *  ======== DSP_Close ========
- *  Purpose:
- *      Called when the client application/driver unloads the DDSP DLL. Upon
- *      unloading, the DDSP DLL will call CloseFile().
- *  Parameters:
- *      dwDeviceContext:    Handle returned by XXX_Open used to identify
- *                          the open context of the device
- *  Returns:
- *      TRUE indicates the device is successfully closed. FALSE indicates
- *      otherwise.
- *  Requires:
- *      dwOpenContext!= NULL.
- *  Ensures:The Application instance owned objects are cleaned up.
- */
-extern bool DSP_Close(u32 dwDeviceContext);
-
-/*
  *  ======== DSP_Deinit ========
  *  Purpose:
  *      This function is called by Device Manager to de-initialize a device.
diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index 24f6336..8cbdeee 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -655,22 +655,9 @@ func_cont:
  * driver. */
 static int bridge_release(struct inode *ip, struct file *filp)
 {
-	int status;
-       u32 pid;
-
-	GT_0trace(driverTrace, GT_ENTER, "-> driver_release\n");
-
-       /* Return PID instead of process handle */
-       pid = current->pid;
-
-       status = DSP_Close(pid);
-
-
-	(status == true) ? (status = 0) : (status = -1);
-
-	GT_0trace(driverTrace, GT_ENTER, " <- driver_release\n");
-
-	return status;
+	GT_0trace(driverTrace, GT_ENTER, "-> bridge_release\n");
+	GT_0trace(driverTrace, GT_ENTER, "<- bridge_release\n");
+	return 0;
 }
 
 /* This function provides IO interface to the bridge driver. */
diff --git a/drivers/dsp/bridge/rmgr/dspdrv.c b/drivers/dsp/bridge/rmgr/dspdrv.c
index a7a74fc..b5caf39 100644
--- a/drivers/dsp/bridge/rmgr/dspdrv.c
+++ b/drivers/dsp/bridge/rmgr/dspdrv.c
@@ -248,29 +248,3 @@ bool DSP_Deinit(u32 deviceContext)
 
 	return retVal;
 }
-
-/*
- *  ======== DSP_Close ========
- *  	The Calling Process handle is passed to DEV_CleanupProcesState
- *      for cleaning up of any resources used by the application
- */
-bool DSP_Close(u32 dwOpenContext)
-{
-	bool retVal = false;
-
-	DBC_Require(dwOpenContext != 0);
-
-	GT_0trace(curTrace, GT_ENTER, "Entering DSP_Close\n");
-
-#ifdef RES_CLEANUP_DISABLE
-
-	if (DSP_SUCCEEDED(DEV_CleanupProcessState((HANDLE) dwOpenContext))) {
-		GT_0trace(curTrace, GT_1CLASS, "DSP_Close Succeeded \r\n");
-		retVal = true;
-	} else {
-		GT_0trace(curTrace, GT_7CLASS, "DSP_Close failed \r\n");
-	}
-#endif
-
-	return retVal;
-}
-- 
1.6.2.4

[PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release

[Ameya Palande]

Instead of doing lazy resource cleanup in bridge_open, this patch will move
it to bridge_release. This way the resources allocated will be cleaned up
when that instance of the open is closed and will not wait till the next
instance of open happens.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 drivers/dsp/bridge/rmgr/drv_interface.c |  132 ++++++++++++-------------------
 1 files changed, 52 insertions(+), 80 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index 8cbdeee..2fcd6f9 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -562,102 +562,74 @@ static void __exit bridge_exit(void)
 	platform_driver_unregister(&bridge_driver);
 }
 
-/* This function is called when an application opens handle to the
- * bridge driver. */
-
+/*
+ * This function is called when an application opens handle to the
+ * bridge driver.
+ */
 static int bridge_open(struct inode *ip, struct file *filp)
 {
 	int status = 0;
-#ifndef RES_CLEANUP_DISABLE
-       u32     hProcess;
-	DSP_STATUS dsp_status = DSP_SOK;
-	HANDLE	     hDrvObject = NULL;
-	struct PROCESS_CONTEXT    *pPctxt = NULL;
-	struct PROCESS_CONTEXT	*next_node = NULL;
-	struct PROCESS_CONTEXT    *pCtxtclosed = NULL;
-	struct PROCESS_CONTEXT    *pCtxttraverse = NULL;
-	struct task_struct *tsk = NULL;
-	GT_0trace(driverTrace, GT_ENTER, "-> driver_open\n");
-	dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
+	DSP_STATUS dsp_status;
+	HANDLE hDrvObject;
+	struct PROCESS_CONTEXT *pr_ctxt = NULL;
 
-	/* Checking weather task structure for all process existing
-	 * in the process context list If not removing those processes*/
-	if (DSP_FAILED(dsp_status))
-		goto func_cont;
+	GT_0trace(driverTrace, GT_ENTER, "-> bridge_open\n");
 
-	DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
-	while (pCtxtclosed != NULL) {
-		tsk = find_task_by_vpid(pCtxtclosed->pid);
-		next_node = pCtxtclosed->next;
-
-		if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
-
-			GT_1trace(driverTrace, GT_5CLASS,
-				 "***Task structure not existing for "
-				 "process***%d\n", pCtxtclosed->pid);
-			DRV_RemoveAllResources(pCtxtclosed);
-			if (pCtxtclosed->hProcessor != NULL) {
-				DRV_GetProcCtxtList(&pCtxttraverse,
-					    (struct DRV_OBJECT *)hDrvObject);
-				if (pCtxttraverse->next == NULL) {
-					PROC_Detach(pCtxtclosed->hProcessor);
-				} else {
-					if ((pCtxtclosed->pid ==
-					  pCtxttraverse->pid) &&
-					  (pCtxttraverse->next != NULL)) {
-						pCtxttraverse =
-							pCtxttraverse->next;
-					}
-					while ((pCtxttraverse != NULL) &&
-					     (pCtxtclosed->hProcessor
-					     != pCtxttraverse->hProcessor)) {
-						pCtxttraverse =
-							pCtxttraverse->next;
-						if ((pCtxttraverse != NULL) &&
-						  (pCtxtclosed->pid ==
-						  pCtxttraverse->pid)) {
-							pCtxttraverse =
-							   pCtxttraverse->next;
-						}
-					}
-					if (pCtxttraverse == NULL) {
-						PROC_Detach
-						     (pCtxtclosed->hProcessor);
-					}
-				}
-			}
-			DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
-					     pCtxtclosed,
-					     (void *)pCtxtclosed->pid);
+	dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
+	if (DSP_SUCCEEDED(dsp_status)) {
+		/*
+		 * Allocate a new process context and insert it into global
+		 * process context list.
+		 */
+		DRV_InsertProcContext(hDrvObject, &pr_ctxt);
+		if (pr_ctxt) {
+			DRV_ProcUpdatestate(pr_ctxt, PROC_RES_ALLOCATED);
+			DRV_ProcSetPID(pr_ctxt, current->pid);
+		} else {
+			status = -ENOMEM;
 		}
-		pCtxtclosed = next_node;
+	} else {
+		status = -EIO;
 	}
-func_cont:
-	dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
-	if (DSP_SUCCEEDED(dsp_status))
-		dsp_status = DRV_InsertProcContext(
-				(struct DRV_OBJECT *)hDrvObject, &pPctxt);
 
-	if (pPctxt != NULL) {
-			/* Return PID instead of process handle */
-			hProcess = current->pid;
+	filp->private_data = pr_ctxt;
 
-		DRV_ProcUpdatestate(pPctxt, PROC_RES_ALLOCATED);
-			DRV_ProcSetPID(pPctxt, hProcess);
-	}
-#endif
-
-	GT_0trace(driverTrace, GT_ENTER, " <- driver_open\n");
+	GT_0trace(driverTrace, GT_ENTER, "<- bridge_open\n");
 	return status;
 }
 
-/* This function is called when an application closes handle to the bridge
- * driver. */
+/*
+ * This function is called when an application closes handle to the bridge
+ * driver.
+ */
 static int bridge_release(struct inode *ip, struct file *filp)
 {
+	int status = 0;
+	DSP_STATUS dsp_status;
+	HANDLE hDrvObject;
+	struct PROCESS_CONTEXT *pr_ctxt;
+
 	GT_0trace(driverTrace, GT_ENTER, "-> bridge_release\n");
+
+	if (!filp->private_data) {
+		status = -EIO;
+	} else {
+		pr_ctxt = filp->private_data;
+		dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
+		if (DSP_SUCCEEDED(dsp_status)) {
+			flush_signals(current);
+			DRV_RemoveAllResources(pr_ctxt);
+			PROC_Detach(pr_ctxt->hProcessor);
+			DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
+					pr_ctxt, (void *)pr_ctxt->pid);
+		} else {
+			status = -EIO;
+		}
+		filp->private_data = NULL;
+	}
+
 	GT_0trace(driverTrace, GT_ENTER, "<- bridge_release\n");
-	return 0;
+	return status;
 }
 
 /* This function provides IO interface to the bridge driver. */
-- 
1.6.2.4

[PATCH 3/3] DSPBRIDGE: Use TGID instead of PID for resource accounting

[Ameya Palande]

This patch uses TGID instead of PID for accounting resources tied to a
user space process. This should fix all the issues related to user space
multithreading and resource cleanup.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 drivers/dsp/bridge/pmgr/cmm.c           |    4 ++--
 drivers/dsp/bridge/rmgr/drv_interface.c |    2 +-
 drivers/dsp/bridge/rmgr/node.c          |   12 ++++++------
 drivers/dsp/bridge/rmgr/proc.c          |   22 +++++++++++-----------
 drivers/dsp/bridge/rmgr/strm.c          |   16 ++++++++--------
 drivers/dsp/bridge/services/kfile.c     |    7 ++-----
 drivers/dsp/bridge/wmd/chnl_sm.c        |    4 ++--
 7 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/cmm.c b/drivers/dsp/bridge/pmgr/cmm.c
index 99a2432..7dea18c 100644
--- a/drivers/dsp/bridge/pmgr/cmm.c
+++ b/drivers/dsp/bridge/pmgr/cmm.c
@@ -284,8 +284,8 @@ void *CMM_CallocBuf(struct CMM_OBJECT *hCmmMgr, u32 uSize,
 			 * We'll need to free up a process's alloc'd SM if the
 			 * client process goes away.
 			 */
-                       /* Return PID instead of process handle */
-                       pNode->hClientProc = current->pid;
+			/* Return TGID instead of process handle */
+			pNode->hClientProc = current->tgid;
 
 			/* put our node on InUse list */
 			LST_PutTail(pAllocator->pInUseListHead,
diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c
index 2fcd6f9..80c0eeb 100644
--- a/drivers/dsp/bridge/rmgr/drv_interface.c
+++ b/drivers/dsp/bridge/rmgr/drv_interface.c
@@ -584,7 +584,7 @@ static int bridge_open(struct inode *ip, struct file *filp)
 		DRV_InsertProcContext(hDrvObject, &pr_ctxt);
 		if (pr_ctxt) {
 			DRV_ProcUpdatestate(pr_ctxt, PROC_RES_ALLOCATED);
-			DRV_ProcSetPID(pr_ctxt, current->pid);
+			DRV_ProcSetPID(pr_ctxt, current->tgid);
 		} else {
 			status = -ENOMEM;
 		}
diff --git a/drivers/dsp/bridge/rmgr/node.c b/drivers/dsp/bridge/rmgr/node.c
index 53a42bf..5ff87e9 100644
--- a/drivers/dsp/bridge/rmgr/node.c
+++ b/drivers/dsp/bridge/rmgr/node.c
@@ -789,8 +789,8 @@ func_cont2:
 
 #ifndef RES_CLEANUP_DISABLE
 	if (DSP_SUCCEEDED(status)) {
-               /* Return PID instead of process handle */
-               hProcess = current->pid;
+		/* Return TGID instead of process handle */
+		hProcess = current->tgid;
 
 		res_status = CFG_GetObject((u32 *)&hDrvObject,
 					  REG_DRV_OBJECT);
@@ -813,8 +813,8 @@ func_cont2:
 		}
 	}
 	if (DSP_SUCCEEDED(status)) {
-               /* Return PID instead of process handle */
-               hProcess = current->pid;
+		/* Return TGID instead of process handle */
+		hProcess = current->tgid;
 		res_status = CFG_GetObject((u32 *)&hDrvObject,
 					REG_DRV_OBJECT);
 		if (DSP_SUCCEEDED(res_status)) {
@@ -1793,8 +1793,8 @@ func_cont1:
 	 /*  Free host-side resources allocated by NODE_Create()
 	 *  DeleteNode() fails if SM buffers not freed by client!  */
 #ifndef RES_CLEANUP_DISABLE
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 	res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
 	if (DSP_FAILED(res_status))
 		goto func_cont;
diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
index 0af05eb..469cf21 100644
--- a/drivers/dsp/bridge/rmgr/proc.c
+++ b/drivers/dsp/bridge/rmgr/proc.c
@@ -215,7 +215,7 @@ DSP_STATUS PROC_CleanupAllResources(void)
 		goto func_end;
 	DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
 	while (pCtxtclosed != NULL) {
-		if (current->pid != pCtxtclosed->pid) {
+		if (current->tgid != pCtxtclosed->pid) {
 			GT_1trace(PROC_DebugMask, GT_5CLASS,
 				 "***Cleanup of "
 				 "process***%d\n", pCtxtclosed->pid);
@@ -308,8 +308,8 @@ PROC_Attach(u32 uProcessor, OPTIONAL CONST struct DSP_PROCESSORATTRIN *pAttrIn,
 	pProcObject->hMgrObject = hMgrObject;
 	pProcObject->uProcessor = devType;
 	/* Get Caller Process and store it */
-       /* Return PID instead of process handle */
-       pProcObject->hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	pProcObject->hProcess = current->tgid;
 
 	if (pAttrIn)
 		pProcObject->uTimeout = pAttrIn->uTimeout;
@@ -385,8 +385,8 @@ func_end:
 	if (DSP_FAILED(status))
 		goto func_cont;
 
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDRVObject, REG_DRV_OBJECT);
 	if (DSP_FAILED(res_status))
@@ -402,8 +402,8 @@ func_end:
 		}
 	}
 func_cont:
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDRVObject, REG_DRV_OBJECT);
 	if (DSP_SUCCEEDED(res_status)) {
@@ -1468,8 +1468,8 @@ DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize,
 #ifndef RES_CLEANUP_DISABLE
 	if (DSP_SUCCEEDED(status)) {
 		/* Update the node and stream resource status */
-               /* Return PID instead of process handle */
-               hProcess = current->pid;
+		/* Return TGID instead of process handle */
+		hProcess = current->tgid;
 
 		res_status = CFG_GetObject((u32 *)&hDrvObject,
 					  REG_DRV_OBJECT);
@@ -1847,8 +1847,8 @@ DSP_STATUS PROC_UnMap(DSP_HPROCESSOR hProcessor, void *pMapAddr)
 		goto func_end;
 
 	/* Update the node and stream resource status */
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
 	if (DSP_FAILED(res_status))
diff --git a/drivers/dsp/bridge/rmgr/strm.c b/drivers/dsp/bridge/rmgr/strm.c
index bd55fd3..b6dbe46 100644
--- a/drivers/dsp/bridge/rmgr/strm.c
+++ b/drivers/dsp/bridge/rmgr/strm.c
@@ -203,8 +203,8 @@ DSP_STATUS STRM_AllocateBuffer(struct STRM_OBJECT *hStrm, u32 uSize,
 	if (DSP_FAILED(status))
 		goto func_end;
 
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
 	if (DSP_FAILED(res_status))
@@ -277,8 +277,8 @@ DSP_STATUS STRM_Close(struct STRM_OBJECT *hStrm)
 		goto func_end;
 
 	/* Update the node and stream resource status */
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
 	if (DSP_FAILED(res_status))
@@ -430,8 +430,8 @@ DSP_STATUS STRM_FreeBuffer(struct STRM_OBJECT *hStrm, u8 **apBuffer,
 	}
 #ifndef RES_CLEANUP_DISABLE
 	/* Update the node and stream resource status */
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
 	if (DSP_SUCCEEDED(res_status)) {
@@ -779,8 +779,8 @@ func_cont:
 		(void)DeleteStrm(pStrm);
 
 #ifndef RES_CLEANUP_DISABLE
-       /* Return PID instead of process handle */
-       hProcess = current->pid;
+	/* Return TGID instead of process handle */
+	hProcess = current->tgid;
 
 	res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
 	if (DSP_SUCCEEDED(res_status)) {
diff --git a/drivers/dsp/bridge/services/kfile.c b/drivers/dsp/bridge/services/kfile.c
index d29bc22..dd2a962 100644
--- a/drivers/dsp/bridge/services/kfile.c
+++ b/drivers/dsp/bridge/services/kfile.c
@@ -92,7 +92,6 @@ s32 KFILE_Close(struct KFILE_FileObj *hFile)
 {
 	s32 cRetVal = 0;	/* 0 indicates success */
 	s32 fRetVal = 0;
-	__kernel_pid_t curr_pid;
 
 	GT_1trace(KFILE_debugMask, GT_ENTER, "KFILE_Close: hFile 0x%x\n",
 		  hFile);
@@ -101,8 +100,6 @@ s32 KFILE_Close(struct KFILE_FileObj *hFile)
 	if (MEM_IsValidHandle(hFile, SIGNATURE)) {
 		/* Close file only if opened by the same process (id). Otherwise
 		 * Linux closes all open file handles when process exits.*/
-               /* Return PID instead of process handle */
-               curr_pid = (__kernel_pid_t)current->pid;
 		fRetVal = filp_close(hFile->fileDesc, NULL) ;
 		if (fRetVal) {
 			cRetVal = E_KFILE_ERROR;
@@ -181,8 +178,8 @@ struct KFILE_FileObj *KFILE_Open(CONST char *pszFileName, CONST char *pszMode)
 			hFile->size = fileDesc->f_op->llseek(fileDesc, 0,
 							    SEEK_END);
 			fileDesc->f_op->llseek(fileDesc, 0, SEEK_SET);
-                       /* Return PID instead of process handle */
-                       hFile->owner_pid = current->pid;
+			/* Return TGID instead of process handle */
+			hFile->owner_pid = current->tgid;
 
 			status = DSP_SOK;
 		}
diff --git a/drivers/dsp/bridge/wmd/chnl_sm.c b/drivers/dsp/bridge/wmd/chnl_sm.c
index e8ffb2f..c70dba8 100644
--- a/drivers/dsp/bridge/wmd/chnl_sm.c
+++ b/drivers/dsp/bridge/wmd/chnl_sm.c
@@ -933,8 +933,8 @@ DSP_STATUS WMD_CHNL_Open(OUT struct CHNL_OBJECT **phChnl,
 			pChnl->uMode = uMode;
 			pChnl->hUserEvent = hSyncEvent;	/* for Linux */
 			pChnl->hSyncEvent = hSyncEvent;
-                       /* get the process handle */
-                       pChnl->hProcess = current->pid;
+			/* Return TGID instead of process handle */
+			pChnl->hProcess = current->tgid;
 			pChnl->pCBArg = 0;
 			pChnl->cBytesMoved = 0;
 			/* Default to proc-copy */
-- 
1.6.2.4

RE: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release

[Ramos Falcon, Ernesto]

Hi,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Ameya Palande
> Sent: Wednesday, August 05, 2009 8:25 AM
> To: linux-omap@vger.kernel.org
> Cc: hiroshi.doyu@nokia.com; Guzman Lugo, Fernando; Ramirez Luna, Omar;
> roman.tereshonkov@nokia.com; Moogi, Suyog
> Subject: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release
> 
> Instead of doing lazy resource cleanup in bridge_open, this patch will
> move
> it to bridge_release. This way the resources allocated will be cleaned up
> when that instance of the open is closed and will not wait till the next
> instance of open happens.
> 
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
> ---
>  drivers/dsp/bridge/rmgr/drv_interface.c |  132 ++++++++++++--------------
> -----
>  1 files changed, 52 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c
> b/drivers/dsp/bridge/rmgr/drv_interface.c
> index 8cbdeee..2fcd6f9 100644
> --- a/drivers/dsp/bridge/rmgr/drv_interface.c
> +++ b/drivers/dsp/bridge/rmgr/drv_interface.c
> @@ -562,102 +562,74 @@ static void __exit bridge_exit(void)
>  	platform_driver_unregister(&bridge_driver);
>  }
> 
> -/* This function is called when an application opens handle to the
> - * bridge driver. */
> -
> +/*
> + * This function is called when an application opens handle to the
> + * bridge driver.
> + */
>  static int bridge_open(struct inode *ip, struct file *filp)
>  {
>  	int status = 0;
> -#ifndef RES_CLEANUP_DISABLE
> -       u32     hProcess;
> -	DSP_STATUS dsp_status = DSP_SOK;
> -	HANDLE	     hDrvObject = NULL;
> -	struct PROCESS_CONTEXT    *pPctxt = NULL;
> -	struct PROCESS_CONTEXT	*next_node = NULL;
> -	struct PROCESS_CONTEXT    *pCtxtclosed = NULL;
> -	struct PROCESS_CONTEXT    *pCtxttraverse = NULL;
> -	struct task_struct *tsk = NULL;
> -	GT_0trace(driverTrace, GT_ENTER, "-> driver_open\n");
> -	dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
> +	DSP_STATUS dsp_status;
> +	HANDLE hDrvObject;
> +	struct PROCESS_CONTEXT *pr_ctxt = NULL;
> 
> -	/* Checking weather task structure for all process existing
> -	 * in the process context list If not removing those processes*/
> -	if (DSP_FAILED(dsp_status))
> -		goto func_cont;
> +	GT_0trace(driverTrace, GT_ENTER, "-> bridge_open\n");
> 
> -	DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
> -	while (pCtxtclosed != NULL) {
> -		tsk = find_task_by_vpid(pCtxtclosed->pid);
> -		next_node = pCtxtclosed->next;
> -
> -		if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
> -
> -			GT_1trace(driverTrace, GT_5CLASS,
> -				 "***Task structure not existing for "
> -				 "process***%d\n", pCtxtclosed->pid);
> -			DRV_RemoveAllResources(pCtxtclosed);
> -			if (pCtxtclosed->hProcessor != NULL) {
> -				DRV_GetProcCtxtList(&pCtxttraverse,
> -					    (struct DRV_OBJECT *)hDrvObject);
> -				if (pCtxttraverse->next == NULL) {
> -					PROC_Detach(pCtxtclosed->hProcessor);
> -				} else {
> -					if ((pCtxtclosed->pid ==
> -					  pCtxttraverse->pid) &&
> -					  (pCtxttraverse->next != NULL)) {
> -						pCtxttraverse =
> -							pCtxttraverse->next;
> -					}
> -					while ((pCtxttraverse != NULL) &&
> -					     (pCtxtclosed->hProcessor
> -					     != pCtxttraverse->hProcessor)) {
> -						pCtxttraverse =
> -							pCtxttraverse->next;
> -						if ((pCtxttraverse != NULL) &&
> -						  (pCtxtclosed->pid ==
> -						  pCtxttraverse->pid)) {
> -							pCtxttraverse =
> -							   pCtxttraverse->next;
> -						}
> -					}
> -					if (pCtxttraverse == NULL) {
> -						PROC_Detach
> -						     (pCtxtclosed->hProcessor);
> -					}
> -				}
> -			}
> -			DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
> -					     pCtxtclosed,
> -					     (void *)pCtxtclosed->pid);
> +	dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
> +	if (DSP_SUCCEEDED(dsp_status)) {
> +		/*
> +		 * Allocate a new process context and insert it into global
> +		 * process context list.
> +		 */
> +		DRV_InsertProcContext(hDrvObject, &pr_ctxt);
> +		if (pr_ctxt) {
> +			DRV_ProcUpdatestate(pr_ctxt, PROC_RES_ALLOCATED);
> +			DRV_ProcSetPID(pr_ctxt, current->pid);
> +		} else {
> +			status = -ENOMEM;
>  		}
> -		pCtxtclosed = next_node;
> +	} else {
> +		status = -EIO;
>  	}
> -func_cont:
> -	dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT);
> -	if (DSP_SUCCEEDED(dsp_status))
> -		dsp_status = DRV_InsertProcContext(
> -				(struct DRV_OBJECT *)hDrvObject, &pPctxt);
> 
> -	if (pPctxt != NULL) {
> -			/* Return PID instead of process handle */
> -			hProcess = current->pid;
> +	filp->private_data = pr_ctxt;
> 
> -		DRV_ProcUpdatestate(pPctxt, PROC_RES_ALLOCATED);
> -			DRV_ProcSetPID(pPctxt, hProcess);
> -	}
> -#endif
> -
> -	GT_0trace(driverTrace, GT_ENTER, " <- driver_open\n");
> +	GT_0trace(driverTrace, GT_ENTER, "<- bridge_open\n");
>  	return status;
>  }
> 
> -/* This function is called when an application closes handle to the
> bridge
> - * driver. */
> +/*
> + * This function is called when an application closes handle to the
> bridge
> + * driver.
> + */
>  static int bridge_release(struct inode *ip, struct file *filp)
>  {
> +	int status = 0;
> +	DSP_STATUS dsp_status;
> +	HANDLE hDrvObject;
> +	struct PROCESS_CONTEXT *pr_ctxt;
> +
>  	GT_0trace(driverTrace, GT_ENTER, "-> bridge_release\n");
> +
> +	if (!filp->private_data) {
> +		status = -EIO;
> +	} else {
> +		pr_ctxt = filp->private_data;
> +		dsp_status = CFG_GetObject((u32 *)&hDrvObject,
> REG_DRV_OBJECT);
> +		if (DSP_SUCCEEDED(dsp_status)) {
> +			flush_signals(current);
> +			DRV_RemoveAllResources(pr_ctxt);
> +			PROC_Detach(pr_ctxt->hProcessor);
> +			DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
> +					pr_ctxt, (void *)pr_ctxt->pid);
> +		} else {
> +			status = -EIO;
> +		}
> +		filp->private_data = NULL;
> +	}
> +
>  	GT_0trace(driverTrace, GT_ENTER, "<- bridge_release\n");
> -	return 0;
> +	return status;
>  }
> 
>  /* This function provides IO interface to the bridge driver. */
> --
> 1.6.2.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

We have detected a use case where if an application creates a child process using fork call, and then the child and father processes call DSPProcessor_Attach() and create a new process context with new tgid; when the processes are terminated, only the last process calls bridge_release cleaning only the resources in the father process, leaving the child resources unreleased.

One solution we have seen is to perform goes through the entire process context list, clean up all the resources for all terminated processes or in "zombie" state, as below,

DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
while (pCtxtclosed != NULL) {
	printk("pCtxtclosed->pid = %d\n",pCtxtclosed->pid);
	tsk = find_task_by_pid(pCtxtclosed->pid);

	if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
	
		GT_1trace(driverTrace, GT_5CLASS,
			"***Task structure not existing for "
			 "process***%d\n", pCtxtclosed->pid);
		DRV_RemoveAllResources(pCtxtclosed);
		if (pCtxtclosed->hProcessor != NULL) {
					PROC_Detach
						 (pCtxtclosed->hProcessor);
		}
		pTmp = pCtxtclosed->next;
		DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
					 pCtxtclosed,
					 (void *)pCtxtclosed->pid);
	} else {
		pTmp = pCtxtclosed->next;
	}
	pCtxtclosed = pTmp;
}

Please let me know your comments.

/Ernesto

Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release

[Ameya Palande]

Hi Ernesto,

ext Ramos Falcon, Ernesto wrote:
> Hi,
> 
> We have detected a use case where if an application creates a child process using fork call, and then the child and father processes call DSPProcessor_Attach() and create a new process context with new tgid; when the processes are terminated, only the last process calls bridge_release cleaning only the resources in the father process, leaving the child resources unreleased.
> 
> One solution we have seen is to perform goes through the entire process context list, clean up all the resources for all terminated processes or in "zombie" state, as below,
> 
> DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
> while (pCtxtclosed != NULL) {
> 	printk("pCtxtclosed->pid = %d\n",pCtxtclosed->pid);
> 	tsk = find_task_by_pid(pCtxtclosed->pid);
> 
> 	if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
> 	
> 		GT_1trace(driverTrace, GT_5CLASS,
> 			"***Task structure not existing for "
> 			 "process***%d\n", pCtxtclosed->pid);
> 		DRV_RemoveAllResources(pCtxtclosed);
> 		if (pCtxtclosed->hProcessor != NULL) {
> 					PROC_Detach
> 						 (pCtxtclosed->hProcessor);
> 		}
> 		pTmp = pCtxtclosed->next;
> 		DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
> 					 pCtxtclosed,
> 					 (void *)pCtxtclosed->pid);
> 	} else {
> 		pTmp = pCtxtclosed->next;
> 	}
> 	pCtxtclosed = pTmp;
> }
> 
> Please let me know your comments.
> 
> /Ernesto

Good point :)

I would like to simplify this use case ;)

If we call DSPProcessor_Attach() twice in the same process and kill the process,
then it will leak memory for 1st instance of PROCESSOR object.

When we call open() on /dev/DspBridge a new PROCESS_CONTEXT is allocated, and it
should be allocated **only once** in bridge_open() unlike in NODE_Allocate() and
PROC_Attach(). PROCESS_CONTEXT tracks all the resources allocated on behalf of
an open file handle(and not the process / thread). When this handle is closed
all these resources should be freed in bridge_release(). Accountability of
resources should be done using PROCESS_CONTEXT and **not pid (which will be
different for different thread) / tgid (which will be different for parent and
child).

Above problem occurs because PROCESS_CONTEXT by design tracks only one PROCESSOR
object which gets freed in bridge_release().

Let me know your comments on this, and then we can proceed to fix this issue.

Cheers,
Ameya.

RE: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release

[Ramos Falcon, Ernesto]

Hi Ameya,

> -----Original Message-----
> From: Ameya Palande [mailto:ameya.palande@nokia.com]
> Sent: Wednesday, August 05, 2009 4:52 PM
> To: Ramos Falcon, Ernesto
> Cc: linux-omap@vger.kernel.org; Doyu Hiroshi (Nokia-D/Helsinki); Guzman
> Lugo, Fernando; Ramirez Luna, Omar; Tereshonkov Roman (Nokia-D/Helsinki);
> Moogi, Suyog
> Subject: Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to
> bridge_release
> 
> Hi Ernesto,
> 
> ext Ramos Falcon, Ernesto wrote:
> > Hi,
> >
> > We have detected a use case where if an application creates a child
> process using fork call, and then the child and father processes call
> DSPProcessor_Attach() and create a new process context with new tgid; when
> the processes are terminated, only the last process calls bridge_release
> cleaning only the resources in the father process, leaving the child
> resources unreleased.
> >
> > One solution we have seen is to perform goes through the entire process
> context list, clean up all the resources for all terminated processes or
> in "zombie" state, as below,
> >
> > DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
> > while (pCtxtclosed != NULL) {
> > 	printk("pCtxtclosed->pid = %d\n",pCtxtclosed->pid);
> > 	tsk = find_task_by_pid(pCtxtclosed->pid);
> >
> > 	if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
> >
> > 		GT_1trace(driverTrace, GT_5CLASS,
> > 			"***Task structure not existing for "
> > 			 "process***%d\n", pCtxtclosed->pid);
> > 		DRV_RemoveAllResources(pCtxtclosed);
> > 		if (pCtxtclosed->hProcessor != NULL) {
> > 					PROC_Detach
> > 						 (pCtxtclosed->hProcessor);
> > 		}
> > 		pTmp = pCtxtclosed->next;
> > 		DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
> > 					 pCtxtclosed,
> > 					 (void *)pCtxtclosed->pid);
> > 	} else {
> > 		pTmp = pCtxtclosed->next;
> > 	}
> > 	pCtxtclosed = pTmp;
> > }
> >
> > Please let me know your comments.
> >
> > /Ernesto
> 
> Good point :)
> 
> I would like to simplify this use case ;)
> 
> If we call DSPProcessor_Attach() twice in the same process and kill the
> process,
> then it will leak memory for 1st instance of PROCESSOR object.
> 
> When we call open() on /dev/DspBridge a new PROCESS_CONTEXT is allocated,
> and it
> should be allocated **only once** in bridge_open() unlike in
> NODE_Allocate() and
> PROC_Attach(). PROCESS_CONTEXT tracks all the resources allocated on
> behalf of
> an open file handle(and not the process / thread). When this handle is
> closed
> all these resources should be freed in bridge_release(). Accountability of
> resources should be done using PROCESS_CONTEXT and **not pid (which will
> be
> different for different thread) / tgid (which will be different for parent
> and
> child).
> 
> Above problem occurs because PROCESS_CONTEXT by design tracks only one
> PROCESSOR
> object which gets freed in bridge_release().
> 
> Let me know your comments on this, and then we can proceed to fix this
> issue.
> 
> Cheers,
> Ameya.
You're right; I think using the PROCESS_CONTEXT to track the resources would resolve the issue. Also, with his approach we don't need to create a new context in the PROC_Attach /NODE_Allocate.

We can solve the issue by implementing a counter to track the number of calls to the PROC_Attach/Detach, so in that way we create a process handle only for the first time, and for the subsequent calls we need to return the existing handle. In the other hand PROC_Detach would be executed for the last call to this function.

I don't know yet how we would access or if there is an easy way to get the private_data as if get the pid using the "current", though.

/Ernesto

Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release

[Ameya Palande]

Hi Ernesto,

ext Ramos Falcon, Ernesto wrote:
> Hi Ameya,
> 
>> -----Original Message-----
>> From: Ameya Palande [mailto:ameya.palande@nokia.com]
>> Sent: Wednesday, August 05, 2009 4:52 PM
>> To: Ramos Falcon, Ernesto
>> Cc: linux-omap@vger.kernel.org; Doyu Hiroshi (Nokia-D/Helsinki); Guzman
>> Lugo, Fernando; Ramirez Luna, Omar; Tereshonkov Roman (Nokia-D/Helsinki);
>> Moogi, Suyog
>> Subject: Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to
>> bridge_release
>>
>> Hi Ernesto,
>>
>> ext Ramos Falcon, Ernesto wrote:
>>> Hi,
>>>
>>> We have detected a use case where if an application creates a child
>> process using fork call, and then the child and father processes call
>> DSPProcessor_Attach() and create a new process context with new tgid; when
>> the processes are terminated, only the last process calls bridge_release
>> cleaning only the resources in the father process, leaving the child
>> resources unreleased.
>>> One solution we have seen is to perform goes through the entire process
>> context list, clean up all the resources for all terminated processes or
>> in "zombie" state, as below,
>>> DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
>>> while (pCtxtclosed != NULL) {
>>> 	printk("pCtxtclosed->pid = %d\n",pCtxtclosed->pid);
>>> 	tsk = find_task_by_pid(pCtxtclosed->pid);
>>>
>>> 	if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
>>>
>>> 		GT_1trace(driverTrace, GT_5CLASS,
>>> 			"***Task structure not existing for "
>>> 			 "process***%d\n", pCtxtclosed->pid);
>>> 		DRV_RemoveAllResources(pCtxtclosed);
>>> 		if (pCtxtclosed->hProcessor != NULL) {
>>> 					PROC_Detach
>>> 						 (pCtxtclosed->hProcessor);
>>> 		}
>>> 		pTmp = pCtxtclosed->next;
>>> 		DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
>>> 					 pCtxtclosed,
>>> 					 (void *)pCtxtclosed->pid);
>>> 	} else {
>>> 		pTmp = pCtxtclosed->next;
>>> 	}
>>> 	pCtxtclosed = pTmp;
>>> }
>>>
>>> Please let me know your comments.
>>>
>>> /Ernesto
>> Good point :)
>>
>> I would like to simplify this use case ;)
>>
>> If we call DSPProcessor_Attach() twice in the same process and kill the
>> process,
>> then it will leak memory for 1st instance of PROCESSOR object.
>>
>> When we call open() on /dev/DspBridge a new PROCESS_CONTEXT is allocated,
>> and it
>> should be allocated **only once** in bridge_open() unlike in
>> NODE_Allocate() and
>> PROC_Attach(). PROCESS_CONTEXT tracks all the resources allocated on
>> behalf of
>> an open file handle(and not the process / thread). When this handle is
>> closed
>> all these resources should be freed in bridge_release(). Accountability of
>> resources should be done using PROCESS_CONTEXT and **not pid (which will
>> be
>> different for different thread) / tgid (which will be different for parent
>> and
>> child).
>>
>> Above problem occurs because PROCESS_CONTEXT by design tracks only one
>> PROCESSOR
>> object which gets freed in bridge_release().
>>
>> Let me know your comments on this, and then we can proceed to fix this
>> issue.
>>
>> Cheers,
>> Ameya.
> You're right; I think using the PROCESS_CONTEXT to track the resources would resolve the issue. Also, with his approach we don't need to create a new context in the PROC_Attach /NODE_Allocate.
> 
> We can solve the issue by implementing a counter to track the number of calls to the PROC_Attach/Detach, so in that way we create a process handle only for the first time, and for the subsequent calls we need to return the existing handle. In the other hand PROC_Detach would be executed for the last call to this function.
> 
> I don't know yet how we would access or if there is an easy way to get the private_data as if get the pid using the "current", though.
> 
> /Ernesto

Thanks for your comment :)
I am working on this and will try to provide a solution by 10th Aug 2009.

Cheers,
Ameya.