Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Russell King]

Sorry, hit the wrong key.  'T' and 'Y' are too close together.  (mutt)

----- Forwarded message from Russell King <rmk+lkml@arm.linux.org.uk> -----

Date: Fri, 28 Dec 2007 19:57:24 +0000
From: Russell King <rmk+lkml@arm.linux.org.uk>
To: Adrian Bunk <adrian.bunk@movial.fi>
Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>,
	Randy Dunlap <randy.dunlap@oracle.com>, phil.el@wanadoo.fr,
	oprofile-list@lists.sf.net, linux-kernel@vger.kernel.org
Subject: Re: [2.6.24 patch] restore ARMv6 OProfile support

On Fri, Dec 28, 2007 at 08:56:36PM +0200, Adrian Bunk wrote:
> This patch restores the ARMv6 OProfile support that was killed by
> commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.
> 
> Signed-off-by: Adrian Bunk <adrian.bunk@movial.fi>

Given where we are in the release cycle, I think this "cleanup" patch
should be reverted.  Once the issues with it are resolved (why was it
never even CC'd to the arch maintainers for review?) then it can go
into mainline.

Note that it also looks like (from the commitdiff) that the above
mentioned commit also removes:

CONFIG_HARDWARE_PM (blackfin)
CONFIG_OPROFILE_CELL (powerpc)

So they're probably subtly broken as well.

Linus, what do you think?  Should 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9
be reverted?

> 
> ---
> 
>  kernel/Kconfig.instrumentation |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> 7fc221ef169610b5eac98e2ddd641811c0d53e4a 
> diff --git a/kernel/Kconfig.instrumentation b/kernel/Kconfig.instrumentation
> index 468f47a..4453187 100644
> --- a/kernel/Kconfig.instrumentation
> +++ b/kernel/Kconfig.instrumentation
> @@ -29,2 +29,17 @@ config OPROFILE
> 
> +config OPROFILE_ARMV6
> +	bool
> +	depends on OPROFILE && ARM && CPU_V6 && !SMP
> +	default y
> +	select OPROFILE_ARM11_CORE
> +
> +config OPROFILE_MPCORE
> +	bool
> +	depends on OPROFILE && ARM && CPU_V6 && SMP
> +	default y
> +	select OPROFILE_ARM11_CORE
> +
> +config OPROFILE_ARM11_CORE
> +	bool
> +
>  config KPROBES

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

----- End forwarded message -----

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Russell King]

I never got a response on my message, but I have just receieved:

| Date: Tue, 15 Jan 2008 11:05:00 +0100
| From: Joerg Wagner <wagner@it.neclab.eu>
| To: ARM Linux Mailing List <linux-arm-kernel@lists.arm.linux.org.uk>
| Subject: 2.6.24-rc7 : oprofile on MPCore broken
| 
| Hello,
| 
| just tried to use oprofile on 2.6.24-rc7.
| It does not detect the right processor
|  (/dev/oprofile/cpu_type contains "timer").
| 
| As I don't know exactly, how the string
| "arm/mpcore" from arch/arm/oprofile/op_model_mpcore.c
| gets feeded into that file, maybe someone else can help ?

So people are hitting the resulting mess created by 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.
Can we please fix this regression one way or another please?

I don't particularly like stuffing the options into some random place
in the architectures Kconfig file when they should stay along side the
instrumentation configuration entries.

