[PATCH v4 0/4] tools: Add a toplevel Makefile

[PATCH v4 0/4] tools: Add a toplevel Makefile

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Hi all,

this is basically a new rediff of the patchset against 3.4-rc2 along
with integrating all comments/reviews I got from the last iteration. If
there are none this time, I would like to suggest merging this, maybe
through -tip or Arnaldo's perf tree.

State of the affairs on how to use it are in the 4/4 patch, adding them
here too:

Now you can do

$ make tools/<toolname>

from the toplevel kernel directory and have the respective tool built.

If you want to build and install it, do

$ make tools/<toolname>_install

$ make tools/<toolname>_clean

should clean the respective tool directories.

If you want to clean all in tools, simply do

$ make tools/clean

Also, if you want to get what the possible targets are, simply calling

$ make tools/

should give you the short help.

$ make tools/install

installs all tools, of course. Doh.

Thanks.

Changelog:
==========

* v3:

Yet another version of the toplevel Makefile integration of tools/.
This round gives you the ability to build the tools from the toplevel
Makefile (explanation below can be found also in patch 4/4's commit
message):

"Now you can do

$ make tools/<toolname>

from the toplevel kernel directory and have the respective tool built.

If you want to build and install it, do

$ make tools/<toolname> tinstall

The install target is called "tinstall" so that there's no conflict with
the main kernel install target and should mean "tool install".

$ make tools/ <toolname>_clean

should clean the respective tool directories.

If you want to clean all in tools, simply do

$ make  tools/ cleanall

Also, if you want to get what the possible targets are, simply calling

$ make tools/

should give you the short help."

Also included are all suggestions from the last time.

Thanks.

* v2:

here's a refreshed version from yesterday incorporating all comments and
suggestions along with a third patch that adds a 'help' target as the
default one causing the following below. Btw, Arnaldo, could you please
pick those up if there are no complaints since the first patch touches
perf and I don't have a clear idea who else to send it to anyway :).

Thanks.

$ make
Possible targets:

  cpupower   - a tool for all things x86 CPU power
  firewire   - the userspace part of nosy, an IEEE-1394 traffic sniffer
  lguest     - a minimal 32-bit x86 hypervisor
  perf       - Linux performance measurements tool
  slub       - slabs reporting tool
  turbostat  - Intel CPU idle stats and freq reporting tool
  usb        - USB testing tools
  virtio     - vhost test module
  x86_energy_perf_policy - Intel energy policy tool

Cleaning targets:

  all of the above with the "_clean" string appended cleans
    the respective build directory.
  clean: a summary clean target to clean _all_ folders


* v1:

this is a refresh and carve-out of an old patchset. It adds a toplevel
Makefile to tools/ so that one can build the tool of her/his liking by
simply doing

$ cd tools/
$ make <toolname>

By default, we build perf. There's also a scripts/Makefile.lib now which
should contain all make-related generic stuff which can be used by all
tools' build process after including this file.

</Changelog>

Any comments/suggestions are welcome,
thanks.

[PATCH v4 1/4] tools: Add Makefile.include

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Put generic enough build settings which could be reused by other tools
into a common Makefile.include file.

This commit reintroduces QUIET_SUBDIR{0,1} (see a3d1ee10d1bf) which are
going to be used in the following patches.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 tools/perf/Makefile            |   47 +--------------------------------
 tools/scripts/Makefile.include |   57 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 46 deletions(-)
 create mode 100644 tools/scripts/Makefile.include

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 820371f10d1b..4ca77cc0f284 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -1,18 +1,10 @@
-ifeq ("$(origin O)", "command line")
-	OUTPUT := $(O)/
-endif
+include ../scripts/Makefile.include
 
 # The default target of this Makefile is...
 all:
 
 include config/utilities.mak
 
-ifneq ($(OUTPUT),)
-# check that the output directory actually exists
-OUTDIR := $(shell cd $(OUTPUT) && /bin/pwd)
-$(if $(OUTDIR),, $(error output directory "$(OUTPUT)" does not exist))
-endif
-
 # Define V to have a more verbose compile.
 #
 # Define O to save output files in a separate directory.
@@ -84,31 +76,6 @@ ifneq ($(WERROR),0)
 	CFLAGS_WERROR := -Werror
 endif
 
