RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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
ext Menon, Nishanth wrote:
Phil,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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
On Tue, 2009-07-14 at 13:05 +0200, ext Menon, Nishanth wrote:
Phil,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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
From: Carmody Phil.2 (EXT-Ixonos/Helsinki) ext-phil.2.carm...@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-ow...@vger.kernel.org [mailto:linux-omap-
ow...@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
Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
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.carm...@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-ow...@vger.kernel.org [mailto:linux-omap-
ow...@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
Re: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
From: Carmody Phil.2 (EXT-Ixonos/Helsinki) ext-phil.2.carm...@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. 0001-DSPBRIDGE-fix-macros-that-break-when-inside-an-if-e.patch Description: Binary data
RE: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an if/else
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.pala...@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.carm...@nokia.com
Subject: [PATCH 1/4] DSPBRIDGE: Fix macros that break when inside an
if/else
From: Phil Carmody ext-phil.2.carm...@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.carm...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
