wrong print debugging message code

wrong print debugging message code

[Jin-Young Park]

Hello,

I written ASoC Audio driver for our company chip in linux kernel 2.6.24.1.
New kernel 2.6.25.7 released before several days.
So, I merge my ASoC Audio dirver to kernel 2.6.25.7.
then I found wrong code that print debuuging message code in several files.
These file location is kernel_src/sound/soc/codecs for SoC audio codec
drvier files

These files include follow define code for print debug message.

------------------------------------------------------------------------------------------------------------------------
#define XXXX_DEBUG 0

#ifdef XXXX_DEBUG
#define dbg(format, arg...) \
        printk(KERN_DEBUG AUDIO_NAME ": " format "\n" , ## arg)
#else
#define dbg(format, arg...) do {} while (0)
#endif
------------------------------------------------------------------------------------------------------------------------

I think "#ifdef XXXX_DEBUG" line is wrong. Because, XXXX_DEBUG was already
defined 0.
I known that "#ifdef" is only check defined or not defined, 0 or 1(false or
true) are not check.
"#if XXXX_DEBUG" code is more correct than "#ifdef XXXX_DEBUG".

thanks

Re: wrong print debugging message code

[Takashi Iwai]

At Sat, 21 Jun 2008 14:44:47 +0900,
Jin-Young Park wrote:
> 
> Hello,
> 
> I written ASoC Audio driver for our company chip in linux kernel 2.6.24.1.
> New kernel 2.6.25.7 released before several days.
> So, I merge my ASoC Audio dirver to kernel 2.6.25.7.
> then I found wrong code that print debuuging message code in several files.
> These file location is kernel_src/sound/soc/codecs for SoC audio codec
> drvier files
> 
> These files include follow define code for print debug message.
> 
> ------------------------------------------------------------------------------------------------------------------------
> #define XXXX_DEBUG 0
> 
> #ifdef XXXX_DEBUG
> #define dbg(format, arg...) \
>         printk(KERN_DEBUG AUDIO_NAME ": " format "\n" , ## arg)
> #else
> #define dbg(format, arg...) do {} while (0)
> #endif
> ------------------------------------------------------------------------------------------------------------------------
> 
> I think "#ifdef XXXX_DEBUG" line is wrong. Because, XXXX_DEBUG was already
> defined 0.
> I known that "#ifdef" is only check defined or not defined, 0 or 1(false or
> true) are not check.
> "#if XXXX_DEBUG" code is more correct than "#ifdef XXXX_DEBUG".

#ifdef XXX_DEBUG is more common than #if XXX_DEBUG in the kernel
tree.  So, I'd suggest to keep #ifdef style.

The confusing thing is that it defines to '0' (and I'm not sure the
debug is activated as default in the current code -- Mark, Liam, is it
intentional?).

IMO, the debug should be basically off as default, and if enabled, it
should be defined to '1'.


Takashi

Re: wrong print debugging message code

[Mark Brown]

On Mon, Jun 23, 2008 at 12:55:57PM +0200, Takashi Iwai wrote:

> The confusing thing is that it defines to '0' (and I'm not sure the
> debug is activated as default in the current code -- Mark, Liam, is it
> intentional?).

I doubt it.

> IMO, the debug should be basically off as default, and if enabled, it
> should be defined to '1'.

Really they should all just be deleted and replaced by the standard
pr_() macros but if we wait for v2 then we'd be able to use the dev_()
variants instead.

Re: wrong print debugging message code

[Takashi Iwai]

At Mon, 23 Jun 2008 12:12:28 +0100,
Mark Brown wrote:
> 
> On Mon, Jun 23, 2008 at 12:55:57PM +0200, Takashi Iwai wrote:
> 
> > The confusing thing is that it defines to '0' (and I'm not sure the
> > debug is activated as default in the current code -- Mark, Liam, is it
> > intentional?).
> 
> I doubt it.
> 
> > IMO, the debug should be basically off as default, and if enabled, it
> > should be defined to '1'.
> 
> Really they should all just be deleted and replaced by the standard
> pr_() macros but if we wait for v2 then we'd be able to use the dev_()
> variants instead.

It depends on your plan.  Definitely nice to fix it for 2.6.27,
though.


Takashi

Re: wrong print debugging message code

[Mark Brown]

On Mon, Jun 23, 2008 at 02:13:19PM +0200, Takashi Iwai wrote:

> It depends on your plan.  Definitely nice to fix it for 2.6.27,
> though.

I've actually just written a patch ripping them all out (I'd probably
have submitted it already if I hadn't been reviewing Ben's changes) -
turns out that none of the drivers were actually using their dbg()
macros anyway so it won't have any runtime effect, though obviously a
fix would still be good.

Re: wrong print debugging message code

[Jin-Young Park]

Hello.

I tested the code that "#define XXXX_DEBUG 0" on our company EVB with
WM8960.
I defined "#define XXXX_DEBUG 0" and dbg() put into mute
function(wm8960_digital_mute()).
After linux boot, played audio file.
My test dbg() message was not print terminal, but I find dbg() code was
working exactly.
Excuted dmesg command, then it was print that logged dbg() messages.
I think, it maybe potential that reduce performance before one knows.

Thanks.
Jinyoung Park.

Re: wrong print debugging message code

[Mark Brown]

On Tue, Jun 24, 2008 at 03:18:49AM +0900, Jin-Young Park wrote:

> I defined "#define XXXX_DEBUG 0" and dbg() put into mute
> function(wm8960_digital_mute()).

The WM8960 driver isn't in the standard Linux kernel yet, it is currently
distributed via the Wolfson web site.  None of the drivers included in
the standard Linux kernel were using their dbg() macros, though some had
such macros present.

> I think, it maybe potential that reduce performance before one knows.

The performance impact should be very minor - the digital mute is not
used in any hot paths and the output will only go do the dmesg buffer by
default so won't have the overhead of writing to the console.

Out of interest, which platform are you working with?

Re: wrong print debugging message code

[Jin-Young Park]

I get the point your explanation. Thanks.

> Out of interest, which platform are you working with?


I working at Mtekvision in Korea.
Mtekvision is fabless company, development MMP and ISP.
you know? Anyway, my development platform is
MV86XX series(MMP) with Wolfson Codecs(WM8960, 8973, 8983, 9713 ...)
and Linux kernel 2.6.25.7 <tr_1214286468096>.