-#
-# Include saner warnings here, which can catch bugs:
-#
-
-EXTRA_WARNINGS := -Wformat
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wformat-security
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wformat-y2k
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wshadow
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Winit-self
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wpacked
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wredundant-decls
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wstrict-aliasing=3
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wswitch-default
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wswitch-enum
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wno-system-headers
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wundef
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wwrite-strings
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wbad-function-cast
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wmissing-declarations
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wmissing-prototypes
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wnested-externs
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wold-style-definition
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wstrict-prototypes
-EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wdeclaration-after-statement
-
 ifeq ("$(origin DEBUG)", "command line")
   PERF_DEBUG = $(DEBUG)
 endif
@@ -679,18 +646,6 @@ else
 	endif
 endif
 
-ifneq ($(findstring $(MAKEFLAGS),s),s)
-ifndef V
-	QUIET_CC       = @echo '   ' CC $@;
-	QUIET_AR       = @echo '   ' AR $@;
-	QUIET_LINK     = @echo '   ' LINK $@;
-	QUIET_MKDIR    = @echo '   ' MKDIR $@;
-	QUIET_GEN      = @echo '   ' GEN $@;
-	QUIET_FLEX     = @echo '   ' FLEX $@;
-	QUIET_BISON    = @echo '   ' BISON $@;
-endif
-endif
-
 ifdef ASCIIDOC8
 	export ASCIIDOC8
 endif
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
new file mode 100644
index 000000000000..52348d3bd8d3
--- /dev/null
+++ b/tools/scripts/Makefile.include
@@ -0,0 +1,57 @@
+ifeq ("$(origin O)", "command line")
+	OUTPUT := $(O)/
+endif
+
+ifneq ($(OUTPUT),)
+# check that the output directory actually exists
+OUTDIR := $(shell cd $(OUTPUT) && /bin/pwd)
+$(if $(OUTDIR),, $(error output directory "$(OUTPUT)" does not exist))
+endif
+
+#
+# Include saner warnings here, which can catch bugs:
+#
+EXTRA_WARNINGS := -Wformat
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wformat-security
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wformat-y2k
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wshadow
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Winit-self
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wpacked
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wredundant-decls
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wstrict-aliasing=3
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wswitch-default
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wswitch-enum
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wno-system-headers
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wundef
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wwrite-strings
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wbad-function-cast
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wmissing-declarations
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wmissing-prototypes
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wnested-externs
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wold-style-definition
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wstrict-prototypes
+EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wdeclaration-after-statement
+
+ifneq ($(findstring $(MAKEFLAGS), w),w)
+PRINT_DIR = --no-print-directory
+else
+NO_SUBDIR = :
+endif
+
+QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
+QUIET_SUBDIR1  =
+
+ifneq ($(findstring $(MAKEFLAGS),s),s)
+ifndef V
+	QUIET_CC       = @echo '   ' CC $@;
+	QUIET_AR       = @echo '   ' AR $@;
+	QUIET_LINK     = @echo '   ' LINK $@;
+	QUIET_MKDIR    = @echo '   ' MKDIR $@;
+	QUIET_GEN      = @echo '   ' GEN $@;
+	QUIET_SUBDIR0  = +@subdir=
+	QUIET_SUBDIR1  = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
+			 $(MAKE) $(PRINT_DIR) -C $$subdir
+	QUIET_FLEX     = @echo '   ' FLEX $@;
+	QUIET_BISON    = @echo '   ' BISON $@;
+endif
+endif
-- 
1.7.9.3.362.g71319

[PATCH v4 2/4] tools: Add a toplevel Makefile

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Add a Makefile with all the targets under tools/.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 tools/Makefile |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 tools/Makefile

