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

[Mauro Carvalho Chehab]

Em 27-08-2011 14:23, Andy Walls escreveu:
> Joe Perches <joe@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/