[PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Ameya Palande]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

cp_{to,fm}_usr break if between an if and an else (with no {}).

http://www.faqs.org/faqs/C-faq/abridged/
10.4: What's the best way to write a multi-statement macro?
A:    #define Func() do {stmt1; stmt2; ... } while(0) /* (no trailing ;) */

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |   42 ++++++++++++++++++++++------------------
 1 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 86812c6..aaf3019 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -147,25 +147,29 @@
 
 /* Following two macros should ideally have do{}while(0) */
 
-#define cp_fm_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(src == NULL) ||				\
-		unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_from_user failed, src=0x%x\n", src);  \
-		status = DSP_EPOINTER ; \
-	} \
-    }
-
-#define cp_to_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(dest == NULL) ||				\
-		unlikely(copy_to_user(dest, src, elements * sizeof(*(src))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_to_user failed, dest=0x%x\n", dest); \
-		status = DSP_EPOINTER ;\
-	} \
-    }
+#define cp_fm_usr(dest, src, status, elements)	\
+    do {						\
+	if (DSP_SUCCEEDED(status)) {			\
+	    if (unlikely((src) == NULL) ||		\
+		unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \
+		GT_1trace(WCD_debugMask, GT_7CLASS,	\
+			  "copy_from_user failed, src=0x%x\n", src);	\
+		(status) = DSP_EPOINTER ;				\
+	    }						\
+	}						\
+    } while (0)
+
+#define cp_to_usr(dest, src, status, elements) \
+    do {						\
+	if (DSP_SUCCEEDED(status)) {			\
+	    if (unlikely((dest) == NULL) ||				\
+		unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \
+		GT_1trace(WCD_debugMask, GT_7CLASS,	\
+			  "copy_to_user failed, dest=0x%x\n", dest);	\
+		(status) = DSP_EPOINTER ;		\
+	    }						\
+	}						\
+    } while (0)
 
 /* Device IOCtl function pointer */
 struct WCD_Cmd {
-- 
1.6.2.4

[PATCHv2 2/4] DSPBRIDGE: Heuristic fixes of strlen/malloc out by one

[Ameya Palande]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

I say 'heuristic', as I can't prove they're wrong, they just look
wrong, and for that reason should be given extra close scrutiny.
These are basically just the old malloc-one-more-than-strlen.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index aaf3019..563a1d8 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -532,8 +532,9 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args)
 	cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, status, 1);
 	if (DSP_FAILED(status))
 		goto func_end;
+	/* pathSize is increased by 1 to accommodate NULL */
 	pathSize = strlen_user((char *)
-				args->ARGS_MGR_REGISTEROBJECT.pszPathName);
+			args->ARGS_MGR_REGISTEROBJECT.pszPathName) + 1;
 	pszPathName = MEM_Alloc(pathSize, MEM_NONPAGED);
 	if (!pszPathName)
 		goto func_end;
@@ -544,7 +545,6 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args)
 		status = DSP_EPOINTER;
 		goto func_end;
 	}
