diff mbox

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

Message ID 1314222175.15882.8.camel@Joe-Laptop (mailing list archive)
State New, archived
Headers show

Commit Message

Joe Perches Aug. 24, 2011, 9:42 p.m. UTC 
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

> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andy Walls Aug. 27, 2011, 1:28 p.m. UTC | #1 
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 




--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Aug. 27, 2011, 4:42 p.m. UTC | #2 
On Sat, 2011-08-27 at 09:28 -0400, Andy Walls wrote:
> 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
[]
> Yuck.
> > > 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?

No.

Andy, I fully understand how this stuff works.
You apparently don't (yet).

Look at include/linux/printk.h

#ifndef pr_fmt
#define pr_fmt(fmt) fmt
#endif

A default empty define is used when one
is not specified before printk.h is
included.  kernel.h includes printk.h

v4l2_<level> uses the "name" of the video
device in its output.  That name may not
be the same name as the module.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Francois Moine Aug. 27, 2011, 5:05 p.m. UTC | #3 
On Sat, 27 Aug 2011 09:42:32 -0700
Joe Perches <joe@perches.com> wrote:

> Andy, I fully understand how this stuff works.
> You apparently don't (yet).
> 
> Look at include/linux/printk.h
> 
> #ifndef pr_fmt
> #define pr_fmt(fmt) fmt
> #endif
> 
> A default empty define is used when one
> is not specified before printk.h is
> included.  kernel.h includes printk.h

Hi Joe,

Yes, but, what if pr_fmt is redefined in some driver specific include
by:

#undef pr_fmt
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

(in gspca.h for example) ?
Andy Walls Aug. 27, 2011, 5:23 p.m. UTC | #4 
Joe Perches <joe@perches.com> wrote:

>On Sat, 2011-08-27 at 09:28 -0400, Andy Walls wrote:
>> 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
>[]
>> Yuck.
>> > > 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?
>
>No.
>
>Andy, I fully understand how this stuff works.
>You apparently don't (yet).
>
>Look at include/linux/printk.h
>
>#ifndef pr_fmt
>#define pr_fmt(fmt) fmt
>#endif
>
>A default empty define is used when one
>is not specified before printk.h is
>included.  kernel.h includes printk.h
>
>v4l2_<level> uses the "name" of the video
>device in its output.  That name may not
>be the same name as the module.
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Joe,

I don't need to fully understand it.

This is a happy to glad change with no functional nor performance benefit.  It adds unneeded lines of code to the driver and mangles some of the log messages.

I see no benefit from my perspective.

Regards,
Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Aug. 27, 2011, 5:31 p.m. UTC | #5 
On Sat, 2011-08-27 at 19:05 +0200, Jean-Francois Moine wrote:
> On Sat, 27 Aug 2011 09:42:32 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > Andy, I fully understand how this stuff works.
> > You apparently don't (yet).
> > 
> > Look at include/linux/printk.h
> > 
> > #ifndef pr_fmt
> > #define pr_fmt(fmt) fmt
> > #endif
> > 
> > A default empty define is used when one
> > is not specified before printk.h is
> > included.  kernel.h includes printk.h
> 
> Hi Joe,
> 
> Yes, but, what if pr_fmt is redefined in some driver specific include
> by:
> 
> #undef pr_fmt
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Of course that's possible.

But any pr_<level> that is used by any .h file
that is included before this redefine like
for instance netdevice.h doesn't have a
properly specified pr_fmt.


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Sept. 4, 2011, 12:21 a.m. UTC | #6 
Em 27-08-2011 14:23, Andy Walls escreveu:
> Joe Perches <joe@perches.com> wrote:
> 
>> On Sat, 2011-08-27 at 09:28 -0400, Andy Walls wrote:
>>> 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
>> []
>>> Yuck.
>>>>> 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?
>>
>> No.
>>
>> Andy, I fully understand how this stuff works.
>> You apparently don't (yet).
>>
>> Look at include/linux/printk.h
>>
>> #ifndef pr_fmt
>> #define pr_fmt(fmt) fmt
>> #endif
>>
>> A default empty define is used when one
>> is not specified before printk.h is
>> included.  kernel.h includes printk.h
>>
>> v4l2_<level> uses the "name" of the video
>> device in its output.  That name may not
>> be the same name as the module.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Hi Joe,
> 
> I don't need to fully understand it.
> 
> This is a happy to glad change with no functional nor performance benefit.  It adds unneeded lines of code to the driver and mangles some of the log messages.
> 
> I see no benefit from my perspective.

Hi Andy and Jean-Francois,

From my perspective, the advantage of using the standard macros for
errors are:

1) Consistency. Except for patches 06 and 14, the other patches were
acked by the maintainers or by me, for the drivers that I maintain
or whose maintainer didn't nack. Also, the same sort of macros are
being used on other places at the Kernel;

2) One of the proposed themes for discussion at the KS/2011 is how to
improve the error reporting. While printk works, there are better
ways of doing it than just printing the error at the console. I'm
working on something like that with regards to hardware errors,
reported via MCE and EDAC subsystems. The idea there is to convert
the printk reports into structured trace events, making easier for
userspace to deal with the errors.

Making all places to use the same macros for it using a similar format
seems to be the first step for replacing the current way for a better
one.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Sept. 4, 2011, 12:52 a.m. UTC | #7 
On Sat, 2011-09-03 at 21:21 -0300, Mauro Carvalho Chehab wrote:
> Except for patches 06 and 14, the other patches were
> acked by the maintainers or by me,

After reviewing it again, Jean-Francois did ack 14/14.
https://lkml.org/lkml/2011/8/22/293


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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