diff --git a/tools/Makefile b/tools/Makefile
new file mode 100644
index 000000000000..e93b8ec3d250
--- /dev/null
+++ b/tools/Makefile
@@ -0,0 +1,31 @@
+include scripts/Makefile.include
+
+perf firewire lguest usb virtio vm: FORCE
+	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)
+
+cpupower: FORCE
+	$(QUIET_SUBDIR0)power/$@/ $(QUIET_SUBDIR1)
+
+turbostat x86_energy_perf_policy: FORCE
+	$(QUIET_SUBDIR0)power/x86/$@/ $(QUIET_SUBDIR1)
+
+selftests: FORCE
+	$(QUIET_SUBDIR0)testing/$@/ $(QUIET_SUBDIR1)
+
+firewire_clean lguest_clean perf_clean vm_clean usb_clean virtio_clean:
+	$(QUIET_SUBDIR0)$(@:_clean=)/ $(QUIET_SUBDIR1) clean
+
+cp_clean:
+	$(QUIET_SUBDIR0)power/cpupower/ $(QUIET_SUBDIR1) clean
+
+turbostat_clean x86_energy_perf_policy_clean:
+	$(QUIET_SUBDIR0)power/x86/$(@:_clean=)/ $(QUIET_SUBDIR1) clean
+
+selftests_clean:
+	$(QUIET_SUBDIR0)testing/$(@:_clean=)/ $(QUIET_SUBDIR1) clean
+
+clean: cp_clean firewire_clean lguest_clean perf_clean selftests_clean \
+		turbostat_clean usb_clean virtio_clean \
+		x86_energy_perf_policy_clean
+
+.PHONY: FORCE
-- 
1.7.9.3.362.g71319

[PATCH v4 3/4] tools: Add a help target

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

... and make it the default one so that calling 'make' without arguments
in the tools/ directory gives you the possible targets to build along
with a short description of what they are.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 tools/Makefile |   26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/tools/Makefile b/tools/Makefile
index e93b8ec3d250..c7ed814e12cf 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -1,5 +1,25 @@
 include scripts/Makefile.include
 
+help:
+	@echo 'Possible targets:'
+	@echo ''
+	@echo '  cpupower   - a tool for all things x86 CPU power'
+	@echo '  firewire   - the userspace part of nosy, an IEEE-1394 traffic sniffer'
+	@echo '  lguest     - a minimal 32-bit x86 hypervisor'
+	@echo '  perf       - Linux performance measurement and analysis tool'
+	@echo '  selftests  - various kernel selftests'
+	@echo '  turbostat  - Intel CPU idle stats and freq reporting tool'
+	@echo '  usb        - USB testing tools'
+	@echo '  virtio     - vhost test module'
+	@echo '  vm         - misc vm tools'
+	@echo '  x86_energy_perf_policy - Intel energy policy tool'
+	@echo ''
+	@echo 'Cleaning targets:'
+	@echo ''
+	@echo '  all of the above with the "_clean" string appended cleans'
+	@echo '    the respective build directory.'
+	@echo '  clean: a summary clean target to clean _all_ folders'
+
 perf firewire lguest usb virtio vm: FORCE
 	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)
 
@@ -15,7 +35,7 @@ selftests: FORCE
 firewire_clean lguest_clean perf_clean vm_clean usb_clean virtio_clean:
 	$(QUIET_SUBDIR0)$(@:_clean=)/ $(QUIET_SUBDIR1) clean
 
-cp_clean:
+cpupower_clean:
 	$(QUIET_SUBDIR0)power/cpupower/ $(QUIET_SUBDIR1) clean
 
 turbostat_clean x86_energy_perf_policy_clean:
@@ -24,8 +44,8 @@ turbostat_clean x86_energy_perf_policy_clean:
 selftests_clean:
 	$(QUIET_SUBDIR0)testing/$(@:_clean=)/ $(QUIET_SUBDIR1) clean
 
-clean: cp_clean firewire_clean lguest_clean perf_clean selftests_clean \
-		turbostat_clean usb_clean virtio_clean \
+clean: cpupower_clean firewire_clean lguest_clean perf_clean \
+		selftests_clean turbostat_clean usb_clean virtio_clean \
 		x86_energy_perf_policy_clean
 
 .PHONY: FORCE
-- 
1.7.9.3.362.g71319

[PATCH v4 4/4] tools: Connect to the kernel build system

[Borislav Petkov]

From: Borislav Petkov <borislav.petkov@amd.com>

Now you can do

$ make tools/<toolname>

from the toplevel kernel directory and have the respective tool built.

If you want to build and install it, do

$ make tools/<toolname>_install

$ make tools/<toolname>_clean

should clean the respective tool directories.

If you want to clean all in tools, simply do

$ make tools/clean

Also, if you want to get what the possible targets are, simply calling

$ make tools/

should give you the short help.

$ make tools/install

installs all tools, of course. Doh.

Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 Makefile       |    7 +++++++
 tools/Makefile |   25 ++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 0df3d003a079..e24ce7104f63 100644
