s390: oprofile: fix error checks in oprofile_hwsampler_init()

PATCH RESEND In review Nicolas Kaiser 4 participants 7 messages May 2, 2011 – May 10, 2011 1 message not in the archive s390

lore

Share & tools

lore

Nicolas Kaiser

author

2011-05-02 13:48 UTC #0 lore raw

PATCH RESEND 1/1 s390: oprofile: fix error checks in oprofile_hwsampler_init()

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 <[email protected]>

---
Untested.

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

arch/s390/oprofile/init.c +8 -4

index c63d7e5..09c3088 100644
@@ -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;

Signature · 1 line

1.7.3.4

Martin Schwidefsky

2011-05-02 15:01 UTC #1 lore raw

Quoted 5 lines · On Mon, 2 May 2011 15:48:05 +0200 Nicolas Kaiser <[email protected]> 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 <[email protected]>

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

Signature · 4 lines

blue skies,
   Martin.

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

Robert Richter

2011-05-03 08:55 UTC #2 lore raw

In reply to Nicolas Kaiser

Quoted 3 lines · 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.

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

Quoted 46 lines

Signed-off-by Nicolas Kaiser <[email protected]>

---
Untested.

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

arch/s390/oprofile/init.c +8 -4

index c63d7e5..09c3088 100644
@@ -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;

Signature · 1 line

1.7.3.4

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

A message that is not in the archive (sent off-list or not yet received): [email protected]

Maynard Johnson

2011-05-04 16:00 UTC #4 lore raw

Quoted 2 lines · durga prasad

Thanks for the reply.

Durga, you're replying to the wrong thread.

-Maynard

Quoted 129 lines

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 <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>>
    >  ---
    >  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
    [email protected] <mailto:[email protected]>
    https://lists.sourceforge.net/lists/listinfo/oprofile-list

--
regards
prasad.....

Martin Schwidefsky

2011-05-10 08:55 UTC #5 lore raw

In reply to Robert Richter

Quoted 16 lines · On Tue, 3 May 2011 10:55:40 +0200 Robert Richter <[email protected]> wrote:

Quoted 3 lines · 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.
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.

Quoted 2 lines

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 <[email protected]>

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 <[email protected]>

Signed-off-by Martin Schwidefsky <[email protected]>

---
 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(-)

arch/s390/oprofile/hwsampler.c +4 ~~-10~~

diff -urpN linux-2.6/arch/s390/oprofile/hwsampler.c linux-2.6-patched/arch/s390/oprofile/hwsampler.c
@@ -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)

arch/s390/oprofile/hwsampler.h +2 -2

diff -urpN linux-2.6/arch/s390/oprofile/hwsampler.h linux-2.6-patched/arch/s390/oprofile/hwsampler.h
@@ -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);

arch/s390/oprofile/init.c +2 -6

diff -urpN linux-2.6/arch/s390/oprofile/init.c linux-2.6-patched/arch/s390/oprofile/init.c
@@ -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;

Signature · 4 lines

blue skies,
   Martin.

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

Robert Richter

Acked-by

2011-05-10 09:00 UTC #6 lore raw

Quoted 28 lines · Martin Schwidefsky

Quoted 16 lines · On Tue, 3 May 2011 10:55:40 +0200 Robert Richter <[email protected]> wrote:
Quoted 3 lines · 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.
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.
Quoted 2 lines
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 <[email protected]>

Quoted 7 lines

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 <[email protected]>
Signed-off-by Martin Schwidefsky <[email protected]>

Looks good.

Acked-by Robert Richter <[email protected]>

Martin, can you queue this up?

Thanks,

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

Martin Schwidefsky

2011-05-10 09:19 UTC #7 lore raw

Quoted 11 lines · On Tue, 10 May 2011 11:00:54 +0200 Robert Richter <[email protected]>…

Quoted 11 lines
Ok, the improved patch now looks like this:
--
Subject: [PATCH] s390: oprofile: fix min/max interval query checks
From: Martin Schwidefsky <[email protected]>
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.

Quoted 8 lines

Quoted 2 lines
Reported-by Nicolas Kaiser <[email protected]>
Signed-off-by Martin Schwidefsky <[email protected]>
Looks good.
Acked-by Robert Richter <[email protected]>
Martin, can you queue this up?

Yes, I will do that.

Signature · 4 lines

blue skies,
   Martin.

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