2011-05-02 13:52:42

by Nicolas Kaiser

[permalink] [raw]
Subject: [PATCH resend] 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(-)

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


2011-05-02 15:01:51

by Martin Schwidefsky

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

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.

--
blue skies,
Martin.

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

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

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]>
> ---
> 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

2011-05-04 16:06:33

by Maynard Johnson

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

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 <[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.....

2011-05-10 08:55:18

by Martin Schwidefsky

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

On Tue, 3 May 2011 10:55:40 +0200
Robert Richter <[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.

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

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.

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

On 10.05.11 04:55:10, Martin Schwidefsky wrote:
> On Tue, 3 May 2011 10:55:40 +0200
> Robert Richter <[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.
>
> 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]>

Looks good.

Acked-by: Robert Richter <[email protected]>

Martin, can you queue this up?

Thanks,

-Robert

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

2011-05-10 09:19:49

by Martin Schwidefsky

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

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

> > 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]>
>
> Looks good.
>
> Acked-by: Robert Richter <[email protected]>
>
> Martin, can you queue this up?

Yes, I will do that.

--
blue skies,
Martin.

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