Re: [PATCH 06/14] [media] cx18: Use current logging styles

From: Andy Walls <awalls@md.metrocast.net>
To: Joe Perches <joe@perches.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>,
	ivtv-devel@ivtvdriver.org, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/14] [media] cx18: Use current logging styles
Date: Sat, 27 Aug 2011 09:28:59 -0400	[thread overview]
Message-ID: <1314451740.2244.7.camel@palomino.walls.org> (raw)
In-Reply-To: <1314222175.15882.8.camel@Joe-Laptop>

On Wed, 2011-08-24 at 14:42 -0700, Joe Perches wrote:
> On Wed, 2011-08-24 at 06:34 -0400, Andy Walls wrote:
> > On Sun, 2011-08-21 at 15:56 -0700, Joe Perches wrote:
> > > Add pr_fmt.
> > > Convert printks to pr_<level>.
> > > Convert printks without KERN_<level> to appropriate pr_<level>.
> > > Removed embedded prefixes when pr_fmt was added.
> > > Use ##__VA_ARGS__ for variadic macros.
> > > Coalesce format strings.
> > 1. It is important to preserve the per-card prefixes emitted by the
> > driver: cx18-0, cx18-1, cx18-2, etc.  With a quick skim, I think your
> > change preserves the format of all output messages (except removing
> > periods).  Can you confirm this?
> 
> Here's the output diff of
> strings built-in.o | grep "^<.>" | sort
> new and old
> $ diff -u0 cx18.old cx18.new
> --- cx18.old	2011-08-24 13:18:41.000000000 -0700
> +++ cx18.new	2011-08-24 14:04:10.000000000 -0700
> @@ -1,2 +1,9 @@
> -<3>cx18-alsa cx is NULL
> -<3>cx18-alsa: %s: struct v4l2_device * is NULL
> +<3>cx18_alsa: cx is NULL
> +<3>cx18_alsa: %s-alsa: %s: failed to create struct snd_cx18_card
> +<3>cx18_alsa: %s-alsa: %s: snd_card_create() failed with err %d
> +<3>cx18_alsa: %s-alsa: %s: snd_card_register() failed with err %d
> +<3>cx18_alsa: %s-alsa: %s: snd_cx18_card_create() failed with err %d
> +<3>cx18_alsa: %s-alsa: %s: snd_cx18_pcm_create() failed with err %d
> +<3>cx18_alsa: %s-alsa: %s: snd_cx18_pcm_create() failed with err %d
> +<3>cx18_alsa: %s-alsa: %s: struct snd_cx18_card * already exists
> +<3>cx18_alsa: %s: struct v4l2_device * is NULL
> @@ -17,7 +23,0 @@
> -<3>%s-alsa: %s: failed to create struct snd_cx18_card
> -<3>%s-alsa: %s: snd_card_create() failed with err %d
> -<3>%s-alsa: %s: snd_card_register() failed with err %d
> -<3>%s-alsa: %s: snd_cx18_card_create() failed with err %d
> -<3>%s-alsa: %s: snd_cx18_pcm_create() failed with err %d
> -<3>%s-alsa: %s: snd_cx18_pcm_create() failed with err %d
> -<3>%s-alsa: %s: struct snd_cx18_card * already exists

Yuck.

> @@ -62 +62 @@
> -<3>%s: Prefix your subject line with [UNKNOWN CX18 CARD].
> +<3>%s: Prefix your subject line with [UNKNOWN CX18 CARD]

> @@ -80 +80 @@
> -<4>%s-alsa: %s: struct snd_cx18_card * is NULL
> +<4>cx18_alsa: %s-alsa: %s: struct snd_cx18_card * is NULL

Yuck.

> @@ -82 +82 @@
> -<4>%s: Could not register GPIO reset controllersubdevice; proceeding anyway.
> +<4>%s: Could not register GPIO reset controller subdevice; proceeding anyway.
> @@ -85 +85 @@
> -<4>%s: MPEG Index stream cannot be claimed directly, but something tried.
> +<4>%s: MPEG Index stream cannot be claimed directly, but something tried
> @@ -99,12 +99,14 @@
> -<6>cx18-alsa: module loading...
> -<6>cx18-alsa: module unload complete
> -<6>cx18-alsa: module unloading...
> -<6>cx18-alsa-pcm %s: Allocating vbuffer
> -<6>cx18-alsa-pcm %s: cx18 alsa announce ptr=%p data=%p num_bytes=%zd
> -<6>cx18-alsa-pcm %s: dma area was NULL - ignoring
> -<6>cx18-alsa-pcm %s: freeing pcm capture region
> -<6>cx18-alsa-pcm %s: runtime was NULL
> -<6>cx18-alsa-pcm %s: %s called
> -<6>cx18-alsa-pcm %s: %s: length was zero
> -<6>cx18-alsa-pcm %s: stride is zero
> -<6>cx18-alsa-pcm %s: substream was NULL
> +<6>cx18_alsa: module loading...
> +<6>cx18_alsa: module unload complete
> +<6>cx18_alsa: module unloading...
> +<6>cx18_alsa: %s: Allocating vbuffer
> +<6>cx18_alsa: %s: created cx18 ALSA interface instance 
> +<6>cx18_alsa: %s: cx18 alsa announce ptr=%p data=%p num_bytes=%zd
> +<6>cx18_alsa: %s: dma area was NULL - ignoring
> +<6>cx18_alsa: %s: freeing pcm capture region
> +<6>cx18_alsa: %s: PCM stream for card is disabled - skipping
> +<6>cx18_alsa: %s: runtime was NULL
> +<6>cx18_alsa: %s: %s called
> +<6>cx18_alsa: %s: %s: length was zero
> +<6>cx18_alsa: %s: stride is zero
> +<6>cx18_alsa: %s: substream was NULL
> @@ -172 +174 @@
> -<6>%s:  info: dualwatch: change stereo flag from 0x%x to 0x%x.
> +<6>%s:  info: dualwatch: change stereo flag from 0x%x to 0x%x
> @@ -188 +190 @@
> -<6>%s:  info: Preparing for firmware halt.
> +<6>%s:  info: Preparing for firmware halt
> @@ -206 +208 @@
> -<6>%s:  info: Switching standard to %llx.
> +<6>%s:  info: Switching standard to %llx
> @@ -236 +237,0 @@
> -<6>%s: %s: created cx18 ALSA interface instance 
> @@ -239 +239,0 @@
> -<6>%s: %s: PCM stream for card is disabled - skipping

> > 2. PLease don't add a pr_fmt() #define to exevry file.  Just put one
> > where all the other CX18_*() macros are defined.  Every file picks those
> > up.
> 
> It's not the first #include of every file.
> printk.h has a default #define pr_fmt(fmt) fmt
> 

Well then don't use "pr_fmt(fmt)" in cx18, if it overloads a define
somewhere else in the kernel and has a dependency on its order relative
to #include statements.  That sort of thing just ups maintenance hours
later.  That's not a good trade off for subjectively better log
messages.

Won't redifining the 'pr_fmt(fmt)' generate preprocessor warnings
anyway?

NACK.

Regards,
Andy 