RE: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
>>-Original Message-
>>From: linux-omap-ow...@vger.kernel.org
>>[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
>>Nishanth Menon
>>Sent: Friday, August 06, 2010 4:38 PM
>>To: Gopinath, Thara
>>Cc: Menon, Nishanth; linux-omap; Kevin Hilman
>>Subject: Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_
>>
>>On 08/06/2010 02:42 AM, Gopinath, Thara wrote:
>>>
>>>
>>>>> -Original Message-
>>>>> From: Menon, Nishanth
>>>>> Sent: Friday, August 06, 2010 3:54 AM
>>>>> To: linux-omap
>>>>> Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>>>> Subject: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_
>>>>>
>>>>> Few more pr_ need cleanup for printing the function name and
>>>>> not using multiline prints when c allows us to do "".
>>>>>
>>>>> Cc: Kevin Hilman
>>>>> Cc: Thara Gopinath
>>>>>
>>>>> Signed-off-by: Nishanth Menon
>>>>> ---
>>>>> arch/arm/mach-omap2/voltage.c |7 ---
>>>>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>>>> index 148e4d4..3431fa3 100644
>>>>> --- a/arch/arm/mach-omap2/voltage.c
>>>>> +++ b/arch/arm/mach-omap2/voltage.c
>>>>> @@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
>>>>> u32 *option = data;
>>>>> *option = val;
>>>>> } else {
>>>>> - pr_notice("DEBUG option not enabled!\n \
>>>>> - echo 1> pm_debug/enable_sr_vp_debug - to enable\n");
>>>>> + pr_notice("%s: DEBUG option not enabled! "
>>>>> + "echo 1> pm_debug/enable_sr_vp_debug - to enable\n",
>>>>> + __func__);
>>>>> }
>>>
>>> I do not think this is needed as these fns get called only from user space.
>>>
>>
>>have you tried working on someone else's unfamiliar code and grepping
>>for a warning message at the middle of the night because some developer
>>forgot to give a "%s:...", __func__ with half a dozen people watching
>>behind your back to know it's meaning because the product is hitting the
>>shelves the next day? Instead of being able to use cscope and pop the
>>function in and go straight to it and understand what is happening?? I
>>believe there are few in this list who had the fortune to be in that sitn..
>>
>>
>>Well that is my motivation here. if driver reports a warning to kernel
>>syslog, well, I expect the log to contain the function name for me to
>>maintain faster.
Bingo!! Kudos on managing to prove that you are a cut above the rest. Poor moi
wont understand at all as moi do not work at all!!
But unless there are others in the community who feel that this is absolutely
necessary I am not pulling this in. For me you do a
cat /debug/pm_debg and get an error it is easy to figure out in the
kernel
with the limited experience I have working on linux kernel.
Regards
Thara
>>
>>> Regards
>>> Thara
>>>
>>>>> return 0;
>>>>> }
>>>>> @@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
>>>>> u8 vsel;
>>>>>
>>>>> vsel = voltage_read_reg(info->vp_offs.voltage_reg);
>>>>> - pr_notice("curr_vsel = %x\n", vsel);
>>>>> + pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
>>>>> *val = vsel * 12500 + 60;
>>>>>
>>>>> return 0;
>>>>> --
>>>>> 1.6.3.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>--
>>To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>the body of a message to majord...@vger.kernel.org
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
On Fri, Aug 06, 2010 at 08:37:52AM -0500, Nishanth Menon wrote: > >Interesting, I see better performance than that, especially hot cache, > >and of course an educated guess as to where to look helps greatly. > tell me about it.. ;) mebbe I can use this thread to get a machine > upgrade budget :P.. but I guess there are a lot more folks stuck > like me.. > just for curiosity sake, this data was on a dual core > model name: Intel(R) Pentium(R) 4 CPU 3.40GHz > bogomips : 6800.29 > with a 7200RPM SATA 500GB drive :(.. That's actually better in most respects than the laptop I tried for comparison. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
Mark Brown had written, on 08/06/2010 08:32 AM, the following:
On Fri, Aug 06, 2010 at 08:10:44AM -0500, Nishanth Menon wrote:
a) when the strings are split up when they go multiple lines:
E.g.:
"abcd "
"efg"
print comes out abcd efg
That's generally considered bad practice for precisely this reason.
I agree, but we dont normally have an cleaner option other than:
a) reduce the size of the print message
b) reduce the nesting to give us more space to print ;)
but things get a little more worse now that I think of my personal
experience,
e.g.:
pr_info("%s is not active\n", my_struct->name);
if name is "mpu", then you get a print of:
mpu is not active, and a git grep does not match up either...
c) time required:
$ time git grep "DEBUG option not enabled" .
arch/arm/mach-omap2/smartreflex.c: pr_notice("DEBUG
option not enabled!\n \
arch/arm/mach-omap2/voltage.c: pr_notice("DEBUG option not
enabled!\n \
real1m34.722s
user0m0.440s
sys 0m1.820s
Vs cscope or ctags where it is rather instantaneous if you know the
function name..
Interesting, I see better performance than that, especially hot cache,
and of course an educated guess as to where to look helps greatly.
tell me about it.. ;) mebbe I can use this thread to get a machine
upgrade budget :P.. but I guess there are a lot more folks stuck like me..
just for curiosity sake, this data was on a dual core
model name : Intel(R) Pentium(R) 4 CPU 3.40GHz
bogomips: 6800.29
with a 7200RPM SATA 500GB drive :(..
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
On Fri, Aug 06, 2010 at 08:10:44AM -0500, Nishanth Menon wrote:
> a) when the strings are split up when they go multiple lines:
> E.g.:
> "abcd "
> "efg"
> print comes out abcd efg
That's generally considered bad practice for precisely this reason.
> c) time required:
> $ time git grep "DEBUG option not enabled" .
> arch/arm/mach-omap2/smartreflex.c: pr_notice("DEBUG
> option not enabled!\n \
> arch/arm/mach-omap2/voltage.c: pr_notice("DEBUG option not
> enabled!\n \
> real 1m34.722s
> user 0m0.440s
> sys 0m1.820s
> Vs cscope or ctags where it is rather instantaneous if you know the
> function name..
Interesting, I see better performance than that, especially hot cache,
and of course an educated guess as to where to look helps greatly.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
Mark Brown had written, on 08/06/2010 07:18 AM, the following:
On Fri, Aug 06, 2010 at 06:08:03AM -0500, Nishanth Menon wrote:
Well that is my motivation here. if driver reports a warning to kernel
syslog, well, I expect the log to contain the function name for me to
maintain faster.
That's really not the expectation for Linux log messages - the general
approach to find the source of a message is generally to just grep for
the message text which is usually very effective.
taking a small sample set of pr_xyz(); (pr which spans a single line):
$ git grep pr_ drivers/|grep ")"|grep __func__|wc -l
589
$ git grep pr_ drivers/|grep ")"|grep -v __func__|wc -l
5373
$ git grep pr_fmt drivers/|wc -l
138
Reading Documentation/dynamic-debug-howto.txt, I see that you are in a
one way right. I can get fine grained control over the log by enabling
CONFIG_DYNAMIC_DEBUG.
At the same time, I have wondered on the usage of pr_fmt() in many of
the drivers. in a way, if i wanted to be that verbose in a driver, I
could in theory do:
#define pr_fmt(fmt) "%s:" fmt, __func__ and get the benefit throughout
the file..
but overall, I still disagree that we dont need to have the function
name in log is a speed booster for maintenance folks.
a) when the strings are split up when they go multiple lines:
E.g.:
"abcd "
"efg"
print comes out abcd efg
i) you do a git grep "abcd efg" will return nada
ii) if you do a git grep of "abcd", you will probably see half a dozen
prints there, then open each file to see where the "real" message is,
and you find the file searching a bit
b) when there are couple more usage in cases of commonly used error
message- (e.g. invalid parameters), you end up getting multiple hits,
and you are left guessing where it came from
in this example: lets see: (on l-o pm branch):
git grep "DEBUG option not enabled" .
arch/arm/mach-omap2/smartreflex.c: pr_notice("DEBUG option
not enabled!\n \
arch/arm/mach-omap2/voltage.c: pr_notice("DEBUG option not
enabled!\n \
now open up both the files to find exactly what you are looking for.
c) time required:
$ time git grep "DEBUG option not enabled" .
arch/arm/mach-omap2/smartreflex.c: pr_notice("DEBUG option
not enabled!\n \
arch/arm/mach-omap2/voltage.c: pr_notice("DEBUG option not
enabled!\n \
real1m34.722s
user0m0.440s
sys 0m1.820s
Vs cscope or ctags where it is rather instantaneous if you know the
function name..
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
On Fri, Aug 06, 2010 at 06:08:03AM -0500, Nishanth Menon wrote: > Well that is my motivation here. if driver reports a warning to kernel > syslog, well, I expect the log to contain the function name for me to > maintain faster. That's really not the expectation for Linux log messages - the general approach to find the source of a message is generally to just grep for the message text which is usually very effective. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
On 08/06/2010 02:42 AM, Gopinath, Thara wrote:
-Original Message-
From: Menon, Nishanth
Sent: Friday, August 06, 2010 3:54 AM
To: linux-omap
Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
Subject: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_
Few more pr_ need cleanup for printing the function name and
not using multiline prints when c allows us to do "".
Cc: Kevin Hilman
Cc: Thara Gopinath
Signed-off-by: Nishanth Menon
---
arch/arm/mach-omap2/voltage.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 148e4d4..3431fa3 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
u32 *option = data;
*option = val;
} else {
- pr_notice("DEBUG option not enabled!\n \
- echo 1> pm_debug/enable_sr_vp_debug - to enable\n");
+ pr_notice("%s: DEBUG option not enabled! "
+ "echo 1> pm_debug/enable_sr_vp_debug - to enable\n",
+ __func__);
}
I do not think this is needed as these fns get called only from user space.
have you tried working on someone else's unfamiliar code and grepping
for a warning message at the middle of the night because some developer
forgot to give a "%s:...", __func__ with half a dozen people watching
behind your back to know it's meaning because the product is hitting the
shelves the next day? Instead of being able to use cscope and pop the
function in and go straight to it and understand what is happening?? I
believe there are few in this list who had the fortune to be in that sitn..
Well that is my motivation here. if driver reports a warning to kernel
syslog, well, I expect the log to contain the function name for me to
maintain faster.
Regards
Thara
return 0;
}
@@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
u8 vsel;
vsel = voltage_read_reg(info->vp_offs.voltage_reg);
- pr_notice("curr_vsel = %x\n", vsel);
+ pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
*val = vsel * 12500 + 60;
return 0;
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
>>-Original Message-
>>From: Menon, Nishanth
>>Sent: Friday, August 06, 2010 3:54 AM
>>To: linux-omap
>>Cc: Menon, Nishanth; Kevin Hilman; Gopinath, Thara
>>Subject: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_
>>
>>Few more pr_ need cleanup for printing the function name and
>>not using multiline prints when c allows us to do "".
>>
>>Cc: Kevin Hilman
>>Cc: Thara Gopinath
>>
>>Signed-off-by: Nishanth Menon
>>---
>> arch/arm/mach-omap2/voltage.c |7 ---
>> 1 files changed, 4 insertions(+), 3 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>index 148e4d4..3431fa3 100644
>>--- a/arch/arm/mach-omap2/voltage.c
>>+++ b/arch/arm/mach-omap2/voltage.c
>>@@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
>> u32 *option = data;
>> *option = val;
>> } else {
>>- pr_notice("DEBUG option not enabled!\n \
>>- echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
>>+ pr_notice("%s: DEBUG option not enabled! "
>>+ "echo 1 > pm_debug/enable_sr_vp_debug - to enable\n",
>>+ __func__);
>> }
I do not think this is needed as these fns get called only from user space.
Regards
Thara
>> return 0;
>> }
>>@@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
>> u8 vsel;
>>
>> vsel = voltage_read_reg(info->vp_offs.voltage_reg);
>>- pr_notice("curr_vsel = %x\n", vsel);
>>+ pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
>> *val = vsel * 12500 + 60;
>>
>> return 0;
>>--
>>1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx
Few more pr_ need cleanup for printing the function name and
not using multiline prints when c allows us to do "".
Cc: Kevin Hilman
Cc: Thara Gopinath
Signed-off-by: Nishanth Menon
---
arch/arm/mach-omap2/voltage.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 148e4d4..3431fa3 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -253,8 +253,9 @@ static int vp_debug_set(void *data, u64 val)
u32 *option = data;
*option = val;
} else {
- pr_notice("DEBUG option not enabled!\n \
- echo 1 > pm_debug/enable_sr_vp_debug - to enable\n");
+ pr_notice("%s: DEBUG option not enabled! "
+ "echo 1 > pm_debug/enable_sr_vp_debug - to enable\n",
+ __func__);
}
return 0;
}
@@ -265,7 +266,7 @@ static int vp_volt_debug_get(void *data, u64 *val)
u8 vsel;
vsel = voltage_read_reg(info->vp_offs.voltage_reg);
- pr_notice("curr_vsel = %x\n", vsel);
+ pr_notice("%s: curr_vsel = %x\n", __func__, vsel);
*val = vsel * 12500 + 60;
return 0;
--
1.6.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