--- a/Makefile
+++ b/Makefile
@@ -1468,6 +1468,13 @@ kernelrelease:
 kernelversion:
 	@echo $(KERNELVERSION)
 
+# Clear a bunch of variables before executing the submake
+tools/: FORCE
+	$(Q)$(MAKE) LDFLAGS= -C $(src)/tools/
+
+tools/%: FORCE
+	$(Q)$(MAKE) LDFLAGS= MAKEFLAGS= -C $(src)/tools/ $*
+
 # Single targets
 # ---------------------------------------------------------------------------
 # Single targets are compatible with:
diff --git a/tools/Makefile b/tools/Makefile
index c7ed814e12cf..b2f3bd567ce3 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -14,13 +14,23 @@ help:
 	@echo '  vm         - misc vm tools'
 	@echo '  x86_energy_perf_policy - Intel energy policy tool'
 	@echo ''
+	@echo 'You can do:'
+	@echo ' $$ make -C tools/<tool>_install'
+	@echo ''
+	@echo '  from the kernel command line to build and install one of'
+	@echo '  the tools above'
+	@echo ''
+	@echo '  $$ make tools/install'
+	@echo ''
+	@echo '  installs all tools.'
+	@echo ''
 	@echo 'Cleaning targets:'
 	@echo ''
 	@echo '  all of the above with the "_clean" string appended cleans'
 	@echo '    the respective build directory.'
 	@echo '  clean: a summary clean target to clean _all_ folders'
 
-perf firewire lguest usb virtio vm: FORCE
+firewire lguest perf usb virtio vm: FORCE
 	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)
 
 cpupower: FORCE
@@ -32,6 +42,19 @@ turbostat x86_energy_perf_policy: FORCE
 selftests: FORCE
 	$(QUIET_SUBDIR0)testing/$@/ $(QUIET_SUBDIR1)
 
+firewire_install lguest_install perf_install usb_install virtio_install vm_install:
+	$(QUIET_SUBDIR0)$(@:_install=)/ $(QUIET_SUBDIR1) install
+
+cpupower_install:
+	$(QUIET_SUBDIR0)power/$(@:_install=)/ $(QUIET_SUBDIR1) install
+
+turbostat_install x86_energy_perf_policy_install:
+	$(QUIET_SUBDIR0)power/x86/$(@:_install=)/ $(QUIET_SUBDIR1) install
+
+install: firewire_install lguest_install perf_install usb_install \
+		virtio_install vm_install cpupower_install turbostat_install \
+		x86_energy_perf_policy_install
+
 firewire_clean lguest_clean perf_clean vm_clean usb_clean virtio_clean:
 	$(QUIET_SUBDIR0)$(@:_clean=)/ $(QUIET_SUBDIR1) clean
 
-- 
1.7.9.3.362.g71319

Re: [PATCH v4 1/4] tools: Add Makefile.include

[Sam Ravnborg]

Hi Borislav.

Some random comments as I really did not look at this
part of the patch-set before.

> +#
> +# Include saner warnings here, which can catch bugs:
> +#
> +EXTRA_WARNINGS := -Wformat
> +EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wformat-security

Two general comments - and I know this is just something you copied...
Why not use the += operator. The below looks like shell script syntax.
And we use the += operator in other places - at least in the kernel stuff.

AND WHY ALL THESE SCREAMING CAPITAL LETTERS?
I know Makefiles and scripts are full of them - but that does not help
the readability.

For kbuild I generally shifted to use:
- lower-case names for local stuff
- Upper case letters for global stuff - properly prefixed to avoid collisions
EXTRA_WARNINGS likely fall into the category global-stuff,
but then a local variable could still be usefull.

	Sam

Re: [PATCH v4 2/4] tools: Add a toplevel Makefile

[Sam Ravnborg]