On Fri, Dec 28, 2007 at 07:58:41PM +0000, Russell King wrote:
> Sorry, hit the wrong key.  'T' and 'Y' are too close together.  (mutt)
> 
> ----- Forwarded message from Russell King <rmk+lkml@arm.linux.org.uk> -----
> 
> Date: Fri, 28 Dec 2007 19:57:24 +0000
> From: Russell King <rmk+lkml@arm.linux.org.uk>
> To: Adrian Bunk <adrian.bunk@movial.fi>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>,
> 	Randy Dunlap <randy.dunlap@oracle.com>, phil.el@wanadoo.fr,
> 	oprofile-list@lists.sf.net, linux-kernel@vger.kernel.org
> Subject: Re: [2.6.24 patch] restore ARMv6 OProfile support
> 
> On Fri, Dec 28, 2007 at 08:56:36PM +0200, Adrian Bunk wrote:
> > This patch restores the ARMv6 OProfile support that was killed by
> > commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.
> > 
> > Signed-off-by: Adrian Bunk <adrian.bunk@movial.fi>
> 
> Given where we are in the release cycle, I think this "cleanup" patch
> should be reverted.  Once the issues with it are resolved (why was it
> never even CC'd to the arch maintainers for review?) then it can go
> into mainline.
> 
> Note that it also looks like (from the commitdiff) that the above
> mentioned commit also removes:
> 
> CONFIG_HARDWARE_PM (blackfin)
> CONFIG_OPROFILE_CELL (powerpc)
> 
> So they're probably subtly broken as well.
> 
> Linus, what do you think?  Should 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9
> be reverted?
> 
> > 
> > ---
> > 
> >  kernel/Kconfig.instrumentation |   19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > 7fc221ef169610b5eac98e2ddd641811c0d53e4a 
> > diff --git a/kernel/Kconfig.instrumentation b/kernel/Kconfig.instrumentation
> > index 468f47a..4453187 100644
> > --- a/kernel/Kconfig.instrumentation
> > +++ b/kernel/Kconfig.instrumentation
> > @@ -29,2 +29,17 @@ config OPROFILE
> > 
> > +config OPROFILE_ARMV6
> > +	bool
> > +	depends on OPROFILE && ARM && CPU_V6 && !SMP
> > +	default y
> > +	select OPROFILE_ARM11_CORE
> > +
> > +config OPROFILE_MPCORE
> > +	bool
> > +	depends on OPROFILE && ARM && CPU_V6 && SMP
> > +	default y
> > +	select OPROFILE_ARM11_CORE
> > +
> > +config OPROFILE_ARM11_CORE
> > +	bool
> > +
> >  config KPROBES
> 
> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:
> 
> ----- End forwarded message -----

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Adrian Bunk]

On Tue, Jan 15, 2008 at 10:45:26AM +0000, Russell King wrote:
> I never got a response on my message, but I have just receieved:
> 
> | Date: Tue, 15 Jan 2008 11:05:00 +0100
> | From: Joerg Wagner <wagner@it.neclab.eu>
> | To: ARM Linux Mailing List <linux-arm-kernel@lists.arm.linux.org.uk>
> | Subject: 2.6.24-rc7 : oprofile on MPCore broken
> | 
> | Hello,
> | 
> | just tried to use oprofile on 2.6.24-rc7.
> | It does not detect the right processor
> |  (/dev/oprofile/cpu_type contains "timer").
> | 
> | As I don't know exactly, how the string
> | "arm/mpcore" from arch/arm/oprofile/op_model_mpcore.c
> | gets feeded into that file, maybe someone else can help ?
> 
> So people are hitting the resulting mess created by 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.
> Can we please fix this regression one way or another please?
> 
> I don't particularly like stuffing the options into some random place
> in the architectures Kconfig file when they should stay along side the
> instrumentation configuration entries.

Below is the patch I already sent on 28 Dec 2007 that stuffs it into 
Kconfig.instrumentation.

Technically it shouldn't make any difference whether this patch or
Mathieu's patch that stuffs it into arch/arm/Kconfig gets applied, but 
one of them should be applied for 2.6.24 (plus either mine or Mathieu's 
fix for the blackfin HARDWARE_PM support broken by the same commit).

I found the bugs in Mathieu's commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9,
I wrote patches that restore the status quo, Cc'ed all people even
remotely related to this issue, and I opened the Bugzilla bugs required
for getting them on the regression lists. Mathieu wants the regressions
he introduced fixed different from what my patches did and that's not a
problem for me (his patches are also OK).

What went wrong that his regression fixes did not land in Linus' tree?

cu
Adrian


<--  snip  -->


This patch restores the ARMv6 OProfile support that was killed by
commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.

Signed-off-by: Adrian Bunk <adrian.bunk@movial.fi>

---

 kernel/Kconfig.instrumentation |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

7fc221ef169610b5eac98e2ddd641811c0d53e4a 
diff --git a/kernel/Kconfig.instrumentation b/kernel/Kconfig.instrumentation
index 468f47a..4453187 100644
--- a/kernel/Kconfig.instrumentation
+++ b/kernel/Kconfig.instrumentation
@@ -29,2 +29,17 @@ config OPROFILE

