RE: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

2010-08-13 Thread Gopinath, Thara 


-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 Hilmankhil...@deeprootsystems.com
 Cc: Thara Gopinathth...@ti.com

 Signed-off-by: Nishanth Menonn...@ti.com
 ---
 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_debgsome entry 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

2010-08-06 Thread Gopinath, Thara 


-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 khil...@deeprootsystems.com
Cc: Thara Gopinath th...@ti.com

Signed-off-by: Nishanth Menon n...@ti.com
---
 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

Re: [PM-SR][PATCH 01/12] omap3: voltage: cleanup pr_xxxx

2010-08-06 Thread Nishanth Menon 

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 Hilmankhil...@deeprootsystems.com
Cc: Thara Gopinathth...@ti.com

Signed-off-by: Nishanth Menonn...@ti.com
---
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

2010-08-06 Thread Mark Brown 
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

2010-08-06 Thread Nishanth Menon 

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

2010-08-06 Thread Mark Brown 
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

2010-08-06 Thread Nishanth Menon 

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

2010-08-06 Thread Mark Brown 
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