On Tue, Apr 10, 2012 at 05:20:38PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Add a Makefile with all the targets under tools/.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  tools/Makefile |   31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 tools/Makefile
> 
> diff --git a/tools/Makefile b/tools/Makefile
> new file mode 100644
> index 000000000000..e93b8ec3d250
> --- /dev/null
> +++ b/tools/Makefile
> @@ -0,0 +1,31 @@
> +include scripts/Makefile.include
> +
> +perf firewire lguest usb virtio vm: FORCE
> +	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)
> +
> +cpupower: FORCE
> +	$(QUIET_SUBDIR0)power/$@/ $(QUIET_SUBDIR1)
> +
> +turbostat x86_energy_perf_policy: FORCE
> +	$(QUIET_SUBDIR0)power/x86/$@/ $(QUIET_SUBDIR1)
> +
> +selftests: FORCE
> +	$(QUIET_SUBDIR0)testing/$@/ $(QUIET_SUBDIR1)
> +
> +firewire_clean lguest_clean perf_clean vm_clean usb_clean virtio_clean:
> +	$(QUIET_SUBDIR0)$(@:_clean=)/ $(QUIET_SUBDIR1) clean
> +
> +cp_clean:
> +	$(QUIET_SUBDIR0)power/cpupower/ $(QUIET_SUBDIR1) clean
> +
> +turbostat_clean x86_energy_perf_policy_clean:
> +	$(QUIET_SUBDIR0)power/x86/$(@:_clean=)/ $(QUIET_SUBDIR1) clean
> +
> +selftests_clean:
> +	$(QUIET_SUBDIR0)testing/$(@:_clean=)/ $(QUIET_SUBDIR1) clean
> +
> +clean: cp_clean firewire_clean lguest_clean perf_clean selftests_clean \
> +		turbostat_clean usb_clean virtio_clean \
> +		x86_energy_perf_policy_clean
> +
> +.PHONY: FORCE

Things would be much simpler is all tools were adjusted to:
a) the subdir they live in is the make target name
b) each tool has three mandatory targets:
 1) default target => build the tool
 2) clean
 3) install

I can see that with the layout today you cannot
distingush between the various targets below power.
But the current solutions is not scaleable nor maintainable.