+config OPROFILE_ARMV6
+	bool
+	depends on OPROFILE && ARM && CPU_V6 && !SMP
+	default y
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_MPCORE
+	bool
+	depends on OPROFILE && ARM && CPU_V6 && SMP
+	default y
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_ARM11_CORE
+	bool
+
 config KPROBES

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Mathieu Desnoyers]

* Adrian Bunk (adrian.bunk@movial.fi) wrote:
> On Tue, Jan 15, 2008 at 10:45:26AM +0000, Russell King wrote:
> > I never got a response on my message, but I have just receieved:
> > 
> > | Date: Tue, 15 Jan 2008 11:05:00 +0100
> > | From: Joerg Wagner <wagner@it.neclab.eu>
> > | To: ARM Linux Mailing List <linux-arm-kernel@lists.arm.linux.org.uk>
> > | Subject: 2.6.24-rc7 : oprofile on MPCore broken
> > | 
> > | Hello,
> > | 
> > | just tried to use oprofile on 2.6.24-rc7.
> > | It does not detect the right processor
> > |  (/dev/oprofile/cpu_type contains "timer").
> > | 
> > | As I don't know exactly, how the string
> > | "arm/mpcore" from arch/arm/oprofile/op_model_mpcore.c
> > | gets feeded into that file, maybe someone else can help ?
> > 
> > So people are hitting the resulting mess created by 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.
> > Can we please fix this regression one way or another please?
> > 
> > I don't particularly like stuffing the options into some random place
> > in the architectures Kconfig file when they should stay along side the
> > instrumentation configuration entries.
> 
> Below is the patch I already sent on 28 Dec 2007 that stuffs it into 
> Kconfig.instrumentation.
> 
> Technically it shouldn't make any difference whether this patch or
> Mathieu's patch that stuffs it into arch/arm/Kconfig gets applied, but 
> one of them should be applied for 2.6.24 (plus either mine or Mathieu's 
> fix for the blackfin HARDWARE_PM support broken by the same commit).
> 
> I found the bugs in Mathieu's commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9,
> I wrote patches that restore the status quo, Cc'ed all people even
> remotely related to this issue, and I opened the Bugzilla bugs required
> for getting them on the regression lists. Mathieu wants the regressions
> he introduced fixed different from what my patches did and that's not a
> problem for me (his patches are also OK).
> 
> What went wrong that his regression fixes did not land in Linus' tree?
> 

Yes Adrian, and I thank you very much for all this work. I only
suggested the alternative patches so it would remove menu options nobody
really needs (historical special-cases) and make it easier to apply the
following set of patches already in -mm.

One way or another, either your or my arm and blackfin patches should
get into 2.6.24 final.

Mathieu

> cu
> Adrian
> 
> 
> <--  snip  -->
> 
> 
> This patch restores the ARMv6 OProfile support that was killed by
> commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.
> 
> Signed-off-by: Adrian Bunk <adrian.bunk@movial.fi>
> 
> ---
> 
>  kernel/Kconfig.instrumentation |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> 7fc221ef169610b5eac98e2ddd641811c0d53e4a 
> diff --git a/kernel/Kconfig.instrumentation b/kernel/Kconfig.instrumentation
> index 468f47a..4453187 100644
> --- a/kernel/Kconfig.instrumentation
> +++ b/kernel/Kconfig.instrumentation
> @@ -29,2 +29,17 @@ config OPROFILE
> 
> +config OPROFILE_ARMV6
> +	bool
> +	depends on OPROFILE && ARM && CPU_V6 && !SMP
> +	default y
> +	select OPROFILE_ARM11_CORE
> +
> +config OPROFILE_MPCORE
> +	bool
> +	depends on OPROFILE && ARM && CPU_V6 && SMP
> +	default y
> +	select OPROFILE_ARM11_CORE
> +
> +config OPROFILE_ARM11_CORE
> +	bool
> +
>  config KPROBES

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Linus Torvalds]

On Tue, 15 Jan 2008, Russell King wrote:
> 
> I don't particularly like stuffing the options into some random place
> in the architectures Kconfig file when they should stay along side the
> instrumentation configuration entries.

Well, I have to say that I don't particularly like obviously 
architecture-specific stuff in an obviously non-architecture file..

I'd almost prefer to revert the thing that caused the problem, because 
with Adrian's patch, I think the end result may *work*, but it's uglier 
than what we started out with.