-	pszPathName[pathSize] = '\0';
 
 	GT_1trace(WCD_debugMask, GT_ENTER,
 		 "MGRWRAP_RegisterObject: entered pg2hMsg "
@@ -904,7 +904,8 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 		if (argv[i] != NULL) {
                         /* User space pointer to argument */
                        temp = (char *) argv[i];
-                       len = strlen_user((char *)temp);
+			/* len is increased by 1 to accommodate NULL */
+			len = strlen_user((char *)temp) + 1;
 			/* Kernel space pointer to argument */
 			argv[i] = MEM_Alloc(len, MEM_NONPAGED);
 			if (argv[i] == NULL) {
@@ -914,7 +915,6 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 			cp_fm_usr(argv[i], temp, status, len);
 			if (DSP_FAILED(status))
 				goto func_cont;
-
 		}
 	}
 	/* TODO: validate this */
@@ -937,7 +937,8 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 		for (i = 0; DSP_SUCCEEDED(status) && (envp[i] != NULL); i++) {
                         /* User space pointer to argument */
                        temp = (char *)envp[i];
-                       len = strlen_user((char *)temp);
+			/* len is increased by 1 to accommodate NULL */
+			len = strlen_user((char *)temp) + 1;
 			/* Kernel space pointer to argument */
 			envp[i] = MEM_Alloc(len, MEM_NONPAGED);
 			if (envp[i] == NULL) {
-- 
1.6.2.4

[PATCHv2 3/4] DSPBRIDGE: PROCWRAP_Load function cleanup in a complete mess

[Ameya Palande]

If you followed some failure paths, it was entirely possible that
you'd attempt to MEM_Free a user-space pointer, because it wouldn't
have been replaced with a kernel-space copy yet.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |  110 +++++++++++++++++++++++-----------------
 1 files changed, 63 insertions(+), 47 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 563a1d8..811b2fc 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -884,99 +884,115 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 {
 	s32 i, len;
 	DSP_STATUS status = DSP_SOK;
-       char *temp;
-	s32 argc = args->ARGS_PROC_LOAD.iArgc;
+	char *temp;
+	s32 count = args->ARGS_PROC_LOAD.iArgc;
 	u8 **argv, **envp = NULL;
 
-
 	DBC_Require(argc > 0);
 	DBC_Require(argc <= MAX_LOADARGS);
 
-	argv = MEM_Alloc(argc * sizeof(u8 *), MEM_NONPAGED);
-	if (argv == NULL)
+	argv = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED);
+	if (!argv) {
 		status = DSP_EMEMORY;
+		goto func_cont;
+	}
 
-	cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, argc);
-	if (DSP_FAILED(status))
+	cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, count);
+	if (DSP_FAILED(status)) {
+		MEM_Free(argv);
+		argv = NULL;
 		goto func_cont;
+	}
 
-	for (i = 0; DSP_SUCCEEDED(status) && (i < argc); i++) {
-		if (argv[i] != NULL) {
-                        /* User space pointer to argument */
-                       temp = (char *) argv[i];
+	for (i = 0; DSP_SUCCEEDED(status) && (i < count); i++) {
+		if (argv[i]) {
+			/* User space pointer to argument */
+			temp = (char *) argv[i];
 			/* len is increased by 1 to accommodate NULL */
 			len = strlen_user((char *)temp) + 1;
 			/* Kernel space pointer to argument */
 			argv[i] = MEM_Alloc(len, MEM_NONPAGED);
-			if (argv[i] == NULL) {
+			if (argv[i]) {
+				cp_fm_usr(argv[i], temp, status, len);
+				if (DSP_FAILED(status)) {
+					MEM_Free(argv[i]);
+					argv[i] = NULL;
+					goto func_cont;
+				}
+			} else {
 				status = DSP_EMEMORY;
-				break;
-			}
-			cp_fm_usr(argv[i], temp, status, len);
-			if (DSP_FAILED(status))
 				goto func_cont;
+			}
 		}
 	}
 	/* TODO: validate this */
-	if (args->ARGS_PROC_LOAD.aEnvp != NULL) {
+	if (args->ARGS_PROC_LOAD.aEnvp) {
 		/* number of elements in the envp array including NULL */
-		len = 0;
+		count = 0;
 		do {
-			len++;
-                       get_user(temp, args->ARGS_PROC_LOAD.aEnvp);
-               } while (temp);
-		envp = MEM_Alloc(len * sizeof(u8 *), MEM_NONPAGED);
-		if (envp == NULL) {
+			get_user(temp, args->ARGS_PROC_LOAD.aEnvp + count);
+			count++;
+		} while (temp);
+		envp = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED);
+		if (!envp) {
 			status = DSP_EMEMORY;
 			goto func_cont;
 		}
 
-		cp_fm_usr(envp, args->ARGS_PROC_LOAD.aEnvp, status, len);
-		if (DSP_FAILED(status))
+		cp_fm_usr(envp, args->ARGS_PROC_LOAD.aEnvp, status, count);
+		if (DSP_FAILED(status)) {
+			MEM_Free(envp);
+			envp = NULL;
 			goto func_cont;
-		for (i = 0; DSP_SUCCEEDED(status) && (envp[i] != NULL); i++) {
-                        /* User space pointer to argument */
-                       temp = (char *)envp[i];
+		}
+		for (i = 0; DSP_SUCCEEDED(status) && envp[i]; i++) {
+			/* User space pointer to argument */
+			temp = (char *)envp[i];
 			/* len is increased by 1 to accommodate NULL */
 			len = strlen_user((char *)temp) + 1;
 			/* Kernel space pointer to argument */
 			envp[i] = MEM_Alloc(len, MEM_NONPAGED);
-			if (envp[i] == NULL) {
+			if (envp[i]) {
+				cp_fm_usr(envp[i], temp, status, len);
+				if (DSP_FAILED(status)) {
+					MEM_Free(envp[i]);
+					envp[i] = NULL;
+					goto func_cont;
+				}
+			} else {
 				status = DSP_EMEMORY;
-				break;
-			}
-			cp_fm_usr(envp[i], temp, status, len);
-			if (DSP_FAILED(status))
 				goto func_cont;
+			}
 		}
 	}
 	GT_5trace(WCD_debugMask, GT_ENTER,
-		 "PROCWRAP_Load, hProcessor: 0x%x\n\tiArgc:"
-		 "0x%x\n\taArgv: 0x%x\n\taArgv[0]: %s\n\taEnvp: 0x%0x\n",
-		 args->ARGS_PROC_LOAD.hProcessor,
-		 args->ARGS_PROC_LOAD.iArgc, args->ARGS_PROC_LOAD.aArgv,
-		 argv[0], args->ARGS_PROC_LOAD.aEnvp);
+		"PROCWRAP_Load, hProcessor: 0x%x\n\tiArgc:"
+		"0x%x\n\taArgv: 0x%x\n\taArgv[0]: %s\n\taEnvp: 0x%0x\n",
+		args->ARGS_PROC_LOAD.hProcessor,
+		args->ARGS_PROC_LOAD.iArgc, args->ARGS_PROC_LOAD.aArgv,
+		argv[0], args->ARGS_PROC_LOAD.aEnvp);
 	if (DSP_SUCCEEDED(status)) {
 		status = PROC_Load(args->ARGS_PROC_LOAD.hProcessor,
-				  args->ARGS_PROC_LOAD.iArgc,
-				  (CONST char **)argv, (CONST char **)envp);
+				args->ARGS_PROC_LOAD.iArgc,
+				(CONST char **)argv, (CONST char **)envp);
 	}
 func_cont:
-	if (envp != NULL) {
+	if (envp) {
 		i = 0;
-		while (envp[i] != NULL)
+		while (envp[i])
 			MEM_Free(envp[i++]);
 
 		MEM_Free(envp);
 	}
-	if (argv != NULL) {
-		for (i = 0; i < argc; i++) {
-			if (argv[i] != NULL)
-				MEM_Free(argv[i]);
 
-		}
+	if (argv) {
+		count = args->ARGS_PROC_LOAD.iArgc;
+		for (i = 0; (i < count) && argv[i]; i++)
+			MEM_Free(argv[i]);
+
 		MEM_Free(argv);
 	}
+
 	return status;
 }
 
-- 
1.6.2.4

[PATCH 4/4] DSPBRIDGE: Remove unnecessary conditions from some for loops

[Ameya Palande]

The status cannot be a failure value at the start of the loops,
and the only changes to a failure state are accompanied by a
break or goto, so these conditions are always true.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 811b2fc..6b2136a 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -904,7 +904,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 		goto func_cont;
 	}
 
-	for (i = 0; DSP_SUCCEEDED(status) && (i < count); i++) {
+	for (i = 0; i < count; i++) {
 		if (argv[i]) {
 			/* User space pointer to argument */
 			temp = (char *) argv[i];
@@ -945,7 +945,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 			envp = NULL;
 			goto func_cont;
 		}
-		for (i = 0; DSP_SUCCEEDED(status) && envp[i]; i++) {
+		for (i = 0; envp[i]; i++) {
 			/* User space pointer to argument */
 			temp = (char *)envp[i];
 			/* len is increased by 1 to accommodate NULL */
-- 
1.6.2.4

RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Guzman Lugo, Fernando]

Thanks for the patch, it only has indentation problems, this is the checkpatch output:

WARNING: suspect code indent for conditional statements (8, 12)
#34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
+       if (DSP_SUCCEEDED(status)) {                    \
+           if (unlikely((src) == NULL) ||              \

WARNING: line over 80 characters
#36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
+               unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \

WARNING: suspect code indent for conditional statements (8, 12)
#46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
+       if (DSP_SUCCEEDED(status)) {                    \
+           if (unlikely((dest) == NULL) ||                             \

WARNING: line over 80 characters
#48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
+               unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \

total: 0 errors, 4 warnings, 48 lines checked

Could you please fix that warning please?

Regards,
Fernando.


> -----Original Message-----
> From: Ameya Palande [mailto:ameya.palande@nokia.com]
> Sent: Tuesday, July 07, 2009 10:02 AM
> To: linux-omap@vger.kernel.org
> Cc: Guzman Lugo, Fernando; Kanigeri, Hari; ext-phil.2.carmody@nokia.com
> Subject: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an
> if/else
> 
> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> cp_{to,fm}_usr break if between an if and an else (with no {}).
> 
> http://www.faqs.org/faqs/C-faq/abridged/
> 10.4: What's the best way to write a multi-statement macro?
> A:    #define Func() do {stmt1; stmt2; ... } while(0) /* (no trailing ;)
> */
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  drivers/dsp/bridge/pmgr/wcd.c |   42 ++++++++++++++++++++++--------------
> ----
>  1 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
> index 86812c6..aaf3019 100644
> --- a/drivers/dsp/bridge/pmgr/wcd.c
> +++ b/drivers/dsp/bridge/pmgr/wcd.c
> @@ -147,25 +147,29 @@
> 
>  /* Following two macros should ideally have do{}while(0) */
> 
> -#define cp_fm_usr(dest, src, status, elements)    \
> -    if (DSP_SUCCEEDED(status)) {\
> -	    if (unlikely(src == NULL) ||				\
> -		unlikely(copy_from_user(dest, src, elements *
> sizeof(*(dest))))) { \
> -		GT_1trace(WCD_debugMask, GT_7CLASS, \
> -		"copy_from_user failed, src=0x%x\n", src);  \
> -		status = DSP_EPOINTER ; \
> -	} \
> -    }
> -
> -#define cp_to_usr(dest, src, status, elements)    \
> -    if (DSP_SUCCEEDED(status)) {\
> -	    if (unlikely(dest == NULL) ||				\
> -		unlikely(copy_to_user(dest, src, elements * sizeof(*(src)))))
> { \
> -		GT_1trace(WCD_debugMask, GT_7CLASS, \
> -		"copy_to_user failed, dest=0x%x\n", dest); \
> -		status = DSP_EPOINTER ;\
> -	} \
> -    }
> +#define cp_fm_usr(dest, src, status, elements)	\
> +    do {						\
> +	if (DSP_SUCCEEDED(status)) {			\
> +	    if (unlikely((src) == NULL) ||		\
> +		unlikely(copy_from_user(dest, src, (elements) *
> sizeof(*(dest))))) { \
> +		GT_1trace(WCD_debugMask, GT_7CLASS,	\
> +			  "copy_from_user failed, src=0x%x\n", src);	\
> +		(status) = DSP_EPOINTER ;				\
> +	    }						\
> +	}						\
> +    } while (0)
> +
> +#define cp_to_usr(dest, src, status, elements) \
> +    do {						\
> +	if (DSP_SUCCEEDED(status)) {			\
> +	    if (unlikely((dest) == NULL) ||				\
> +		unlikely(copy_to_user(dest, src, (elements) *
> sizeof(*(src))))) { \
> +		GT_1trace(WCD_debugMask, GT_7CLASS,	\
> +			  "copy_to_user failed, dest=0x%x\n", dest);	\
> +		(status) = DSP_EPOINTER ;		\
> +	    }						\
> +	}						\
> +    } while (0)
> 
>  /* Device IOCtl function pointer */
>  struct WCD_Cmd {
> --
> 1.6.2.4
> 

RE: [PATCHv2 2/4] DSPBRIDGE: Heuristic fixes of strlen/malloc out by one

[Guzman Lugo, Fernando]

Thanks good patch.

> -----Original Message-----
> From: Ameya Palande [mailto:ameya.palande@nokia.com]
> Sent: Tuesday, July 07, 2009 10:02 AM
> To: linux-omap@vger.kernel.org
> Cc: Guzman Lugo, Fernando; Kanigeri, Hari; ext-phil.2.carmody@nokia.com
> Subject: [PATCHv2 2/4] DSPBRIDGE: Heuristic fixes of strlen/malloc out by
> one
> 
> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> I say 'heuristic', as I can't prove they're wrong, they just look
> wrong, and for that reason should be given extra close scrutiny.
> These are basically just the old malloc-one-more-than-strlen.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  drivers/dsp/bridge/pmgr/wcd.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
> index aaf3019..563a1d8 100644
> --- a/drivers/dsp/bridge/pmgr/wcd.c
> +++ b/drivers/dsp/bridge/pmgr/wcd.c
> @@ -532,8 +532,9 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args)
>  	cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, status, 1);
>  	if (DSP_FAILED(status))
>  		goto func_end;
> +	/* pathSize is increased by 1 to accommodate NULL */
>  	pathSize = strlen_user((char *)
> -				args->ARGS_MGR_REGISTEROBJECT.pszPathName);
> +			args->ARGS_MGR_REGISTEROBJECT.pszPathName) + 1;
>  	pszPathName = MEM_Alloc(pathSize, MEM_NONPAGED);
>  	if (!pszPathName)
>  		goto func_end;
> @@ -544,7 +545,6 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args)
>  		status = DSP_EPOINTER;
>  		goto func_end;
>  	}
> -	pszPathName[pathSize] = '\0';
> 
>  	GT_1trace(WCD_debugMask, GT_ENTER,
>  		 "MGRWRAP_RegisterObject: entered pg2hMsg "
> @@ -904,7 +904,8 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
>  		if (argv[i] != NULL) {
>                          /* User space pointer to argument */
>                         temp = (char *) argv[i];
> -                       len = strlen_user((char *)temp);
> +			/* len is increased by 1 to accommodate NULL */
> +			len = strlen_user((char *)temp) + 1;
>  			/* Kernel space pointer to argument */
>  			argv[i] = MEM_Alloc(len, MEM_NONPAGED);
>  			if (argv[i] == NULL) {
> @@ -914,7 +915,6 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
>  			cp_fm_usr(argv[i], temp, status, len);
>  			if (DSP_FAILED(status))
>  				goto func_cont;
> -
>  		}
>  	}
>  	/* TODO: validate this */
> @@ -937,7 +937,8 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
>  		for (i = 0; DSP_SUCCEEDED(status) && (envp[i] != NULL); i++) {
>                          /* User space pointer to argument */
>                         temp = (char *)envp[i];
> -                       len = strlen_user((char *)temp);
> +			/* len is increased by 1 to accommodate NULL */
> +			len = strlen_user((char *)temp) + 1;
>  			/* Kernel space pointer to argument */
>  			envp[i] = MEM_Alloc(len, MEM_NONPAGED);
>  			if (envp[i] == NULL) {
> --
> 1.6.2.4
> 

Acked-by Fernando Guzman Lugo <x0095840@ti.com>

RE: [PATCH 4/4] DSPBRIDGE: Remove unnecessary conditions from some for loops

[Guzman Lugo, Fernando]

Good.

Regards,
Fernando.

> -----Original Message-----
> From: Ameya Palande [mailto:ameya.palande@nokia.com]
> Sent: Tuesday, July 07, 2009 10:02 AM
> To: linux-omap@vger.kernel.org
> Cc: Guzman Lugo, Fernando; Kanigeri, Hari; ext-phil.2.carmody@nokia.com
> Subject: [PATCH 4/4] DSPBRIDGE: Remove unnecessary conditions from some
> for loops
> 
> The status cannot be a failure value at the start of the loops,
> and the only changes to a failure state are accompanied by a
> break or goto, so these conditions are always true.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  drivers/dsp/bridge/pmgr/wcd.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
> index 811b2fc..6b2136a 100644
> --- a/drivers/dsp/bridge/pmgr/wcd.c
> +++ b/drivers/dsp/bridge/pmgr/wcd.c
> @@ -904,7 +904,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
>  		goto func_cont;
>  	}
> 
> -	for (i = 0; DSP_SUCCEEDED(status) && (i < count); i++) {
> +	for (i = 0; i < count; i++) {
>  		if (argv[i]) {
>  			/* User space pointer to argument */
>  			temp = (char *) argv[i];
> @@ -945,7 +945,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
>  			envp = NULL;
>  			goto func_cont;
>  		}
> -		for (i = 0; DSP_SUCCEEDED(status) && envp[i]; i++) {
> +		for (i = 0; envp[i]; i++) {
>  			/* User space pointer to argument */
>  			temp = (char *)envp[i];
>  			/* len is increased by 1 to accommodate NULL */
> --
> 1.6.2.4
> 

Acked-by Fernando Guzman Lugo <x0095840@ti.com>

RE: [PATCHv2 3/4] DSPBRIDGE: PROCWRAP_Load function cleanup in a complete mess

[Guzman Lugo, Fernando]

Hi,

Could you please review this patch? I am getting corrupted patch when I try to apply.

Please see comments below.

Regards,
Fernando.

> -----Original Message-----
> From: Ameya Palande [mailto:ameya.palande@nokia.com]
> Sent: Tuesday, July 07, 2009 10:02 AM
> To: linux-omap@vger.kernel.org
> Cc: Guzman Lugo, Fernando; Kanigeri, Hari; ext-phil.2.carmody@nokia.com
> Subject: [PATCHv2 3/4] DSPBRIDGE: PROCWRAP_Load function cleanup in a
> complete mess
> 
> If you followed some failure paths, it was entirely possible that
> you'd attempt to MEM_Free a user-space pointer, because it wouldn't
> have been replaced with a kernel-space copy yet.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
> ---
>  drivers/dsp/bridge/pmgr/wcd.c |  110 +++++++++++++++++++++++-------------
> ----
>  1 files changed, 63 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
> index 563a1d8..811b2fc 100644
> --- a/drivers/dsp/bridge/pmgr/wcd.c
> +++ b/drivers/dsp/bridge/pmgr/wcd.c
> @@ -884,99 +884,115 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
>  {
>  	s32 i, len;
>  	DSP_STATUS status = DSP_SOK;
> -       char *temp;
> -	s32 argc = args->ARGS_PROC_LOAD.iArgc;
> +	char *temp;
> +	s32 count = args->ARGS_PROC_LOAD.iArgc;
>  	u8 **argv, **envp = NULL;
> 
> -
>  	DBC_Require(argc > 0);
>  	DBC_Require(argc <= MAX_LOADARGS);

These two line will cause a compilation error since you removed argc and replace it by count. Please change argc in these two lines too.	
	
> 
> -	argv = MEM_Alloc(argc * sizeof(u8 *), MEM_NONPAGED);
> -	if (argv == NULL)
> +	argv = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED);
> +	if (!argv) {
>  		status = DSP_EMEMORY;
> +		goto func_cont;
> +	}
> 
> -	cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, argc);
> -	if (DSP_FAILED(status))
> +	cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, count);
> +	if (DSP_FAILED(status)) {
> +		MEM_Free(argv);
> +		argv = NULL;
>  		goto func_cont;
> +	}
> 
> -	for (i = 0; DSP_SUCCEEDED(status) && (i < argc); i++) {
> -		if (argv[i] != NULL) {
> -                        /* User space pointer to argument */
> -                       temp = (char *) argv[i];
> +	for (i = 0; DSP_SUCCEEDED(status) && (i < count); i++) {
> +		if (argv[i]) {
> +			/* User space pointer to argument */
> +			temp = (char *) argv[i];
>  			/* len is increased by 1 to accommodate NULL */
>  			len = strlen_user((char *)temp) + 1;
>  			/* Kernel space pointer to argument */
>  			argv[i] = MEM_Alloc(len, MEM_NONPAGED);
> -			if (argv[i] == NULL) {
> +			if (argv[i]) {
> +				cp_fm_usr(argv[i], temp, status, len);
> +				if (DSP_FAILED(status)) {
> +					MEM_Free(argv[i]);
> +					argv[i] = NULL;
> +					goto func_cont;
> +				}
> +			} else {
>  				status = DSP_EMEMORY;
> -				break;
> -			}
> -			cp_fm_usr(argv[i], temp, status, len);
> -			if (DSP_FAILED(status))
>  				goto func_cont;
> +			}
>  		}
>  	}
>  	/* TODO: validate this */
> -	if (args->ARGS_PROC_LOAD.aEnvp != NULL) {
> +	if (args->ARGS_PROC_LOAD.aEnvp) {
>  		/* number of elements in the envp array including NULL */
> -		len = 0;
> +		count = 0;
>  		do {
> -			len++;
> -                       get_user(temp, args->ARGS_PROC_LOAD.aEnvp);
> -               } while (temp);
> -		envp = MEM_Alloc(len * sizeof(u8 *), MEM_NONPAGED);
> -		if (envp == NULL) {
> +			get_user(temp, args->ARGS_PROC_LOAD.aEnvp + count);
> +			count++;
> +		} while (temp);
> +		envp = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED);
> +		if (!envp) {
>  			status = DSP_EMEMORY;
>  			goto func_cont;
>  		}
> 
> -		cp_fm_usr(envp, args->ARGS_PROC_LOAD.aEnvp, status, len);
> -		if (DSP_FAILED(status))
> +		cp_fm_usr(envp, args->ARGS_PROC_LOAD.aEnvp, status, count);
> +		if (DSP_FAILED(status)) {
> +			MEM_Free(envp);
> +			envp = NULL;
>  			goto func_cont;
> -		for (i = 0; DSP_SUCCEEDED(status) && (envp[i] != NULL); i++) {
> -                        /* User space pointer to argument */
> -                       temp = (char *)envp[i];
> +		}
> +		for (i = 0; DSP_SUCCEEDED(status) && envp[i]; i++) {
> +			/* User space pointer to argument */
> +			temp = (char *)envp[i];
>  			/* len is increased by 1 to accommodate NULL */
>  			len = strlen_user((char *)temp) + 1;
>  			/* Kernel space pointer to argument */
>  			envp[i] = MEM_Alloc(len, MEM_NONPAGED);
> -			if (envp[i] == NULL) {
> +			if (envp[i]) {
> +				cp_fm_usr(envp[i], temp, status, len);
> +				if (DSP_FAILED(status)) {
> +					MEM_Free(envp[i]);
> +					envp[i] = NULL;
> +					goto func_cont;
> +				}
> +			} else {
>  				status = DSP_EMEMORY;
> -				break;
> -			}
> -			cp_fm_usr(envp[i], temp, status, len);
> -			if (DSP_FAILED(status))
>  				goto func_cont;
> +			}
>  		}
>  	}
>  	GT_5trace(WCD_debugMask, GT_ENTER,
> -		 "PROCWRAP_Load, hProcessor: 0x%x\n\tiArgc:"
> -		 "0x%x\n\taArgv: 0x%x\n\taArgv[0]: %s\n\taEnvp: 0x%0x\n",
> -		 args->ARGS_PROC_LOAD.hProcessor,
> -		 args->ARGS_PROC_LOAD.iArgc, args->ARGS_PROC_LOAD.aArgv,
> -		 argv[0], args->ARGS_PROC_LOAD.aEnvp);
> +		"PROCWRAP_Load, hProcessor: 0x%x\n\tiArgc:"
> +		"0x%x\n\taArgv: 0x%x\n\taArgv[0]: %s\n\taEnvp: 0x%0x\n",
> +		args->ARGS_PROC_LOAD.hProcessor,
> +		args->ARGS_PROC_LOAD.iArgc, args->ARGS_PROC_LOAD.aArgv,
> +		argv[0], args->ARGS_PROC_LOAD.aEnvp);
>  	if (DSP_SUCCEEDED(status)) {
>  		status = PROC_Load(args->ARGS_PROC_LOAD.hProcessor,
> -				  args->ARGS_PROC_LOAD.iArgc,
> -				  (CONST char **)argv, (CONST char **)envp);
> +				args->ARGS_PROC_LOAD.iArgc,
> +				(CONST char **)argv, (CONST char **)envp);
>  	}
>  func_cont:
> -	if (envp != NULL) {
> +	if (envp) {
>  		i = 0;
> -		while (envp[i] != NULL)
> +		while (envp[i])
>  			MEM_Free(envp[i++]);
> 
>  		MEM_Free(envp);
>  	}
> -	if (argv != NULL) {
> -		for (i = 0; i < argc; i++) {
> -			if (argv[i] != NULL)
> -				MEM_Free(argv[i]);
> 
> -		}
> +	if (argv) {
> +		count = args->ARGS_PROC_LOAD.iArgc;
> +		for (i = 0; (i < count) && argv[i]; i++)
> +			MEM_Free(argv[i]);
> +
>  		MEM_Free(argv);
>  	}
> +
>  	return status;
>  }
> 
> --
> 1.6.2.4
> 

[PATCHv3 3/4] DSPBRIDGE: PROCWRAP_Load function cleanup in a complete mess

[Ameya Palande]

If you followed some failure paths, it was entirely possible that
you'd attempt to MEM_Free a user-space pointer, because it wouldn't
have been replaced with a kernel-space copy yet. Now ensure there's
a NULL pointer to stop the cleanup at the position of the first error.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
Signed-off-by: Ameya Palande <ameya.palande@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |  114 +++++++++++++++++++++++-----------------
 1 files changed, 65 insertions(+), 49 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 314ee49..d9de5af 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -884,99 +884,115 @@ u32 PROCWRAP_Load(union Trapped_Args *args)
 {
 	s32 i, len;
 	DSP_STATUS status = DSP_SOK;
-       char *temp;
-	s32 argc = args->ARGS_PROC_LOAD.iArgc;
+	char *temp;
+	s32 count = args->ARGS_PROC_LOAD.iArgc;
 	u8 **argv, **envp = NULL;
 
+	DBC_Require(count > 0);
+	DBC_Require(count <= MAX_LOADARGS);
 
-	DBC_Require(argc > 0);
-	DBC_Require(argc <= MAX_LOADARGS);
-
-	argv = MEM_Alloc(argc * sizeof(u8 *), MEM_NONPAGED);
-	if (argv == NULL)
+	argv = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED);
+	if (!argv) {
 		status = DSP_EMEMORY;
+		goto func_cont;
+	}
 
-	cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, argc);
-	if (DSP_FAILED(status))
+	cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, count);
+	if (DSP_FAILED(status)) {
+		MEM_Free(argv);
+		argv = NULL;
 		goto func_cont;
+	}
 
-	for (i = 0; DSP_SUCCEEDED(status) && (i < argc); i++) {
-		if (argv[i] != NULL) {
-                        /* User space pointer to argument */
-                       temp = (char *) argv[i];
+	for (i = 0; DSP_SUCCEEDED(status) && (i < count); i++) {
+		if (argv[i]) {
+			/* User space pointer to argument */
+			temp = (char *) argv[i];
 			/* len is increased by 1 to accommodate NULL */
 			len = strlen_user((char *)temp) + 1;
 			/* Kernel space pointer to argument */
 			argv[i] = MEM_Alloc(len, MEM_NONPAGED);
-			if (argv[i] == NULL) {
+			if (argv[i]) {
+				cp_fm_usr(argv[i], temp, status, len);
+				if (DSP_FAILED(status)) {
+					MEM_Free(argv[i]);
+					argv[i] = NULL;
+					goto func_cont;
+				}
+			} else {
 				status = DSP_EMEMORY;
-				break;
-			}
-			cp_fm_usr(argv[i], temp, status, len);
-			if (DSP_FAILED(status))
 				goto func_cont;
+			}
 		}
 	}
 	/* TODO: validate this */
-	if (args->ARGS_PROC_LOAD.aEnvp != NULL) {
+	if (args->ARGS_PROC_LOAD.aEnvp) {
 		/* number of elements in the envp array including NULL */
-		len = 0;
+		count = 0;
 		do {
-			len++;
-                       get_user(temp, args->ARGS_PROC_LOAD.aEnvp);
-               } while (temp);
-		envp = MEM_Alloc(len * sizeof(u8 *), MEM_NONPAGED);
-		if (envp == NULL) {
+			get_user(temp, args->ARGS_PROC_LOAD.aEnvp + count);
+			count++;
+		} while (temp);
+		envp = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED);
+		if (!envp) {
 			status = DSP_EMEMORY;
 			goto func_cont;
 		}
 
-		cp_fm_usr(envp, args->ARGS_PROC_LOAD.aEnvp, status, len);
-		if (DSP_FAILED(status))
+		cp_fm_usr(envp, args->ARGS_PROC_LOAD.aEnvp, status, count);
+		if (DSP_FAILED(status)) {
+			MEM_Free(envp);
+			envp = NULL;
 			goto func_cont;
-		for (i = 0; DSP_SUCCEEDED(status) && (envp[i] != NULL); i++) {
-                        /* User space pointer to argument */
-                       temp = (char *)envp[i];
+		}
+		for (i = 0; DSP_SUCCEEDED(status) && envp[i]; i++) {
+			/* User space pointer to argument */
+			temp = (char *)envp[i];
 			/* len is increased by 1 to accommodate NULL */
 			len = strlen_user((char *)temp) + 1;
 			/* Kernel space pointer to argument */
 			envp[i] = MEM_Alloc(len, MEM_NONPAGED);
-			if (envp[i] == NULL) {
+			if (envp[i]) {
+				cp_fm_usr(envp[i], temp, status, len);
+				if (DSP_FAILED(status)) {
+					MEM_Free(envp[i]);
+					envp[i] = NULL;
+					goto func_cont;
+				}
+			} else {
 				status = DSP_EMEMORY;
-				break;
-			}
-			cp_fm_usr(envp[i], temp, status, len);
-			if (DSP_FAILED(status))
 				goto func_cont;
+			}
 		}
 	}
 	GT_5trace(WCD_debugMask, GT_ENTER,
-		 "PROCWRAP_Load, hProcessor: 0x%x\n\tiArgc:"
-		 "0x%x\n\taArgv: 0x%x\n\taArgv[0]: %s\n\taEnvp: 0x%0x\n",
-		 args->ARGS_PROC_LOAD.hProcessor,
-		 args->ARGS_PROC_LOAD.iArgc, args->ARGS_PROC_LOAD.aArgv,
-		 argv[0], args->ARGS_PROC_LOAD.aEnvp);
+		"PROCWRAP_Load, hProcessor: 0x%x\n\tiArgc:"
+		"0x%x\n\taArgv: 0x%x\n\taArgv[0]: %s\n\taEnvp: 0x%0x\n",
+		args->ARGS_PROC_LOAD.hProcessor,
+		args->ARGS_PROC_LOAD.iArgc, args->ARGS_PROC_LOAD.aArgv,
+		argv[0], args->ARGS_PROC_LOAD.aEnvp);
 	if (DSP_SUCCEEDED(status)) {
 		status = PROC_Load(args->ARGS_PROC_LOAD.hProcessor,
-				  args->ARGS_PROC_LOAD.iArgc,
-				  (CONST char **)argv, (CONST char **)envp);
+				args->ARGS_PROC_LOAD.iArgc,
+				(CONST char **)argv, (CONST char **)envp);
 	}
 func_cont:
-	if (envp != NULL) {
+	if (envp) {
 		i = 0;
-		while (envp[i] != NULL)
+		while (envp[i])
 			MEM_Free(envp[i++]);
 
 		MEM_Free(envp);
 	}
-	if (argv != NULL) {
-		for (i = 0; i < argc; i++) {
-			if (argv[i] != NULL)
-				MEM_Free(argv[i]);
 
-		}
+	if (argv) {
+		count = args->ARGS_PROC_LOAD.iArgc;
+		for (i = 0; (i < count) && argv[i]; i++)
+			MEM_Free(argv[i]);
+
 		MEM_Free(argv);
 	}
+
 	return status;
 }
 
-- 
1.6.2.4

[PATCHv2 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Ameya Palande]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

cp_{to,fm}_usr break if between an if and an else (with no {}).

http://www.faqs.org/faqs/C-faq/abridged/
10.4: What's the best way to write a multi-statement macro?
A:    #define Func() do {stmt1; stmt2; ... } while(0) /* (no trailing ;) */

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |   42 ++++++++++++++++++++++------------------
 1 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 86812c6..da0467c 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -147,25 +147,29 @@
 
 /* Following two macros should ideally have do{}while(0) */
 
-#define cp_fm_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(src == NULL) ||				\
-		unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_from_user failed, src=0x%x\n", src);  \
-		status = DSP_EPOINTER ; \
-	} \
-    }
-
-#define cp_to_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(dest == NULL) ||				\
-		unlikely(copy_to_user(dest, src, elements * sizeof(*(src))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_to_user failed, dest=0x%x\n", dest); \
-		status = DSP_EPOINTER ;\
-	} \
-    }
+#define cp_fm_usr(dest, src, status, elements)	\
+do {						\
+  if (DSP_SUCCEEDED(status)) {			\
+    if (unlikely((src) == NULL) ||		\
+      unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \
+        GT_1trace(WCD_debugMask, GT_7CLASS,	\
+	  "copy_from_user failed, src=0x%x\n", src);	\
+      (status) = DSP_EPOINTER ;			\
+    }						\
+  }						\
+} while (0)
+
+#define cp_to_usr(dest, src, status, elements)	\
+do {						\
+  if (DSP_SUCCEEDED(status)) {			\
+    if (unlikely((dest) == NULL) ||		\
+      unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \
+        GT_1trace(WCD_debugMask, GT_7CLASS,	\
+          "copy_to_user failed, dest=0x%x\n", dest);	\
+        (status) = DSP_EPOINTER ;		\
+    }						\
+  }						\
+} while (0)
 
 /* Device IOCtl function pointer */
 struct WCD_Cmd {
-- 
1.6.2.4

Re: [PATCHv2 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Ameya Palande]

I still get following warnings from checkpatch.pl:

WARNING: suspect code indent for conditional statements (0, 2)
#45: FILE: drivers/dsp/bridge/pmgr/wcd.c:151:
+do {						\
+  if (DSP_SUCCEEDED(status)) {			\

WARNING: suspect code indent for conditional statements (2, 4)
#46: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
+  if (DSP_SUCCEEDED(status)) {			\
+    if (unlikely((src) == NULL) ||		\

ERROR: code indent should use tabs where possible
#49: FILE: drivers/dsp/bridge/pmgr/wcd.c:155:
+        GT_1trace(WCD_debugMask, GT_7CLASS,^I\$

WARNING: suspect code indent for conditional statements (0, 2)
#57: FILE: drivers/dsp/bridge/pmgr/wcd.c:163:
+do {						\
+  if (DSP_SUCCEEDED(status)) {			\

WARNING: suspect code indent for conditional statements (2, 4)
#58: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
+  if (DSP_SUCCEEDED(status)) {			\
+    if (unlikely((dest) == NULL) ||		\

ERROR: code indent should use tabs where possible
#61: FILE: drivers/dsp/bridge/pmgr/wcd.c:167:
+        GT_1trace(WCD_debugMask, GT_7CLASS,^I\$

ERROR: code indent should use tabs where possible
#62: FILE: drivers/dsp/bridge/pmgr/wcd.c:168:
+          "copy_to_user failed, dest=0x%x\n", dest);^I\$

ERROR: code indent should use tabs where possible
#63: FILE: drivers/dsp/bridge/pmgr/wcd.c:169:
+        (status) = DSP_EPOINTER ;^I^I\$

total: 4 errors, 4 warnings, 48 lines checked

I have used 2 spaces instead of a tab for indentation so that line
won't exceed 80 columns limit, and we still maintain good amount of
readability with this macro.

Cheers,
Ameya.

RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Phil Carmody]

On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote:
> Thanks for the patch, it only has indentation problems, this is the checkpatch output:
> 
> WARNING: suspect code indent for conditional statements (8, 12)
> #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
> +       if (DSP_SUCCEEDED(status)) {                    \
> +           if (unlikely((src) == NULL) ||              \
> 
> WARNING: line over 80 characters
> #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
> +               unlikely(copy_from_user(dest, src, (elements) * sizeof(*(dest))))) { \
> 
> WARNING: suspect code indent for conditional statements (8, 12)
> #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
> +       if (DSP_SUCCEEDED(status)) {                    \
> +           if (unlikely((dest) == NULL) ||                             \
> 
> WARNING: line over 80 characters
> #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
> +               unlikely(copy_to_user(dest, src, (elements) * sizeof(*(src))))) { \
> 
> total: 0 errors, 4 warnings, 48 lines checked
> 
> Could you please fix that warning please?

I suspect the only way to fix those warnings would either introduce
other warnings, or \
							would \
							lead \
							to \
							utterly \
							unread- \
							able \
							code.

If you check how and why the original TI-originated version of the code
does not follow the linux coding standards, the difficulties we would
have making a warning-free patch of it should be apparent.

Phil

RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Menon, Nishanth]

Phil,
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Phil Carmody
> Sent: Tuesday, July 14, 2009 6:03 AM
> 
> On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote:
> > Thanks for the patch, it only has indentation problems, this is the
> checkpatch output:
> >
> > WARNING: suspect code indent for conditional statements (8, 12)
> > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
> > +       if (DSP_SUCCEEDED(status)) {                    \
> > +           if (unlikely((src) == NULL) ||              \
> >
> > WARNING: line over 80 characters
> > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
> > +               unlikely(copy_from_user(dest, src, (elements) *
> sizeof(*(dest))))) { \
> >
> > WARNING: suspect code indent for conditional statements (8, 12)
> > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
> > +       if (DSP_SUCCEEDED(status)) {                    \
> > +           if (unlikely((dest) == NULL) ||
> \
> >
> > WARNING: line over 80 characters
> > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
> > +               unlikely(copy_to_user(dest, src, (elements) *
> sizeof(*(src))))) { \
> >
> > total: 0 errors, 4 warnings, 48 lines checked
> >
> > Could you please fix that warning please?
> 
> I suspect the only way to fix those warnings would either introduce
> other warnings, or \
Gentle query: Have we already tried this or is it just a suspicion?


> 							would \
> 							lead \
> 							to \
> 							utterly \
> 							unread- \
> 							able \
> 							code.
> 
> If you check how and why the original TI-originated version of the code
> does not follow the linux coding standards, the difficulties we would
> have making a warning-free patch of it should be apparent.

Past is past. The idea was not to introduce anymore warning code.

Regards,
Nishanth Menon

Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Ameya Palande]

ext Menon, Nishanth wrote:
> Phil,
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Phil Carmody
>> Sent: Tuesday, July 14, 2009 6:03 AM
>>
>> On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote:
>>> Thanks for the patch, it only has indentation problems, this is the
>> checkpatch output:
>>> WARNING: suspect code indent for conditional statements (8, 12)
>>> #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
>>> +       if (DSP_SUCCEEDED(status)) {                    \
>>> +           if (unlikely((src) == NULL) ||              \
>>>
>>> WARNING: line over 80 characters
>>> #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
>>> +               unlikely(copy_from_user(dest, src, (elements) *
>> sizeof(*(dest))))) { \
>>> WARNING: suspect code indent for conditional statements (8, 12)
>>> #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
>>> +       if (DSP_SUCCEEDED(status)) {                    \
>>> +           if (unlikely((dest) == NULL) ||
>> \
>>> WARNING: line over 80 characters
>>> #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
>>> +               unlikely(copy_to_user(dest, src, (elements) *
>> sizeof(*(src))))) { \
>>> total: 0 errors, 4 warnings, 48 lines checked
>>>
>>> Could you please fix that warning please?
>> I suspect the only way to fix those warnings would either introduce
>> other warnings, or \
> Gentle query: Have we already tried this or is it just a suspicion?
> 
> 
>> 							would \
>> 							lead \
>> 							to \
>> 							utterly \
>> 							unread- \
>> 							able \
>> 							code.
>>
>> If you check how and why the original TI-originated version of the code
>> does not follow the linux coding standards, the difficulties we would
>> have making a warning-free patch of it should be apparent.
> 
> Past is past. The idea was not to introduce anymore warning code.

I agree but if you want to stick to this, then I don't think we can get a
readable macro. And I personally prefer readability over warnings generated by
checkpatch.pl script.

Cheers,
Ameya.

RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Phil Carmody]

On Tue, 2009-07-14 at 13:05 +0200, ext Menon, Nishanth wrote:
> Phil,
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Phil Carmody
> > Sent: Tuesday, July 14, 2009 6:03 AM
> > 
> > On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote:
> > > Thanks for the patch, it only has indentation problems, this is the
> > checkpatch output:
> > >
> > > WARNING: suspect code indent for conditional statements (8, 12)
> > > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
> > > +       if (DSP_SUCCEEDED(status)) {                    \
> > > +           if (unlikely((src) == NULL) ||              \
> > >
> > > WARNING: line over 80 characters
> > > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
> > > +               unlikely(copy_from_user(dest, src, (elements) *
> > sizeof(*(dest))))) { \
> > >
> > > WARNING: suspect code indent for conditional statements (8, 12)
> > > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
> > > +       if (DSP_SUCCEEDED(status)) {                    \
> > > +           if (unlikely((dest) == NULL) ||
> > \
> > >
> > > WARNING: line over 80 characters
> > > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
> > > +               unlikely(copy_to_user(dest, src, (elements) *
> > sizeof(*(src))))) { \
> > >
> > > total: 0 errors, 4 warnings, 48 lines checked
> > >
> > > Could you please fix that warning please?
> > 
> > I suspect the only way to fix those warnings would either introduce
> > other warnings, or \
> Gentle query: Have we already tried this or is it just a suspicion?

Being an Englishman, I use various forms of irony more than others do;
that particular form's called "litotes". It indeed is not a suspicion -
I tried about 4 or 5 variations, none of which were both readable and
warning-free. In the end I decided that whatever was closest to the
original would be safest.

> > 							would \
> > 							lead \
> > 							to \
> > 							utterly \
> > 							unread- \
> > 							able \
> > 							code.
> > 
> > If you check how and why the original TI-originated version of the code
> > does not follow the linux coding standards, the difficulties we would
> > have making a warning-free patch of it should be apparent.
> 
> Past is past. The idea was not to introduce anymore warning code.

The TI code would trigger the following 4 warnings:
WARNING: suspect code indent for conditional statements (4, 12)
WARNING: line over 80 characters
WARNING: suspect code indent for conditional statements (4, 12)
WARNING: line over 80 characters
4 warnings isn't more than 4 warnings, it's no change - and as I mention
above, in the absence of an easy fix, that was deliberate.

Phil

Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Hiroshi DOYU]

From: "Carmody Phil.2 (EXT-Ixonos/Helsinki)" <ext-phil.2.carmody@nokia.com>
Subject: RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
Date: Tue, 14 Jul 2009 13:20:30 +0200

> On Tue, 2009-07-14 at 13:05 +0200, ext Menon, Nishanth wrote:
> > Phil,
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > > owner@vger.kernel.org] On Behalf Of Phil Carmody
> > > Sent: Tuesday, July 14, 2009 6:03 AM
> > > 
> > > On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote:
> > > > Thanks for the patch, it only has indentation problems, this is the
> > > checkpatch output:
> > > >
> > > > WARNING: suspect code indent for conditional statements (8, 12)
> > > > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
> > > > +       if (DSP_SUCCEEDED(status)) {                    \
> > > > +           if (unlikely((src) == NULL) ||              \
> > > >
> > > > WARNING: line over 80 characters
> > > > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
> > > > +               unlikely(copy_from_user(dest, src, (elements) *
> > > sizeof(*(dest))))) { \
> > > >
> > > > WARNING: suspect code indent for conditional statements (8, 12)
> > > > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
> > > > +       if (DSP_SUCCEEDED(status)) {                    \
> > > > +           if (unlikely((dest) == NULL) ||
> > > \
> > > >
> > > > WARNING: line over 80 characters
> > > > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
> > > > +               unlikely(copy_to_user(dest, src, (elements) *
> > > sizeof(*(src))))) { \
> > > >
> > > > total: 0 errors, 4 warnings, 48 lines checked
> > > >
> > > > Could you please fix that warning please?
> > > 
> > > I suspect the only way to fix those warnings would either introduce
> > > other warnings, or \
> > Gentle query: Have we already tried this or is it just a suspicion?
> 
> Being an Englishman, I use various forms of irony more than others do;
> that particular form's called "litotes". It indeed is not a suspicion -
> I tried about 4 or 5 variations, none of which were both readable and
> warning-free. In the end I decided that whatever was closest to the
> original would be safest.
> 
> > > 							would \
> > > 							lead \
> > > 							to \
> > > 							utterly \
> > > 							unread- \
> > > 							able \
> > > 							code.
> > > 
> > > If you check how and why the original TI-originated version of the code
> > > does not follow the linux coding standards, the difficulties we would
> > > have making a warning-free patch of it should be apparent.
> > 
> > Past is past. The idea was not to introduce anymore warning code.
> 
> The TI code would trigger the following 4 warnings:
> WARNING: suspect code indent for conditional statements (4, 12)
> WARNING: line over 80 characters
> WARNING: suspect code indent for conditional statements (4, 12)
> WARNING: line over 80 characters
> 4 warnings isn't more than 4 warnings, it's no change - and as I mention
> above, in the absence of an easy fix, that was deliberate.

I am not sure if these copy_from_user() wrapping are practically
useful or not. It may be enough just to use kernel API as is. But if
there are some reason to requires strict parameter checkings or
debugging feature support for these family here, introducing inline
functions for them, instead of macro, may be another option?

For example,

	Modified drivers/dsp/bridge/pmgr/wcd.c
diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 86812c6..c8b26c0 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -146,16 +146,23 @@
 #define MAX_BUFS	64
 
 /* Following two macros should ideally have do{}while(0) */
+static inline void cp_fm_usr(void *to, const void __user *from,
+			     DSP_STATUS *err, unsigned long n)
+{
+	if (DSP_FAILED(err))
+		return;
 
-#define cp_fm_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(src == NULL) ||				\
-		unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_from_user failed, src=0x%x\n", src);  \
-		status = DSP_EPOINTER ; \
-	} \
-    }
+	if (unlikely(!from)) {
+		*err = DSP_EPOINTER ;
+		return;
+	}
+
+	if (unlikely(copy_from_user(to, from, n * sizeof(*(to))))) {
+		GT_1trace(WCD_debugMask, GT_7CLASS,
+			  "%s failed, from=0x%x\n", src, __func__);
+		*err = DSP_EPOINTER ;
+	}
+}
 
 #define cp_to_usr(dest, src, status, elements)    \
     if (DSP_SUCCEEDED(status)) {\
@@ -525,7 +532,7 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args)
 	char *pszPathName = NULL;
 	DSP_STATUS status = DSP_SOK;
 
-	cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, status, 1);
+	cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, &status, 1);
 	if (DSP_FAILED(status))
 		goto func_end;
 	pathSize = strlen_user((char *)
....

> 
> Phil
> 
> --
> 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

Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Phil Carmody]

On Tue, 2009-07-14 at 14:30 +0200, Doyu Hiroshi (Nokia-D/Helsinki)
wrote:
> From: "Carmody Phil.2 (EXT-Ixonos/Helsinki)" <ext-phil.2.carmody@nokia.com>
> Subject: RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
> Date: Tue, 14 Jul 2009 13:20:30 +0200
> 
> > On Tue, 2009-07-14 at 13:05 +0200, ext Menon, Nishanth wrote:
> > > Phil,
> > > > -----Original Message-----
> > > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > > > owner@vger.kernel.org] On Behalf Of Phil Carmody
> > > > Sent: Tuesday, July 14, 2009 6:03 AM
> > > > 
> > > > On Fri, 2009-07-10 at 01:51 +0200, ext Guzman Lugo, Fernando wrote:
> > > > > Thanks for the patch, it only has indentation problems, this is the
> > > > checkpatch output:
> > > > >
> > > > > WARNING: suspect code indent for conditional statements (8, 12)
> > > > > #34: FILE: drivers/dsp/bridge/pmgr/wcd.c:152:
> > > > > +       if (DSP_SUCCEEDED(status)) {                    \
> > > > > +           if (unlikely((src) == NULL) ||              \
> > > > >
> > > > > WARNING: line over 80 characters
> > > > > #36: FILE: drivers/dsp/bridge/pmgr/wcd.c:154:
> > > > > +               unlikely(copy_from_user(dest, src, (elements) *
> > > > sizeof(*(dest))))) { \
> > > > >
> > > > > WARNING: suspect code indent for conditional statements (8, 12)
> > > > > #46: FILE: drivers/dsp/bridge/pmgr/wcd.c:164:
> > > > > +       if (DSP_SUCCEEDED(status)) {                    \
> > > > > +           if (unlikely((dest) == NULL) ||
> > > > \
> > > > >
> > > > > WARNING: line over 80 characters
> > > > > #48: FILE: drivers/dsp/bridge/pmgr/wcd.c:166:
> > > > > +               unlikely(copy_to_user(dest, src, (elements) *
> > > > sizeof(*(src))))) { \
> > > > >
> > > > > total: 0 errors, 4 warnings, 48 lines checked
> > > > >
> > > > > Could you please fix that warning please?
> > > > 
> > > > I suspect the only way to fix those warnings would either introduce
> > > > other warnings, or \
> > > Gentle query: Have we already tried this or is it just a suspicion?
> > 
> > Being an Englishman, I use various forms of irony more than others do;
> > that particular form's called "litotes". It indeed is not a suspicion -
> > I tried about 4 or 5 variations, none of which were both readable and
> > warning-free. In the end I decided that whatever was closest to the
> > original would be safest.
> > 
> > > > 							would \
> > > > 							lead \
> > > > 							to \
> > > > 							utterly \
> > > > 							unread- \
> > > > 							able \
> > > > 							code.
> > > > 
> > > > If you check how and why the original TI-originated version of the code
> > > > does not follow the linux coding standards, the difficulties we would
> > > > have making a warning-free patch of it should be apparent.
> > > 
> > > Past is past. The idea was not to introduce anymore warning code.
> > 
> > The TI code would trigger the following 4 warnings:
> > WARNING: suspect code indent for conditional statements (4, 12)
> > WARNING: line over 80 characters
> > WARNING: suspect code indent for conditional statements (4, 12)
> > WARNING: line over 80 characters
> > 4 warnings isn't more than 4 warnings, it's no change - and as I mention
> > above, in the absence of an easy fix, that was deliberate.
> 
> I am not sure if these copy_from_user() wrapping are practically
> useful or not. It may be enough just to use kernel API as is. But if
> there are some reason to requires strict parameter checkings or
> debugging feature support for these family here, introducing inline
> functions for them, instead of macro, may be another option?

I did mention that possibility to Ameya. In the 21st century, I think we
should be able to trust the compiler to generate identical code from
inline functions as functional macros. (Are we allowed to use 'restrict'
pointers?)

> For example,
> 
> 	Modified drivers/dsp/bridge/pmgr/wcd.c
> diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
> index 86812c6..c8b26c0 100644
> --- a/drivers/dsp/bridge/pmgr/wcd.c
> +++ b/drivers/dsp/bridge/pmgr/wcd.c
> @@ -146,16 +146,23 @@
>  #define MAX_BUFS	64
>  
>  /* Following two macros should ideally have do{}while(0) */
> +static inline void cp_fm_usr(void *to, const void __user *from,
> +			     DSP_STATUS *err, unsigned long n)
> +{
> +	if (DSP_FAILED(err))

*err

> +		return;
>  
> -#define cp_fm_usr(dest, src, status, elements)    \
> -    if (DSP_SUCCEEDED(status)) {\
> -	    if (unlikely(src == NULL) ||				\
> -		unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \
> -		GT_1trace(WCD_debugMask, GT_7CLASS, \
> -		"copy_from_user failed, src=0x%x\n", src);  \
> -		status = DSP_EPOINTER ; \
> -	} \
> -    }
> +	if (unlikely(!from)) {
> +		*err = DSP_EPOINTER ;
> +		return;
> +	}
> +
> +	if (unlikely(copy_from_user(to, from, n * sizeof(*(to))))) {

additional () not needed any more.

> +		GT_1trace(WCD_debugMask, GT_7CLASS,
> +			  "%s failed, from=0x%x\n", src, __func__);
> +		*err = DSP_EPOINTER ;
> +	}
> +}
>  
>  #define cp_to_usr(dest, src, status, elements)    \
>      if (DSP_SUCCEEDED(status)) {\
> @@ -525,7 +532,7 @@ u32 MGRWRAP_RegisterObject(union Trapped_Args *args)
>  	char *pszPathName = NULL;
>  	DSP_STATUS status = DSP_SOK;
>  
> -	cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, status, 1);
> +	cp_fm_usr(&pUuid, args->ARGS_MGR_REGISTEROBJECT.pUuid, &status, 1);
>  	if (DSP_FAILED(status))
>  		goto func_end;
>  	pathSize = strlen_user((char *)
> ....

That kind of thing would work. There would be loads of changes (62, by
the looks of it), but at least they are trivial ones.

Phil

Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else

[Hiroshi DOYU]

[-- Attachment #1: Type: Text/Plain, Size: 825 bytes --]

From: "Carmody Phil.2 (EXT-Ixonos/Helsinki)" <ext-phil.2.carmody@nokia.com>
Subject: Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
Date: Tue, 14 Jul 2009 15:17:25 +0200

[...]
> > I am not sure if these copy_from_user() wrapping are practically
> > useful or not. It may be enough just to use kernel API as is. But if
> > there are some reason to requires strict parameter checkings or
> > debugging feature support for these family here, introducing inline
> > functions for them, instead of macro, may be another option?
> 
> I did mention that possibility to Ameya. In the 21st century, I think we
> should be able to trust the compiler to generate identical code from
> inline functions as functional macros. (Are we allowed to use 'restrict'
> pointers?)

I tried to create the attached patch.

[-- Attachment #2: 0001-DSPBRIDGE-fix-macros-that-break-when-inside-an-if-e.patch --]
[-- Type: Application/Octet-Stream, Size: 2534 bytes --]

From a4a9380f25e8d9447c02b81440b59d87628eec16 Mon Sep 17 00:00:00 2001
From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
Date: Tue, 14 Jul 2009 22:27:44 +0300
Subject: [PATCH 1/1] DSPBRIDGE: fix macros that break when inside an if/else

Based on the following discussion:
      http://marc.info/?l=linux-omap&m=124697893724881&w=2

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 drivers/dsp/bridge/pmgr/wcd.c |   60 ++++++++++++++++++++++++++--------------
 1 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c
index 86812c6..430960b 100644
--- a/drivers/dsp/bridge/pmgr/wcd.c
+++ b/drivers/dsp/bridge/pmgr/wcd.c
@@ -145,27 +145,45 @@
 #define MAX_STREAMS     16
 #define MAX_BUFS	64
 
-/* Following two macros should ideally have do{}while(0) */
-
-#define cp_fm_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(src == NULL) ||				\
-		unlikely(copy_from_user(dest, src, elements * sizeof(*(dest))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_from_user failed, src=0x%x\n", src);  \
-		status = DSP_EPOINTER ; \
-	} \
-    }
-
-#define cp_to_usr(dest, src, status, elements)    \
-    if (DSP_SUCCEEDED(status)) {\
-	    if (unlikely(dest == NULL) ||				\
-		unlikely(copy_to_user(dest, src, elements * sizeof(*(src))))) { \
-		GT_1trace(WCD_debugMask, GT_7CLASS, \
-		"copy_to_user failed, dest=0x%x\n", dest); \
-		status = DSP_EPOINTER ;\
-	} \
-    }
+static inline void __cp_fm_usr(void *to, const void __user *from,
+			       DSP_STATUS *err, unsigned long bytes)
+{
+	if (DSP_FAILED(*err))
+		return;
+
+	if (unlikely(!from)) {
+		*err = DSP_EPOINTER;
+		return;
+	}
+
+	if (unlikely(copy_from_user(to, from, bytes))) {
+		GT_2trace(WCD_debugMask, GT_7CLASS,
+			  "%s failed, from=0x%08x\n", __func__, from);
+		*err = DSP_EPOINTER;
+	}
+}
+#define cp_fm_usr(to, from, err, n)				\
+	__cp_fm_usr(to, from, &(err), (n) * sizeof(*(to)))
+
+static inline void __cp_to_usr(void __user *to, const void *from,
+			       DSP_STATUS *err, unsigned long bytes)
+{
+	if (DSP_FAILED(*err))
+		return;
+
+	if (unlikely(!to)) {
+		*err = DSP_EPOINTER;
+		return;
+	}
+
+	if (unlikely(copy_to_user(to, from, bytes))) {
+		GT_2trace(WCD_debugMask, GT_7CLASS,
+			  "%s failed, to=0x%08x\n", __func__, to);
+		*err = DSP_EPOINTER;
+	}
+}
+#define cp_to_usr(to, from, err, n)				\
+	__cp_to_usr(to, from, &(err), (n) * sizeof(*(from)))
 
 /* Device IOCtl function pointer */
 struct WCD_Cmd {
-- 
1.6.0.4