Even if you cannot fix power/* then testing looks easy.

But all that can always to improved and consolidated later.

	Sam

Re: [PATCH v4 3/4] tools: Add a help target

[Sam Ravnborg]

On Tue, Apr 10, 2012 at 05:20:39PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> ... and make it the default one so that calling 'make' without arguments
> in the tools/ directory gives you the possible targets to build along
> with a short description of what they are.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  tools/Makefile |   26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index e93b8ec3d250..c7ed814e12cf 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -1,5 +1,25 @@
>  include scripts/Makefile.include
>  
> +help:
> +	@echo 'Possible targets:'
> +	@echo ''
> +	@echo '  cpupower   - a tool for all things x86 CPU power'
> +	@echo '  firewire   - the userspace part of nosy, an IEEE-1394 traffic sniffer'
> +	@echo '  lguest     - a minimal 32-bit x86 hypervisor'
> +	@echo '  perf       - Linux performance measurement and analysis tool'
> +	@echo '  selftests  - various kernel selftests'
> +	@echo '  turbostat  - Intel CPU idle stats and freq reporting tool'
> +	@echo '  usb        - USB testing tools'
> +	@echo '  virtio     - vhost test module'
> +	@echo '  vm         - misc vm tools'
> +	@echo '  x86_energy_perf_policy - Intel energy policy tool'
> +	@echo ''
> +	@echo 'Cleaning targets:'
> +	@echo ''
> +	@echo '  all of the above with the "_clean" string appended cleans'
> +	@echo '    the respective build directory.'
> +	@echo '  clean: a summary clean target to clean _all_ folders'
> +
>  perf firewire lguest usb virtio vm: FORCE
>  	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)
>  
> @@ -15,7 +35,7 @@ selftests: FORCE
>  firewire_clean lguest_clean perf_clean vm_clean usb_clean virtio_clean:
>  	$(QUIET_SUBDIR0)$(@:_clean=)/ $(QUIET_SUBDIR1) clean
>  
> -cp_clean:
> +cpupower_clean:
>  	$(QUIET_SUBDIR0)power/cpupower/ $(QUIET_SUBDIR1) clean

Fold this into 2/4

>  
>  turbostat_clean x86_energy_perf_policy_clean:
> @@ -24,8 +44,8 @@ turbostat_clean x86_energy_perf_policy_clean:
>  selftests_clean:
>  	$(QUIET_SUBDIR0)testing/$(@:_clean=)/ $(QUIET_SUBDIR1) clean
>  
> -clean: cp_clean firewire_clean lguest_clean perf_clean selftests_clean \
> -		turbostat_clean usb_clean virtio_clean \
> +clean: cpupower_clean firewire_clean lguest_clean perf_clean \
> +		selftests_clean turbostat_clean usb_clean virtio_clean \
>  		x86_energy_perf_policy_clean
Fold this into 2/4


	Sam

Re: [PATCH v4 4/4] tools: Connect to the kernel build system

[Sam Ravnborg]

On Tue, Apr 10, 2012 at 05:20:40PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <borislav.petkov@amd.com>
> 
> Now you can do
> 
> $ make tools/<toolname>
> 
> from the toplevel kernel directory and have the respective tool built.
> 
> If you want to build and install it, do
> 
> $ make tools/<toolname>_install
> 
> $ make tools/<toolname>_clean
> 
> should clean the respective tool directories.
> 
> If you want to clean all in tools, simply do
> 
> $ make tools/clean
> 
> Also, if you want to get what the possible targets are, simply calling
> 
> $ make tools/
> 
> should give you the short help.
> 
> $ make tools/install
> 
> installs all tools, of course. Doh.
> 
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  Makefile       |    7 +++++++
>  tools/Makefile |   25 ++++++++++++++++++++++++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 0df3d003a079..e24ce7104f63 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1468,6 +1468,13 @@ kernelrelease:
>  kernelversion:
>  	@echo $(KERNELVERSION)
>  
> +# Clear a bunch of variables before executing the submake
> +tools/: FORCE
> +	$(Q)$(MAKE) LDFLAGS= -C $(src)/tools/
I think you need to clear MAKEFLAGS here too?

> +
> +tools/%: FORCE
> +	$(Q)$(MAKE) LDFLAGS= MAKEFLAGS= -C $(src)/tools/ $*
> +

>  
> -perf firewire lguest usb virtio vm: FORCE
> +firewire lguest perf usb virtio vm: FORCE
>  	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)

Fold this into 2/4

>  
>  cpupower: FORCE
> @@ -32,6 +42,19 @@ turbostat x86_energy_perf_policy: FORCE
>  selftests: FORCE
>  	$(QUIET_SUBDIR0)testing/$@/ $(QUIET_SUBDIR1)
>  
> +firewire_install lguest_install perf_install usb_install virtio_install vm_install:
> +	$(QUIET_SUBDIR0)$(@:_install=)/ $(QUIET_SUBDIR1) install
> +
> +cpupower_install:
> +	$(QUIET_SUBDIR0)power/$(@:_install=)/ $(QUIET_SUBDIR1) install
> +
> +turbostat_install x86_energy_perf_policy_install:
> +	$(QUIET_SUBDIR0)power/x86/$(@:_install=)/ $(QUIET_SUBDIR1) install
> +
> +install: firewire_install lguest_install perf_install usb_install \
> +		virtio_install vm_install cpupower_install turbostat_install \
> +		x86_energy_perf_policy_install
> +

This really belongs in 2/4 or a separate patch.

Sorry for bing picky about these details - but they destroy
an otherwise fine patch-set.

	Sam

Re: [PATCH v4 1/4] tools: Add Makefile.include

[Borislav Petkov]

On Tue, Apr 10, 2012 at 10:27:21PM +0200, Sam Ravnborg wrote:
> Hi Borislav.
> 
> Some random comments as I really did not look at this
> part of the patch-set before.

.. and I appreciate those, thanks.

> > +#
> > +# Include saner warnings here, which can catch bugs:
> > +#
> > +EXTRA_WARNINGS := -Wformat
> > +EXTRA_WARNINGS := $(EXTRA_WARNINGS) -Wformat-security
> 
> Two general comments - and I know this is just something you copied...

Yep, I did from perf's Makefile.

> Why not use the += operator. The below looks like shell script syntax.
> And we use the += operator in other places - at least in the kernel stuff.

Yep, this makes this assignment monster a bit more readable, sure.

> AND WHY ALL THESE SCREAMING CAPITAL LETTERS?
> I know Makefiles and scripts are full of them - but that does not help
> the readability.
> 
> For kbuild I generally shifted to use:
> - lower-case names for local stuff
> - Upper case letters for global stuff - properly prefixed to avoid collisions
> EXTRA_WARNINGS likely fall into the category global-stuff,
> but then a local variable could still be usefull.

Well, my intention was to have EXTRA_WARNINGS be a global variable which
all tools under tools/ could start using so that we can benefit from the
compiler doing some more extensive checking. Thus all caps, no?

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v4 2/4] tools: Add a toplevel Makefile

[Borislav Petkov]

On Tue, Apr 10, 2012 at 10:33:48PM +0200, Sam Ravnborg wrote:
> Things would be much simpler is all tools were adjusted to:
> a) the subdir they live in is the make target name
> b) each tool has three mandatory targets:
>  1) default target => build the tool
>  2) clean
>  3) install
> 
> I can see that with the layout today you cannot
> distingush between the various targets below power.
> But the current solutions is not scaleable nor maintainable.
> 
> Even if you cannot fix power/* then testing looks easy.
> 
> But all that can always to improved and consolidated later.

Right, I was deliberating on whether to unify the dir layout so that it
fits with the Makefile targets nicely but decided to leave it out for
later. IOW, do the "one thing at a time" deal. Besides, some tools don't
have that clearly defined targets and selftests, for example, doesn't
need to be installed but simply run in a loop.

So definitely agreed, the tools/ dir could use a bunch of cultivating in
later cycles.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v4 4/4] tools: Connect to the kernel build system

[Borislav Petkov]

On Tue, Apr 10, 2012 at 10:37:33PM +0200, Sam Ravnborg wrote:
> On Tue, Apr 10, 2012 at 05:20:40PM +0200, Borislav Petkov wrote:
> > From: Borislav Petkov <borislav.petkov@amd.com>
> > 
> > Now you can do
> > 
> > $ make tools/<toolname>
> > 
> > from the toplevel kernel directory and have the respective tool built.
> > 
> > If you want to build and install it, do
> > 
> > $ make tools/<toolname>_install
> > 
> > $ make tools/<toolname>_clean
> > 
> > should clean the respective tool directories.
> > 
> > If you want to clean all in tools, simply do
> > 
> > $ make tools/clean
> > 
> > Also, if you want to get what the possible targets are, simply calling
> > 
> > $ make tools/
> > 
> > should give you the short help.
> > 
> > $ make tools/install
> > 
> > installs all tools, of course. Doh.
> > 
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> > ---
> >  Makefile       |    7 +++++++
> >  tools/Makefile |   25 ++++++++++++++++++++++++-
> >  2 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 0df3d003a079..e24ce7104f63 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1468,6 +1468,13 @@ kernelrelease:
> >  kernelversion:
> >  	@echo $(KERNELVERSION)
> >  
> > +# Clear a bunch of variables before executing the submake
> > +tools/: FORCE
> > +	$(Q)$(MAKE) LDFLAGS= -C $(src)/tools/
> I think you need to clear MAKEFLAGS here too?
> 
> > +
> > +tools/%: FORCE
> > +	$(Q)$(MAKE) LDFLAGS= MAKEFLAGS= -C $(src)/tools/ $*
> > +
> 
> >  
> > -perf firewire lguest usb virtio vm: FORCE
> > +firewire lguest perf usb virtio vm: FORCE
> >  	$(QUIET_SUBDIR0)$@/ $(QUIET_SUBDIR1)
> 
> Fold this into 2/4
> 
> >  
> >  cpupower: FORCE
> > @@ -32,6 +42,19 @@ turbostat x86_energy_perf_policy: FORCE
> >  selftests: FORCE
> >  	$(QUIET_SUBDIR0)testing/$@/ $(QUIET_SUBDIR1)
> >  
> > +firewire_install lguest_install perf_install usb_install virtio_install vm_install:
> > +	$(QUIET_SUBDIR0)$(@:_install=)/ $(QUIET_SUBDIR1) install
> > +
> > +cpupower_install:
> > +	$(QUIET_SUBDIR0)power/$(@:_install=)/ $(QUIET_SUBDIR1) install
> > +
> > +turbostat_install x86_energy_perf_policy_install:
> > +	$(QUIET_SUBDIR0)power/x86/$(@:_install=)/ $(QUIET_SUBDIR1) install
> > +
> > +install: firewire_install lguest_install perf_install usb_install \
> > +		virtio_install vm_install cpupower_install turbostat_install \
> > +		x86_energy_perf_policy_install
> > +
> 
> This really belongs in 2/4 or a separate patch.
> 
> Sorry for bing picky about these details - but they destroy
> an otherwise fine patch-set.

Agreed with all above, will rework soon.

Thanks for looking at those.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551