However, I think the *cleanest* solution right now may be something like 
the appended. Totally untested, of course. It basically just copies the 
generic kernel/Kconfig.instrumentation file into the arm directory, makes 
arm use its own instead of the generic one, and removes the dependencies 
on ARM in there (including all of the KPROBES entry that apparently isn't 
an issue on ARM anyway). It then adds back the ARM-specific ones.

This follows the sacred rules of good code:

 - generic code is either generic or not. If it's not generic, don't claim 
   it is.

 - don't *force* people to use generic code if it doesn't suit them. Make 
   it available for the cases it makes sense for, but don't shoe-horn it 
   into cases where it doesn't work well.

So it allows the sharing of the common case and *many* architectures end 
up using the generic Kconfig file, but hey, if it doesn't make sense for 
ARM, it doesn't make sense for ARM. It's that simple.

But as mentioned, it's totally untested and I don't have (or really want 
to have) a cross-compiling environment. And I don't care *that* much. I 
just want something we can all live with.

So does something like this work for people?

		Linus

---
 arch/arm/Kconfig                 |    2 +-
 arch/arm/Kconfig.instrumentation |   52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c4de2d4..3a75a0b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1076,7 +1076,7 @@ endmenu
 
 source "fs/Kconfig"
 
-source "kernel/Kconfig.instrumentation"
+source "arch/arm/Kconfig.instrumentation"
 
 source "arch/arm/Kconfig.debug"
 
diff --git a/arch/arm/Kconfig.instrumentation b/arch/arm/Kconfig.instrumentation
new file mode 100644
index 0000000..63b8c6d
--- /dev/null
+++ b/arch/arm/Kconfig.instrumentation
@@ -0,0 +1,52 @@
+menuconfig INSTRUMENTATION
+	bool "Instrumentation Support"
+	default y
+	---help---
+	  Say Y here to get to see options related to performance measurement,
+	  system-wide debugging, and testing. This option alone does not add any
+	  kernel code.
+
+	  If you say N, all options in this submenu will be skipped and
+	  disabled. If you're trying to debug the kernel itself, go see the
+	  Kernel Hacking menu.
+
+if INSTRUMENTATION
+
+config PROFILING
+	bool "Profiling support (EXPERIMENTAL)"
+	help
+	  Say Y here to enable the extended profiling support mechanisms used
+	  by profilers such as OProfile.
+
+config OPROFILE
+	tristate "OProfile system profiling (EXPERIMENTAL)"
+	depends on PROFILING && !UML
+	help
+	  OProfile is a profiling system capable of profiling the
+	  whole system, include the kernel, kernel modules, libraries,
+	  and applications.
+
+	  If unsure, say N.
+
+config OPROFILE_ARMV6
+	bool
+	depends on OPROFILE && CPU_V6 && !SMP
+	default y
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_MPCORE
+	bool
+	depends on OPROFILE && CPU_V6 && SMP
+	default y
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_ARM11_CORE
+	bool
+
+config MARKERS
+	bool "Activate markers"
+	help
+	  Place an empty function call at each marker site. Can be
+	  dynamically changed for a probe function.
+
+endif # INSTRUMENTATION

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> But as mentioned, it's totally untested and I don't have (or really 
> want to have) a cross-compiling environment. And I don't care *that* 
> much. I just want something we can all live with.
> 
> So does something like this work for people?

>  arch/arm/Kconfig                 |    2 +-
>  arch/arm/Kconfig.instrumentation |   52 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+), 1 deletions(-)

i like this approach better, not the least because it affects only one 
architecture so late in the .24-rc cycle. Instrumentation bugs tend to 
be found with a few weeks/months of delays. (because historically 
instrumentation has been typically used by folks who cannot change their 
kernel easily to debug it, i.e. by extension they dont run bleeding edge 
kernels either.)

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Russell King]

On Tue, Jan 15, 2008 at 08:24:34AM -0800, Linus Torvalds wrote:
> So does something like this work for people?

Just tested it and the right Kconfig symbols get set again - thanks.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Mathieu Desnoyers]

