[PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions

[PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions

[Niels de Vos]

When DBG() is used in a simple if-else, the resulting code path
currently depends on the definition of DBG(). Inserting the statement in
a "do { ... } while (0)" prevents this possible misuse.

Signed-off-by: Niels de Vos <ndevos@redhat.com>

---
Note, I have not found any offenders, but a mistake can easily be made.
The following example shows what can go wrong if little intention is
paid to the definition of the DBG() macro.

Example:
	if something_went_wrong()
		DBG("oh no, something went wrong!\n");
	else
		printk("all went fine\n");

Old result where the else is placed inside the first if-statment:
	if something_went_wrong() {
		if (omapfb_debug) {
			printk(KERN_DEBUG "oh no, something went wrong!\n");
		} else {
			printk("all went fine\n");
		}
	}

New result where the else is an alternative to the first if-statement:
	if something_went_wrong() {
		do {
			if (omapfb_debug)
				printk(KERN_DEBUG "oh no, something went wrong!\n");
		} while (0);
	} else {
		printk("all went fine\n");
	}
---
 drivers/video/omap2/omapfb/omapfb.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 1305fc9..a01b0bb 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -34,8 +34,10 @@
 #ifdef DEBUG
 extern unsigned int omapfb_debug;
 #define DBG(format, ...) \
-	if (omapfb_debug) \
-		printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
+	do { \
+		if (omapfb_debug) \
+			printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
+	while (0)
 #else
 #define DBG(format, ...)
 #endif
-- 
1.7.4.4

Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions

[Geert Uytterhoeven]

On Tue, May 10, 2011 at 11:20, Niels de Vos <ndevos@redhat.com> wrote:
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the statement in
> a "do { ... } while (0)" prevents this possible misuse.
>
> Signed-off-by: Niels de Vos <ndevos@redhat.com>

> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h
> @@ -34,8 +34,10 @@
>  #ifdef DEBUG
>  extern unsigned int omapfb_debug;
>  #define DBG(format, ...) \
> -       if (omapfb_debug) \
> -               printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
> +       do { \
> +               if (omapfb_debug) \
> +                       printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
> +       while (0)

Where's the closing '}'?

>  #else
>  #define DBG(format, ...)

BTW, no printf()-style format checking here.

>  #endif

What about using the standard pr_debug()/dev_dbg() instead?
With dynamic debug, it can be enabled at run time.
As a bonus, you get printf()-style format checking if debugging is disabled.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] omap2/omapfb: make DBG() more resistant in if-else

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Niels de Vos
> Sent: Tuesday, May 10, 2011 2:51 PM
> To: linux-omap@vger.kernel.org
> Cc: linux-fbdev@vger.kernel.org; 
> linux-kernel@vger.kernel.org; Niels de Vos
> Subject: [PATCH] omap2/omapfb: make DBG() more resistant in 
> if-else constructions
> 
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the 
> statement in
> a "do { ... } while (0)" prevents this possible misuse.
> 
> Signed-off-by: Niels de Vos <ndevos@redhat.com>
> 
> ---
> Note, I have not found any offenders, but a mistake can 
> easily be made.
> The following example shows what can go wrong if little intention is
> paid to the definition of the DBG() macro.
> 
> Example:
> 	if something_went_wrong()
> 		DBG("oh no, something went wrong!\n");
> 	else
> 		printk("all went fine\n");
> 
> Old result where the else is placed inside the first if-statment:
> 	if something_went_wrong() {
> 		if (omapfb_debug) {
> 			printk(KERN_DEBUG "oh no, something 
> went wrong!\n");
> 		} else {
> 			printk("all went fine\n");
> 		}
> 	}
> 
> New result where the else is an alternative to the first if-statement:
> 	if something_went_wrong() {
> 		do {
> 			if (omapfb_debug)
> 				printk(KERN_DEBUG "oh no, 
> something went wrong!\n");
> 		} while (0);
> 	} else {
> 		printk("all went fine\n");
> 	}
> ---
>  drivers/video/omap2/omapfb/omapfb.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/omap2/omapfb/omapfb.h 
> b/drivers/video/omap2/omapfb/omapfb.h
> index 1305fc9..a01b0bb 100644
> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h
> @@ -34,8 +34,10 @@
>  #ifdef DEBUG
>  extern unsigned int omapfb_debug;
>  #define DBG(format, ...) \
> -	if (omapfb_debug) \
> -		printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
> +	do { \
> +		if (omapfb_debug) \
> +			printk(KERN_DEBUG "OMAPFB: " format, ## 
> __VA_ARGS__); \
> +	while (0)

A real good find. Wondering if it really didn't create any problems so far...

~sanjeev

>  #else
>  #define DBG(format, ...)
>  #endif
> -- 
> 1.7.4.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
> 

Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions

[Niels de Vos]

On Tue, May 10, 2011 at 10:42 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, May 10, 2011 at 11:20, Niels de Vos <ndevos@redhat.com> wrote:
>> When DBG() is used in a simple if-else, the resulting code path
>> currently depends on the definition of DBG(). Inserting the statement in
>> a "do { ... } while (0)" prevents this possible misuse.
>>
>> Signed-off-by: Niels de Vos <ndevos@redhat.com>
>
>> --- a/drivers/video/omap2/omapfb/omapfb.h
>> +++ b/drivers/video/omap2/omapfb/omapfb.h
>> @@ -34,8 +34,10 @@
>>  #ifdef DEBUG
>>  extern unsigned int omapfb_debug;
>>  #define DBG(format, ...) \
>> -       if (omapfb_debug) \
>> -               printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
>> +       do { \
>> +               if (omapfb_debug) \
>> +                       printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
>> +       while (0)
>
> Where's the closing '}'?

Good catch! That's in a "fixup!" that I forgot to squash :-/
I'll post an update version in a bit.


>>  #else
>>  #define DBG(format, ...)
>
> BTW, no printf()-style format checking here.
>
>>  #endif
>
> What about using the standard pr_debug()/dev_dbg() instead?
> With dynamic debug, it can be enabled at run time.
> As a bonus, you get printf()-style format checking if debugging is disabled.

I think removing DBG() and the omapfb_debug module-parameter is surely
a good thing. Unfortunately DBG() is used quite a bit in the code and
replacing them 'll take some more time. I don't know yet when I find
some time to do and test that.

Thanks for the pointers,
Niels


> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>

[PATCH V2] omap2/omapfb: make DBG() more resistant in if-else constructions

[Niels de Vos]

When DBG() is used in a simple if-else, the resulting code path
currently depends on the definition of DBG(). Inserting the statement in
a "do { ... } while (0)" prevents this possible misuse.

Signed-off-by: Niels de Vos <ndevos@redhat.com>

---
V2: add the missing closing }

Note, I have not found any offenders, but a mistake can easily be made.
The following example shows what can go wrong if little intention is
paid to the definition of the DBG() macro.

Example:
	if something_went_wrong()
		DBG("oh no, something went wrong!\n");
	else
		printk("all went fine\n");

Old result where the else is placed inside the first if-statment:
	if something_went_wrong() {
		if (omapfb_debug) {
			printk(KERN_DEBUG "oh no, something went wrong!\n");
		} else {
			printk("all went fine\n");
		}
	}

New result where the else is an alternative to the first if-statement:
	if something_went_wrong() {
		do {
			if (omapfb_debug)
				printk(KERN_DEBUG "oh no, something went wrong!\n");
		} while (0);
	} else {
		printk("all went fine\n");
	}
---
 drivers/video/omap2/omapfb/omapfb.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 1305fc9..456c586 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -34,8 +34,10 @@
 #ifdef DEBUG
 extern unsigned int omapfb_debug;
 #define DBG(format, ...) \
-	if (omapfb_debug) \
-		printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__)
+	do { \
+		if (omapfb_debug) \
+			printk(KERN_DEBUG "OMAPFB: " format, ## __VA_ARGS__); \
+	} while (0)
 #else
 #define DBG(format, ...)
 #endif
-- 
1.7.4.4

Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else

[Tomi Valkeinen]

On Tue, 2011-05-10 at 11:42 +0200, Geert Uytterhoeven wrote:

> What about using the standard pr_debug()/dev_dbg() instead?
> With dynamic debug, it can be enabled at run time.
> As a bonus, you get printf()-style format checking if debugging is disabled.

Yes, dev_dbg & co. would be better.

However, one thing I dislike about them is the extra stuff they print.
For example, for omapfb and omapdss dev_dbg will print:

omapfb omapfb: foo
omapdss_dss omapdss_dss: foo

I originally added the debug macros to omapdss to be able to
automatically print the DSS module name, as at that point there was only
one big omapdss device. And I guess I just followed with similar macro
in omapfb also. But I believe both omapdss and omapfb should be changed
to dev_* prints sometime soon.

 Tomi

Re: [PATCH] omap2/omapfb: make DBG() more resistant in if-else constructions

[Geert Uytterhoeven]

On Tue, May 10, 2011 at 14:08, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2011-05-10 at 11:42 +0200, Geert Uytterhoeven wrote:
>> What about using the standard pr_debug()/dev_dbg() instead?
>> With dynamic debug, it can be enabled at run time.
>> As a bonus, you get printf()-style format checking if debugging is disabled.
>
> Yes, dev_dbg & co. would be better.
>
> However, one thing I dislike about them is the extra stuff they print.
> For example, for omapfb and omapdss dev_dbg will print:
>
> omapfb omapfb: foo
> omapdss_dss omapdss_dss: foo
>
> I originally added the debug macros to omapdss to be able to
> automatically print the DSS module name, as at that point there was only
> one big omapdss device. And I guess I just followed with similar macro
> in omapfb also. But I believe both omapdss and omapfb should be changed
> to dev_* prints sometime soon.

If you don't want the extra baggage, do

        #define pr_fmt(fmt)     KBUILD_MODNAME ": " fmt

and use pr_debug().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V2] omap2/omapfb: make DBG() more resistant in if-else

[Tomi Valkeinen]

On Tue, 2011-05-10 at 12:07 +0100, Niels de Vos wrote:
> When DBG() is used in a simple if-else, the resulting code path
> currently depends on the definition of DBG(). Inserting the statement in
> a "do { ... } while (0)" prevents this possible misuse.
> 
> Signed-off-by: Niels de Vos <ndevos@redhat.com>

Thanks, I'll add this to the dss2 tree.

 Tomi