[PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Nicolas Kaiser]

Checking 'oprofile_min_interval < 0' and
'oprofile_max_interval < 0' doesn't work because
'oprofile_min_interval' and 'oprofile_max_interval' are unsigned.

Signed-off-by: Nicolas Kaiser <nikai@nikai.net>
---
Untested.

 arch/s390/oprofile/init.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/s390/oprofile/init.c b/arch/s390/oprofile/init.c
index c63d7e5..09c3088 100644
--- a/arch/s390/oprofile/init.c
+++ b/arch/s390/oprofile/init.c
@@ -138,22 +138,26 @@ static int oprofile_create_hwsampling_files(struct super_block *sb,
 
 static int oprofile_hwsampler_init(struct oprofile_operations *ops)
 {
+	long retval;
+
 	if (hwsampler_setup())
 		return -ENODEV;
 
 	/*
 	 * create hwsampler files only if hwsampler_setup() succeeds.
 	 */
-	oprofile_min_interval = hwsampler_query_min_interval();
-	if (oprofile_min_interval < 0) {
+	retval = hwsampler_query_min_interval();
+	if (retval < 0) {
 		oprofile_min_interval = 0;
 		return -ENODEV;
 	}
-	oprofile_max_interval = hwsampler_query_max_interval();
-	if (oprofile_max_interval < 0) {
+	oprofile_min_interval = retval;
+	retval = hwsampler_query_max_interval();
+	if (retval < 0) {
 		oprofile_max_interval = 0;
 		return -ENODEV;
 	}
+	oprofile_max_interval = retval;
 
 	if (oprofile_timer_init(ops))
 		return -ENODEV;
-- 
1.7.3.4

Re: [PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Martin Schwidefsky]

On Mon, 2 May 2011 15:48:05 +0200
Nicolas Kaiser <nikai@nikai.net> wrote:

> Checking 'oprofile_min_interval < 0' and
> 'oprofile_max_interval < 0' doesn't work because
> 'oprofile_min_interval' and 'oprofile_max_interval' are unsigned.
> 
> Signed-off-by: Nicolas Kaiser <nikai@nikai.net>

Makes sense, I'll queue it up. Thanks.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Robert Richter]

On 02.05.11 09:48:05, Nicolas Kaiser wrote:
> Checking 'oprofile_min_interval < 0' and
> 'oprofile_max_interval < 0' doesn't work because
> 'oprofile_min_interval' and 'oprofile_max_interval' are unsigned.

max/min_interval are through all the code always unsigned. I don't
know how min/max_sampl_rate in struct hws_qsi_info_block is spec'ed,
but there it is unsigned too.

So the best would be to return qsi.min/max_sampl_rate in
hwsampler_query_min/max_interval() directly with no error codes as
unsigned longs and to change the code in oprofile_hwsampler_init() to
check for null. Both functions hwsampler_query_min/max_interval()
could be moved to hwsampler.h as static inline functions. This makes
the code also easier.

This patch does not handle the null value case and the data truncation
by casting from unsigned to singed is not fixed.

-Robert

> 
> Signed-off-by: Nicolas Kaiser <nikai@nikai.net>
> ---
> Untested.
> 
>  arch/s390/oprofile/init.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/oprofile/init.c b/arch/s390/oprofile/init.c
> index c63d7e5..09c3088 100644
> --- a/arch/s390/oprofile/init.c
> +++ b/arch/s390/oprofile/init.c
> @@ -138,22 +138,26 @@ static int oprofile_create_hwsampling_files(struct super_block *sb,
>  
>  static int oprofile_hwsampler_init(struct oprofile_operations *ops)
>  {
> +	long retval;
> +
>  	if (hwsampler_setup())
>  		return -ENODEV;
>  
>  	/*
>  	 * create hwsampler files only if hwsampler_setup() succeeds.
>  	 */
> -	oprofile_min_interval = hwsampler_query_min_interval();
> -	if (oprofile_min_interval < 0) {
> +	retval = hwsampler_query_min_interval();
> +	if (retval < 0) {
>  		oprofile_min_interval = 0;
>  		return -ENODEV;
>  	}
> -	oprofile_max_interval = hwsampler_query_max_interval();
> -	if (oprofile_max_interval < 0) {
> +	oprofile_min_interval = retval;
> +	retval = hwsampler_query_max_interval();
> +	if (retval < 0) {
>  		oprofile_max_interval = 0;
>  		return -ENODEV;
>  	}
> +	oprofile_max_interval = retval;
>  
>  	if (oprofile_timer_init(ops))
>  		return -ENODEV;
> -- 
> 1.7.3.4
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Maynard Johnson]

On 05/04/2011 2:49 AM, durga prasad wrote:
>
> Thanks for the reply.
Durga, you're replying to the wrong thread.

-Maynard

> I can able to run oprofile on the target(ARM V7-CA9) in timer mode.
> The problem of Bad magic number is because of the JFFS2 file system.So I mounted
> usb on the target and colleted the sample on the target.
> I am able to analyze the sample on the target.But for analyzing the samples on
> the host I am doing the follows process.
>
> I mounted the USB to /var/lib/oprofile -- so the sample will be collected
> directly to USB
>
> After the I am running oparchieve on the target
>
> *oparchive -o /var/lib/oprofile/durga/*
>
> I am doing opimport on the host PC
>
> *# opimport -V -a /media/Pen_drive/durga/ -o /home/durga/oprofile/
>    error: must specify exactly 1 input file
>
> *I am getting the above error always.*
> *How to analyze the sample collected in the Host PC after collecting the samples
> in Pen drive from target*?
>
>
> *Your *help* will *be* warmly received.
>
> Regards,
> Durga Prasad
> *
>
>
> *
>
>
>
>
> On Tue, May 3, 2011 at 5:55 PM, Robert Richter <robert.richter@amd.com
> <mailto:robert.richter@amd.com>> wrote:
>
>     On 02.05.11 09:48:05, Nicolas Kaiser wrote:
>     >  Checking 'oprofile_min_interval < 0' and
>     >  'oprofile_max_interval < 0' doesn't work because
>     >  'oprofile_min_interval' and 'oprofile_max_interval' are unsigned.
>
>     max/min_interval are through all the code always unsigned. I don't
>     know how min/max_sampl_rate in struct hws_qsi_info_block is spec'ed,
>     but there it is unsigned too.
>
>     So the best would be to return qsi.min/max_sampl_rate in
>     hwsampler_query_min/max_interval() directly with no error codes as
>     unsigned longs and to change the code in oprofile_hwsampler_init() to
>     check for null. Both functions hwsampler_query_min/max_interval()
>     could be moved to hwsampler.h as static inline functions. This makes
>     the code also easier.
>
>     This patch does not handle the null value case and the data truncation
>     by casting from unsigned to singed is not fixed.
>
>     -Robert
>
>     >
>     >  Signed-off-by: Nicolas Kaiser <nikai@nikai.net <mailto:nikai@nikai.net>>
>     >  ---
>     >  Untested.
>     >
>     >   arch/s390/oprofile/init.c |   12 ++++++++----
>     >   1 files changed, 8 insertions(+), 4 deletions(-)
>     >
>     >  diff --git a/arch/s390/oprofile/init.c b/arch/s390/oprofile/init.c
>     >  index c63d7e5..09c3088 100644
>     >  --- a/arch/s390/oprofile/init.c
>     >  +++ b/arch/s390/oprofile/init.c
>     >  @@ -138,22 +138,26 @@ static int oprofile_create_hwsampling_files(struct
>     super_block *sb,
>     >
>     >   static int oprofile_hwsampler_init(struct oprofile_operations *ops)
>     >   {
>     >  +     long retval;
>     >  +
>     >        if (hwsampler_setup())
>     >                return -ENODEV;
>     >
>     >        /*
>     >         * create hwsampler files only if hwsampler_setup() succeeds.
>     >         */
>     >  -     oprofile_min_interval = hwsampler_query_min_interval();
>     >  -     if (oprofile_min_interval < 0) {
>     >  +     retval = hwsampler_query_min_interval();
>     >  +     if (retval < 0) {
>     >                oprofile_min_interval = 0;
>     >                return -ENODEV;
>     >        }
>     >  -     oprofile_max_interval = hwsampler_query_max_interval();
>     >  -     if (oprofile_max_interval < 0) {
>     >  +     oprofile_min_interval = retval;
>     >  +     retval = hwsampler_query_max_interval();
>     >  +     if (retval < 0) {
>     >                oprofile_max_interval = 0;
>     >                return -ENODEV;
>     >        }
>     >  +     oprofile_max_interval = retval;
>     >
>     >        if (oprofile_timer_init(ops))
>     >                return -ENODEV;
>     >  --
>     >  1.7.3.4
>     >
>
>     --
>     Advanced Micro Devices, Inc.
>     Operating System Research Center
>
>
>     ------------------------------------------------------------------------------
>     WhatsUp Gold - Download Free Network Management Software
>     The most intuitive, comprehensive, and cost-effective network
>     management toolset available today.  Delivers lowest initial
>     acquisition cost and overall TCO of any competing solution.
>     http://p.sf.net/sfu/whatsupgold-sd
>     _______________________________________________
>     oprofile-list mailing list
>     oprofile-list@lists.sourceforge.net <mailto:oprofile-list@lists.sourceforge.net>
>     https://lists.sourceforge.net/lists/listinfo/oprofile-list
>
>
>
>
> --
> regards
> prasad.....

Re: [PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Martin Schwidefsky]

On Tue, 3 May 2011 10:55:40 +0200
Robert Richter <robert.richter@amd.com> wrote:

> On 02.05.11 09:48:05, Nicolas Kaiser wrote:
> > Checking 'oprofile_min_interval < 0' and
> > 'oprofile_max_interval < 0' doesn't work because
> > 'oprofile_min_interval' and 'oprofile_max_interval' are unsigned.
> 
> max/min_interval are through all the code always unsigned. I don't
> know how min/max_sampl_rate in struct hws_qsi_info_block is spec'ed,
> but there it is unsigned too.
> 
> So the best would be to return qsi.min/max_sampl_rate in
> hwsampler_query_min/max_interval() directly with no error codes as
> unsigned longs and to change the code in oprofile_hwsampler_init() to
> check for null. Both functions hwsampler_query_min/max_interval()
> could be moved to hwsampler.h as static inline functions. This makes
> the code also easier.
> 
> This patch does not handle the null value case and the data truncation
> by casting from unsigned to singed is not fixed.

Ok, the improved patch now looks like this:
--
Subject: [PATCH] s390: oprofile: fix min/max interval query checks

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

oprofile_min_interval and oprofile_max_interval are unsigned, checking
for negative values doesn't work. Change hwsampler_query_min_interval
and hwsampler_query_max_interval to return an unsigned long and
check for a zero value instead.

Reported-by: Nicolas Kaiser <nikai@nikai.net>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/oprofile/hwsampler.c |   14 ++++----------
 arch/s390/oprofile/hwsampler.h |    4 ++--
 arch/s390/oprofile/init.c      |    8 ++------
 3 files changed, 8 insertions(+), 18 deletions(-)

diff -urpN linux-2.6/arch/s390/oprofile/hwsampler.c linux-2.6-patched/arch/s390/oprofile/hwsampler.c
--- linux-2.6/arch/s390/oprofile/hwsampler.c	2011-05-10 10:53:04.000000000 +0200
+++ linux-2.6-patched/arch/s390/oprofile/hwsampler.c	2011-05-10 10:53:11.000000000 +0200
@@ -1021,20 +1021,14 @@ deallocate_exit:
 	return rc;
 }
 
-long hwsampler_query_min_interval(void)
+unsigned long hwsampler_query_min_interval(void)
 {
-	if (min_sampler_rate)
-		return min_sampler_rate;
-	else
-		return -EINVAL;
+	return min_sampler_rate;
 }
 
-long hwsampler_query_max_interval(void)
+unsigned long hwsampler_query_max_interval(void)
 {
-	if (max_sampler_rate)
-		return max_sampler_rate;
-	else
-		return -EINVAL;
+	return max_sampler_rate;
 }
 
 unsigned long hwsampler_get_sample_overflow_count(unsigned int cpu)
diff -urpN linux-2.6/arch/s390/oprofile/hwsampler.h linux-2.6-patched/arch/s390/oprofile/hwsampler.h
--- linux-2.6/arch/s390/oprofile/hwsampler.h	2011-05-10 10:53:04.000000000 +0200
+++ linux-2.6-patched/arch/s390/oprofile/hwsampler.h	2011-05-10 10:53:11.000000000 +0200
@@ -102,8 +102,8 @@ int hwsampler_setup(void);
 int hwsampler_shutdown(void);
 int hwsampler_allocate(unsigned long sdbt, unsigned long sdb);
 int hwsampler_deallocate(void);
-long hwsampler_query_min_interval(void);
-long hwsampler_query_max_interval(void);
+unsigned long hwsampler_query_min_interval(void);
+unsigned long hwsampler_query_max_interval(void);
 int hwsampler_start_all(unsigned long interval);
 int hwsampler_stop_all(void);
 int hwsampler_deactivate(unsigned int cpu);
diff -urpN linux-2.6/arch/s390/oprofile/init.c linux-2.6-patched/arch/s390/oprofile/init.c
--- linux-2.6/arch/s390/oprofile/init.c	2011-05-10 10:53:04.000000000 +0200
+++ linux-2.6-patched/arch/s390/oprofile/init.c	2011-05-10 10:53:11.000000000 +0200
@@ -145,15 +145,11 @@ static int oprofile_hwsampler_init(struc
 	 * create hwsampler files only if hwsampler_setup() succeeds.
 	 */
 	oprofile_min_interval = hwsampler_query_min_interval();
-	if (oprofile_min_interval < 0) {
-		oprofile_min_interval = 0;
+	if (oprofile_min_interval == 0)
 		return -ENODEV;
-	}
 	oprofile_max_interval = hwsampler_query_max_interval();
-	if (oprofile_max_interval < 0) {
-		oprofile_max_interval = 0;
+	if (oprofile_max_interval == 0)
 		return -ENODEV;
-	}
 
 	if (oprofile_timer_init(ops))
 		return -ENODEV;

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Robert Richter]

On 10.05.11 04:55:10, Martin Schwidefsky wrote:
> On Tue, 3 May 2011 10:55:40 +0200
> Robert Richter <robert.richter@amd.com> wrote:
> 
> > On 02.05.11 09:48:05, Nicolas Kaiser wrote:
> > > Checking 'oprofile_min_interval < 0' and
> > > 'oprofile_max_interval < 0' doesn't work because
> > > 'oprofile_min_interval' and 'oprofile_max_interval' are unsigned.
> > 
> > max/min_interval are through all the code always unsigned. I don't
> > know how min/max_sampl_rate in struct hws_qsi_info_block is spec'ed,
> > but there it is unsigned too.
> > 
> > So the best would be to return qsi.min/max_sampl_rate in
> > hwsampler_query_min/max_interval() directly with no error codes as
> > unsigned longs and to change the code in oprofile_hwsampler_init() to
> > check for null. Both functions hwsampler_query_min/max_interval()
> > could be moved to hwsampler.h as static inline functions. This makes
> > the code also easier.
> > 
> > This patch does not handle the null value case and the data truncation
> > by casting from unsigned to singed is not fixed.
> 
> Ok, the improved patch now looks like this:
> --
> Subject: [PATCH] s390: oprofile: fix min/max interval query checks
> 
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> oprofile_min_interval and oprofile_max_interval are unsigned, checking
> for negative values doesn't work. Change hwsampler_query_min_interval
> and hwsampler_query_max_interval to return an unsigned long and
> check for a zero value instead.
> 
> Reported-by: Nicolas Kaiser <nikai@nikai.net>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

Looks good.

Acked-by: Robert Richter <robert.richter@amd.com>

Martin, can you queue this up?

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH resend] s390: oprofile: fix error checks in oprofile_hwsampler_init()

[Martin Schwidefsky]

On Tue, 10 May 2011 11:00:54 +0200
Robert Richter <robert.richter@amd.com> wrote:

> > Ok, the improved patch now looks like this:
> > --
> > Subject: [PATCH] s390: oprofile: fix min/max interval query checks
> > 
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > 
> > oprofile_min_interval and oprofile_max_interval are unsigned, checking
> > for negative values doesn't work. Change hwsampler_query_min_interval
> > and hwsampler_query_max_interval to return an unsigned long and
> > check for a zero value instead.
> > 
> > Reported-by: Nicolas Kaiser <nikai@nikai.net>
> > Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Looks good.
> 
> Acked-by: Robert Richter <robert.richter@amd.com>
> 
> Martin, can you queue this up?

Yes, I will do that.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.