* Linus Torvalds (torvalds@linux-foundation.org) wrote:
> 
> 
> On Tue, 15 Jan 2008, Russell King wrote:
> > 
> > I don't particularly like stuffing the options into some random place
> > in the architectures Kconfig file when they should stay along side the
> > instrumentation configuration entries.
> 
> Well, I have to say that I don't particularly like obviously 
> architecture-specific stuff in an obviously non-architecture file..
> 
> I'd almost prefer to revert the thing that caused the problem, because 
> with Adrian's patch, I think the end result may *work*, but it's uglier 
> than what we started out with.
> 
> However, I think the *cleanest* solution right now may be something like 
> the appended. Totally untested, of course. It basically just copies the 
> generic kernel/Kconfig.instrumentation file into the arm directory, makes 
> arm use its own instead of the generic one, and removes the dependencies 
> on ARM in there (including all of the KPROBES entry that apparently isn't 
> an issue on ARM anyway). It then adds back the ARM-specific ones.
> 
> This follows the sacred rules of good code:
> 
>  - generic code is either generic or not. If it's not generic, don't claim 
>    it is.
> 
>  - don't *force* people to use generic code if it doesn't suit them. Make 
>    it available for the cases it makes sense for, but don't shoe-horn it 
>    into cases where it doesn't work well.
> 
> So it allows the sharing of the common case and *many* architectures end 
> up using the generic Kconfig file, but hey, if it doesn't make sense for 
> ARM, it doesn't make sense for ARM. It's that simple.
> 
> But as mentioned, it's totally untested and I don't have (or really want 
> to have) a cross-compiling environment. And I don't care *that* much. I 
> just want something we can all live with.
> 
> So does something like this work for people?
> 

Hi,

Well, it goes along the lines of the patch I suggested as a reply to
Adrian, with these differences :

- I still source the kernel/Kconfig.instrumentation file.
- I put back the missing OPROFILE options directly in arch/arm/Kconfig

Then end result is the same as your patch, but without the code
duplication.

Here is the patch :


Fix ARMv6 oprofile support

This patch restores the ARMv6 OProfile support that was killed by
commit 09cadedbdc01f1a4bea1f427d4fb4642eaa19da9.

It puts the config options in arch/arm/Kconfig.

Thanks to Adrian Bunk for finding this bug and providing an initial
patch.

Changelog :
Use def_bool.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
CC: Adrian Bunk <adrian.bunk@movial.fi>
CC: Randy Dunlap <randy.dunlap@oracle.com>
CC: rmk@arm.linux.org.uk
CC: phil.el@wanadoo.fr
CC: oprofile-list@lists.sourceforge.net
---
 arch/arm/Kconfig |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-2.6-lttng/arch/arm/Kconfig
===================================================================
--- linux-2.6-lttng.orig/arch/arm/Kconfig	2007-12-29 16:58:32.000000000 -0500
+++ linux-2.6-lttng/arch/arm/Kconfig	2007-12-29 16:59:25.000000000 -0500
@@ -130,6 +130,23 @@ config FIQ
 config ARCH_MTD_XIP
 	bool
 
+if OPROFILE
+
+config OPROFILE_ARMV6
+	def_bool y
+	depends on CPU_V6 && !SMP
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_MPCORE
+	def_bool y
+	depends on CPU_V6 && SMP
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_ARM11_CORE
+	bool
+
+endif
+
 config VECTORS_BASE
 	hex
 	default 0xffff0000 if MMU || CPU_HIGH_VECTOR


- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Linus Torvalds]

On Tue, 15 Jan 2008, Mathieu Desnoyers wrote:
> 
> Well, it goes along the lines of the patch I suggested as a reply to
> Adrian, with these differences :
> 
> - I still source the kernel/Kconfig.instrumentation file.
> - I put back the missing OPROFILE options directly in arch/arm/Kconfig
> 
> Then end result is the same as your patch, but without the code
> duplication.

No it's not.

Now the config variables may all be there, but the UI for the *menu* 
system is broken (ie all the ARM profiling config options are now outside 
the profiling menu).

Is that menu really needed? I dunno. But since it exists, it should be 
correct.

		Linus

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Mathieu Desnoyers]

* Linus Torvalds (torvalds@linux-foundation.org) wrote:
> 
> 
> On Tue, 15 Jan 2008, Mathieu Desnoyers wrote:
> > 
> > Well, it goes along the lines of the patch I suggested as a reply to
> > Adrian, with these differences :
> > 
> > - I still source the kernel/Kconfig.instrumentation file.
> > - I put back the missing OPROFILE options directly in arch/arm/Kconfig
> > 
> > Then end result is the same as your patch, but without the code
> > duplication.
> 
> No it's not.
> 
> Now the config variables may all be there, but the UI for the *menu* 
> system is broken (ie all the ARM profiling config options are now outside 
> the profiling menu).
> 
> Is that menu really needed? I dunno. But since it exists, it should be 
> correct.
> 
> 		Linus

There is an "instrumentation menu removal" patchset I've submitted to
Andrew for the next release cycle that moves the instrumentation menu
content into General setup (I did this following your advice).

Furthermore, on ARM, the OPROFILE_ARMV6, OPROFILE_MPCORE and
OPROFILE_ARM11_CORE are all "bool , default y" (equivalent to the
preferred def_bool y). Unless I am grossly mistaken, this is not
supposed to show up in the menus; it's just selected when the
dependencies are met.

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Sam Ravnborg]

On Tue, Jan 15, 2008 at 02:07:20PM -0500, Mathieu Desnoyers wrote:
> * Linus Torvalds (torvalds@linux-foundation.org) wrote:
> > 
> > 
> > On Tue, 15 Jan 2008, Mathieu Desnoyers wrote:
> > > 
> > > Well, it goes along the lines of the patch I suggested as a reply to
> > > Adrian, with these differences :
> > > 
> > > - I still source the kernel/Kconfig.instrumentation file.
> > > - I put back the missing OPROFILE options directly in arch/arm/Kconfig
> > > 
> > > Then end result is the same as your patch, but without the code
> > > duplication.
> > 
> > No it's not.
> > 
> > Now the config variables may all be there, but the UI for the *menu* 
> > system is broken (ie all the ARM profiling config options are now outside 
> > the profiling menu).
> > 
> > Is that menu really needed? I dunno. But since it exists, it should be 
> > correct.
> > 
> > 		Linus
> 
> There is an "instrumentation menu removal" patchset I've submitted to
> Andrew for the next release cycle that moves the instrumentation menu
> content into General setup (I did this following your advice).
> 
> Furthermore, on ARM, the OPROFILE_ARMV6, OPROFILE_MPCORE and
> OPROFILE_ARM11_CORE are all "bool , default y" (equivalent to the
> preferred def_bool y). Unless I am grossly mistaken, this is not
> supposed to show up in the menus; it's just selected when the
> dependencies are met.

This was the patch:
+if OPROFILE
+
+config OPROFILE_ARMV6
+	def_bool y
+	depends on CPU_V6 && !SMP
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_MPCORE
+	def_bool y
+	depends on CPU_V6 && SMP
+	select OPROFILE_ARM11_CORE
+
+config OPROFILE_ARM11_CORE
+	bool
+
+endif

And none of these has a prompt defined so they do not show up
as a menu.
It is very easy to test if you have the patch applied.
No cross toolchin is needed - just do:
make ARCH=arm menuconfig

	Sam

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Mathieu Desnoyers]

* Sam Ravnborg (sam@ravnborg.org) wrote:
> On Tue, Jan 15, 2008 at 02:07:20PM -0500, Mathieu Desnoyers wrote:
> > * Linus Torvalds (torvalds@linux-foundation.org) wrote:
> > > 
> > > 
> > > On Tue, 15 Jan 2008, Mathieu Desnoyers wrote:
> > > > 
> > > > Well, it goes along the lines of the patch I suggested as a reply to
> > > > Adrian, with these differences :
> > > > 
> > > > - I still source the kernel/Kconfig.instrumentation file.
> > > > - I put back the missing OPROFILE options directly in arch/arm/Kconfig
> > > > 
> > > > Then end result is the same as your patch, but without the code
> > > > duplication.
> > > 
> > > No it's not.
> > > 
> > > Now the config variables may all be there, but the UI for the *menu* 
> > > system is broken (ie all the ARM profiling config options are now outside 
> > > the profiling menu).
> > > 
> > > Is that menu really needed? I dunno. But since it exists, it should be 
> > > correct.
> > > 
> > > 		Linus
> > 
> > There is an "instrumentation menu removal" patchset I've submitted to
> > Andrew for the next release cycle that moves the instrumentation menu
> > content into General setup (I did this following your advice).
> > 
> > Furthermore, on ARM, the OPROFILE_ARMV6, OPROFILE_MPCORE and
> > OPROFILE_ARM11_CORE are all "bool , default y" (equivalent to the
> > preferred def_bool y). Unless I am grossly mistaken, this is not
> > supposed to show up in the menus; it's just selected when the
> > dependencies are met.
> 
> This was the patch:
> +if OPROFILE
> +
> +config OPROFILE_ARMV6
> +	def_bool y
> +	depends on CPU_V6 && !SMP
> +	select OPROFILE_ARM11_CORE
> +
> +config OPROFILE_MPCORE
> +	def_bool y
> +	depends on CPU_V6 && SMP
> +	select OPROFILE_ARM11_CORE
> +
> +config OPROFILE_ARM11_CORE
> +	bool
> +
> +endif
> 
> And none of these has a prompt defined so they do not show up
> as a menu.
> It is very easy to test if you have the patch applied.
> No cross toolchin is needed - just do:
> make ARCH=arm menuconfig
> 
> 	Sam

Just tested with and without oprofile/MPCORE/CPU_V6 combinations and I
confirm that :

- no menu is showing up (as expected)
- the OPROFILE_* config options are selected (or not) as expected

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Linus Torvalds]

On Tue, 15 Jan 2008, Mathieu Desnoyers wrote:
> 
> Furthermore, on ARM, the OPROFILE_ARMV6, OPROFILE_MPCORE and
> OPROFILE_ARM11_CORE are all "bool , default y" (equivalent to the
> preferred def_bool y).

Ahh, I didn't notice that. If so, yes, it doesn't matter at all whether 
it's inside the menu or not, of course.

		Linus

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Russell King]

On Tue, Jan 15, 2008 at 08:24:34AM -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 15 Jan 2008, Russell King wrote:
> > 
> > I don't particularly like stuffing the options into some random place
> > in the architectures Kconfig file when they should stay along side the
> > instrumentation configuration entries.
> 
> Well, I have to say that I don't particularly like obviously 
> architecture-specific stuff in an obviously non-architecture file..
> 
> I'd almost prefer to revert the thing that caused the problem, because 
> with Adrian's patch, I think the end result may *work*, but it's uglier 
> than what we started out with.
> 
> However, I think the *cleanest* solution right now may be something like 
> the appended. Totally untested, of course. It basically just copies the 
> generic kernel/Kconfig.instrumentation file into the arm directory, makes 
> arm use its own instead of the generic one, and removes the dependencies 
> on ARM in there (including all of the KPROBES entry that apparently isn't 
> an issue on ARM anyway). It then adds back the ARM-specific ones.
> 
> This follows the sacred rules of good code:
> 
>  - generic code is either generic or not. If it's not generic, don't claim 
>    it is.
> 
>  - don't *force* people to use generic code if it doesn't suit them. Make 
>    it available for the cases it makes sense for, but don't shoe-horn it 
>    into cases where it doesn't work well.
> 
> So it allows the sharing of the common case and *many* architectures end 
> up using the generic Kconfig file, but hey, if it doesn't make sense for 
> ARM, it doesn't make sense for ARM. It's that simple.
> 
> But as mentioned, it's totally untested and I don't have (or really want 
> to have) a cross-compiling environment. And I don't care *that* much. I 
> just want something we can all live with.
> 
> So does something like this work for people?

BTW, your patch may fix ARM, but the original commit broke blackfin as
well - it removed the Kconfig entry for HARDWARE_PM, which is still used:

$ grep HARDWARE_PM arch/blackfin/ -r
arch/blackfin/mach-common/interrupt.S:#ifdef CONFIG_HARDWARE_PM
arch/blackfin/mach-common/interrupt.S:#ifdef CONFIG_HARDWARE_PM
arch/blackfin/mach-common/irqpanic.c:#ifdef CONFIG_HARDWARE_PM
arch/blackfin/oprofile/Makefile:oprofile-$(CONFIG_HARDWARE_PM) += op_model_bf533.o
arch/blackfin/oprofile/common.c:#ifdef CONFIG_HARDWARE_PM

So blackfin also needs fixing.  I don't know if blackfin people are
aware of this.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: Fwd: Re: [2.6.24 patch] restore ARMv6 OProfile support

[Adrian Bunk]

On Tue, Jan 15, 2008 at 05:49:48PM +0000, Russell King wrote:
>...
> So blackfin also needs fixing.  I don't know if blackfin people are
> aware of this.

They are:
  http://lkml.org/lkml/2007/12/28/100

> Russell King

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed