[PATCH 1/4] kbuild: replace $(hdr-arch) with $(SRCARCH)

[PATCH 1/4] kbuild: replace $(hdr-arch) with $(SRCARCH)

[Masahiro Yamada]

Since commit 5e53879008b9 ("sparc,sparc64: unify Makefile"), hdr-arch
and SRCARCH always match.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Makefile                     | 21 +++++++++------------
 scripts/Makefile.headersinst |  2 +-
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index cf007a3..8d900ba 100644
--- a/Makefile
+++ b/Makefile
@@ -283,9 +283,6 @@ ifeq ($(ARCH),tilegx)
        SRCARCH := tile
 endif
 
-# Where to locate arch specific headers
-hdr-arch  := $(SRCARCH)
-
 KCONFIG_CONFIG	?= .config
 export KCONFIG_CONFIG
 
@@ -378,8 +375,8 @@ CFLAGS_KCOV	:= $(call cc-option,-fsanitize-coverage=trace-pc,)
 
 # Use USERINCLUDE when you must reference the UAPI directories only.
 USERINCLUDE    := \
-		-I$(srctree)/arch/$(hdr-arch)/include/uapi \
-		-I$(objtree)/arch/$(hdr-arch)/include/generated/uapi \
+		-I$(srctree)/arch/$(SRCARCH)/include/uapi \
+		-I$(objtree)/arch/$(SRCARCH)/include/generated/uapi \
 		-I$(srctree)/include/uapi \
 		-I$(objtree)/include/generated/uapi \
                 -include $(srctree)/include/linux/kconfig.h
@@ -387,8 +384,8 @@ USERINCLUDE    := \
 # Use LINUXINCLUDE when you must reference the include/ directory.
 # Needed to be compatible with the O= option
 LINUXINCLUDE    := \
-		-I$(srctree)/arch/$(hdr-arch)/include \
-		-I$(objtree)/arch/$(hdr-arch)/include/generated \
+		-I$(srctree)/arch/$(SRCARCH)/include \
+		-I$(objtree)/arch/$(SRCARCH)/include/generated \
 		$(if $(KBUILD_SRC), -I$(srctree)/include) \
 		-I$(objtree)/include \
 		$(USERINCLUDE)
@@ -1134,8 +1131,8 @@ headerdep:
 #Default location for installed headers
 export INSTALL_HDR_PATH = $(objtree)/usr
 
-# If we do an all arch process set dst to include/arch-$(hdr-arch)
-hdr-dst = $(if $(KBUILD_HEADERS), dst=include/arch-$(hdr-arch), dst=include)
+# If we do an all arch process set dst to include/arch-$(SRCARCH)
+hdr-dst = $(if $(KBUILD_HEADERS), dst=include/arch-$(SRCARCH), dst=include)
 
 PHONY += archheaders
 archheaders:
@@ -1153,10 +1150,10 @@ headers_install_all:
 
 PHONY += headers_install
 headers_install: __headers
-	$(if $(wildcard $(srctree)/arch/$(hdr-arch)/include/uapi/asm/Kbuild),, \
+	$(if $(wildcard $(srctree)/arch/$(SRCARCH)/include/uapi/asm/Kbuild),, \
 	  $(error Headers not exportable for the $(SRCARCH) architecture))
 	$(Q)$(MAKE) $(hdr-inst)=include/uapi dst=include
-	$(Q)$(MAKE) $(hdr-inst)=arch/$(hdr-arch)/include/uapi $(hdr-dst)
+	$(Q)$(MAKE) $(hdr-inst)=arch/$(SRCARCH)/include/uapi $(hdr-dst)
 
 PHONY += headers_check_all
 headers_check_all: headers_install_all
@@ -1165,7 +1162,7 @@ headers_check_all: headers_install_all
 PHONY += headers_check
 headers_check: headers_install
 	$(Q)$(MAKE) $(hdr-inst)=include/uapi dst=include HDRCHECK=1
-	$(Q)$(MAKE) $(hdr-inst)=arch/$(hdr-arch)/include/uapi $(hdr-dst) HDRCHECK=1
+	$(Q)$(MAKE) $(hdr-inst)=arch/$(SRCARCH)/include/uapi $(hdr-dst) HDRCHECK=1
 
 # ---------------------------------------------------------------------------
 # Kernel selftest
diff --git a/scripts/Makefile.headersinst b/scripts/Makefile.headersinst
index 343d586..5692d7a 100644
--- a/scripts/Makefile.headersinst
+++ b/scripts/Makefile.headersinst
@@ -30,7 +30,7 @@ __headers: $(subdirs)
 $(subdirs):
 	$(Q)$(MAKE) $(hdr-inst)=$(obj)/$@ dst=$(dst)/$@
 
-# Skip header install/check for include/uapi and arch/$(hdr-arch)/include/uapi.
+# Skip header install/check for include/uapi and arch/$(SRCARCH)/include/uapi.
 # We have only sub-directories there.
 skip-inst := $(if $(filter %/uapi,$(obj)),1)
 
-- 
2.7.4

[PATCH 2/4] kbuild: move "_all" target out of $(KBUILD_SRC) conditional

[Masahiro Yamada]

The first "_all" occurrence around line 120 is only visible when
KBUILD_SRC is unset.

If O=... is specified, the working directory is relocated, then the
only second occurrence around line 193 is visible, that is not set
to PHONY.

Move the first one to an always visible place.  This clarifies "_all"
is our default target and it is always set to PHONY.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 8d900ba..39a7c03 100644
--- a/Makefile
+++ b/Makefile
@@ -10,6 +10,10 @@ NAME = Fearless Coyote
 # Comments in this file are targeted only to the developer, do not
 # expect to learn how to build the kernel reading this file.
 
+# That's our default target when none is given on the command line
+PHONY := _all
+_all:
+
 # o Do not use make's built-in rules and variables
 #   (this increases performance and avoids hard-to-debug behaviour);
 # o Look for make include files relative to root of kernel src
@@ -116,10 +120,6 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)
 endif
 
-# That's our default target when none is given on the command line
-PHONY := _all
-_all:
-
 # Cancel implicit rules on top Makefile
 $(CURDIR)/Makefile Makefile: ;
 
-- 
2.7.4

[PATCH 3/4] kbuild: re-order the code to not parse unnecessary variables

[Masahiro Yamada]

The top Makefile is divided into some sections such as mixed targets,
config targets, build targets, etc.

When we build mixed targets, Kbuild just invokes submake to process
them one by one.  In this case, compiler-related variables like CC,
KBUILD_CFLAGS, etc. are unneeded.

Check what kind of targets we are building first, then parse necessary
variables for building them.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Makefile | 233 ++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 118 insertions(+), 115 deletions(-)

diff --git a/Makefile b/Makefile
index 39a7c03..a4fd682 100644
--- a/Makefile
+++ b/Makefile
@@ -186,15 +186,6 @@ ifeq ("$(origin M)", "command line")
   KBUILD_EXTMOD := $(M)
 endif
 
-# If building an external module we do not care about the all: rule
-# but instead _all depend on modules
-PHONY += all
-ifeq ($(KBUILD_EXTMOD),)
-_all: all
-else
-_all: modules
-endif
-
 ifeq ($(KBUILD_SRC),)
         # building in the source tree
         srctree := .
@@ -206,6 +197,9 @@ else
                 srctree := $(KBUILD_SRC)
         endif
 endif
+
+export KBUILD_CHECKSRC KBUILD_EXTMOD KBUILD_SRC
+
 objtree		:= .
 src		:= $(srctree)
 obj		:= $(objtree)
@@ -214,6 +208,74 @@ VPATH		:= $(srctree)$(if $(KBUILD_EXTMOD),:$(KBUILD_EXTMOD))
 
 export srctree objtree VPATH
 
+# To make sure we do not include .config for any of the *config targets
+# catch them early, and hand them over to scripts/kconfig/Makefile
+# It is allowed to specify more targets when calling make, including
+# mixing *config targets and build targets.
+# For example 'make oldconfig all'.
+# Detect when mixed targets is specified, and make a second invocation
+# of make so .config is not included in this case either (for *config).
+
+version_h := include/generated/uapi/linux/version.h
+old_version_h := include/linux/version.h
+
+no-dot-config-targets := clean mrproper distclean \
+			 cscope gtags TAGS tags help% %docs check% coccicheck \
+			 $(version_h) headers_% archheaders archscripts \
+			 kernelversion %src-pkg
+
+config-targets := 0
+mixed-targets  := 0
+dot-config     := 1
+
+ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),)
+	ifeq ($(filter-out $(no-dot-config-targets), $(MAKECMDGOALS)),)
+		dot-config := 0
+	endif
+endif
+
+ifeq ($(KBUILD_EXTMOD),)
+        ifneq ($(filter config %config,$(MAKECMDGOALS)),)
+                config-targets := 1
+                ifneq ($(words $(MAKECMDGOALS)),1)
+                        mixed-targets := 1
+                endif
+        endif
+endif
+# install and modules_install need also be processed one by one
+ifneq ($(filter install,$(MAKECMDGOALS)),)
+        ifneq ($(filter modules_install,$(MAKECMDGOALS)),)
+	        mixed-targets := 1
+        endif
+endif
+
+ifeq ($(mixed-targets),1)
+# ===========================================================================
+# We're called with mixed targets (*config and build targets).
+# Handle them one by one.
+
+PHONY += $(MAKECMDGOALS) __build_one_by_one
+
+$(filter-out __build_one_by_one, $(MAKECMDGOALS)): __build_one_by_one
+	@:
+
+__build_one_by_one:
+	$(Q)set -e; \
+	for i in $(MAKECMDGOALS); do \
+		$(MAKE) -f $(srctree)/Makefile $$i; \
+	done
+
+else
+
+# We need some generic definitions (do not try to remake the file).
+scripts/Kbuild.include: ;
+include scripts/Kbuild.include
+
+# Read KERNELRELEASE from include/config/kernel.release (if it exists)
+KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
+KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if $(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION)
+export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
+
 # SUBARCH tells the usermode build what the underlying arch is.  That is set
 # first, and if a usermode build is happening, the "ARCH=um" on the command
 # line overrides the setting of ARCH below.  If a native build is happening,
@@ -308,40 +370,6 @@ HOSTCFLAGS  += -Wno-unused-value -Wno-unused-parameter \
 		-Wno-missing-field-initializers -fno-delete-null-pointer-checks
 endif
 
-# Decide whether to build built-in, modular, or both.
-# Normally, just do built-in.
-
-KBUILD_MODULES :=
-KBUILD_BUILTIN := 1
-
-# If we have only "make modules", don't compile built-in objects.
-# When we're building modules with modversions, we need to consider
-# the built-in objects during the descend as well, in order to
-# make sure the checksums are up to date before we record them.
-
-ifeq ($(MAKECMDGOALS),modules)
-  KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1)
-endif
-
-# If we have "make <whatever> modules", compile modules
-# in addition to whatever we do anyway.
-# Just "make" or "make all" shall build modules as well
-
-ifneq ($(filter all _all modules,$(MAKECMDGOALS)),)
-  KBUILD_MODULES := 1
-endif
-
-ifeq ($(MAKECMDGOALS),)
-  KBUILD_MODULES := 1
-endif
-
-export KBUILD_MODULES KBUILD_BUILTIN
-export KBUILD_CHECKSRC KBUILD_SRC KBUILD_EXTMOD
-
-# We need some generic definitions (do not try to remake the file).
-scripts/Kbuild.include: ;
-include scripts/Kbuild.include
-
 # Make variables (CC, etc...)
 AS		= $(CROSS_COMPILE)as
 LD		= $(CROSS_COMPILE)ld
@@ -406,11 +434,6 @@ KBUILD_AFLAGS_MODULE  := -DMODULE
 KBUILD_CFLAGS_MODULE  := -DMODULE
 KBUILD_LDFLAGS_MODULE := -T $(srctree)/scripts/module-common.lds
 
-# Read KERNELRELEASE from include/config/kernel.release (if it exists)
-KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
-KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if $(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION)
-
-export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
 export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
 export CPP AR NM STRIP OBJCOPY OBJDUMP HOSTLDFLAGS HOST_LOADLIBES
 export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
@@ -459,73 +482,6 @@ ifneq ($(KBUILD_SRC),)
 	    $(srctree) $(objtree) $(VERSION) $(PATCHLEVEL)
 endif
 
-# Support for using generic headers in asm-generic
-PHONY += asm-generic uapi-asm-generic
-asm-generic: uapi-asm-generic
-	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.asm-generic \
-	            src=asm obj=arch/$(SRCARCH)/include/generated/asm
-uapi-asm-generic:
-	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.asm-generic \
-	            src=uapi/asm obj=arch/$(SRCARCH)/include/generated/uapi/asm
-
-# To make sure we do not include .config for any of the *config targets
-# catch them early, and hand them over to scripts/kconfig/Makefile
-# It is allowed to specify more targets when calling make, including
-# mixing *config targets and build targets.
-# For example 'make oldconfig all'.
-# Detect when mixed targets is specified, and make a second invocation
-# of make so .config is not included in this case either (for *config).
-
-version_h := include/generated/uapi/linux/version.h
-old_version_h := include/linux/version.h
-
-no-dot-config-targets := clean mrproper distclean \
-			 cscope gtags TAGS tags help% %docs check% coccicheck \
-			 $(version_h) headers_% archheaders archscripts \
-			 kernelversion %src-pkg
-
-config-targets := 0
-mixed-targets  := 0
-dot-config     := 1
-
-ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),)
-	ifeq ($(filter-out $(no-dot-config-targets), $(MAKECMDGOALS)),)
-		dot-config := 0
-	endif
-endif
-
-ifeq ($(KBUILD_EXTMOD),)
-        ifneq ($(filter config %config,$(MAKECMDGOALS)),)
-                config-targets := 1
-                ifneq ($(words $(MAKECMDGOALS)),1)
-                        mixed-targets := 1
-                endif
-        endif
-endif
-# install and modules_install need also be processed one by one
-ifneq ($(filter install,$(MAKECMDGOALS)),)
-        ifneq ($(filter modules_install,$(MAKECMDGOALS)),)
-	        mixed-targets := 1
-        endif
-endif
-
-ifeq ($(mixed-targets),1)
-# ===========================================================================
-# We're called with mixed targets (*config and build targets).
-# Handle them one by one.
-
-PHONY += $(MAKECMDGOALS) __build_one_by_one
-
-$(filter-out __build_one_by_one, $(MAKECMDGOALS)): __build_one_by_one
-	@:
-
-__build_one_by_one:
-	$(Q)set -e; \
-	for i in $(MAKECMDGOALS); do \
-		$(MAKE) -f $(srctree)/Makefile $$i; \
-	done
-
-else
 ifeq ($(config-targets),1)
 # ===========================================================================
 # *config targets only - make sure prerequisites are updated, and descend
@@ -548,6 +504,44 @@ else
 # Build targets only - this includes vmlinux, arch specific targets, clean
 # targets and others. In general all targets except *config targets.
 
+# If building an external module we do not care about the all: rule
+# but instead _all depend on modules
+PHONY += all
+ifeq ($(KBUILD_EXTMOD),)
+_all: all
+else
+_all: modules
+endif
+
+# Decide whether to build built-in, modular, or both.
+# Normally, just do built-in.
+
+KBUILD_MODULES :=
+KBUILD_BUILTIN := 1
+
+# If we have only "make modules", don't compile built-in objects.
+# When we're building modules with modversions, we need to consider
+# the built-in objects during the descend as well, in order to
+# make sure the checksums are up to date before we record them.
+
+ifeq ($(MAKECMDGOALS),modules)
+  KBUILD_BUILTIN := $(if $(CONFIG_MODVERSIONS),1)
+endif
+
+# If we have "make <whatever> modules", compile modules
+# in addition to whatever we do anyway.
+# Just "make" or "make all" shall build modules as well
+
+ifneq ($(filter all _all modules,$(MAKECMDGOALS)),)
+  KBUILD_MODULES := 1
+endif
+
+ifeq ($(MAKECMDGOALS),)
+  KBUILD_MODULES := 1
+endif
+
+export KBUILD_MODULES KBUILD_BUILTIN
+
 ifeq ($(KBUILD_EXTMOD),)
 # Additional helpers built in scripts/
 # Carefully list dependencies so we do not try to build scripts twice
@@ -1063,6 +1057,15 @@ prepare0: archprepare gcc-plugins
 # All the preparing..
 prepare: prepare0 prepare-objtool
 
+# Support for using generic headers in asm-generic
+PHONY += asm-generic uapi-asm-generic
+asm-generic: uapi-asm-generic
+	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.asm-generic \
+	            src=asm obj=arch/$(SRCARCH)/include/generated/asm
+uapi-asm-generic:
+	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.asm-generic \
+	            src=uapi/asm obj=arch/$(SRCARCH)/include/generated/uapi/asm
+
 PHONY += prepare-objtool
 prepare-objtool: $(objtool_target)
 
-- 
2.7.4

[RFC PATCH 4/4] kbuild: evaluate cc-option and friends only when building kernel

[Masahiro Yamada]

$(call cc-option,...) is costly because it invokes the C compiler.

Given that only some targets need compiler flags, it is pointless
to compute them all the time.

The variable no-dot-config-targets lists the targets we can run
without the .config file, such as "make clean", "make help", etc.

My idea is similar here.  We can add no-compiler-targets to list the
targets we can run without the target compiler information.  This
includes no-dot-config-targets + config targets + misc.

When we run only targets listed in the no-compiler-targets, we can
set cc-option and friends to no-op.  The hostcc-option is an exception
since the host compiler is needed for building fixdep, kconfig, etc.

I did not add "dtbs" and "%.dtb" to no-compiler-targets.  This is
intentional.  Theoretically, we can build device tree blobs without
the C compiler.  It it true that in-kernel DT files are pre-processed
by CPP before DTC, but KBUILD_CPPFLAGS is unused.  However, for the
reason of scripts/dtc/ location, Kbuild descends into scripts/mod/
before the DT build.  I do not want to trigger the unrelated re-build
of modpost when building DT.  Perhaps, we can fix this with further
refactoring, but not now.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Makefile               | 10 ++++++++++
 scripts/Kbuild.include | 14 +++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index a4fd682..80e1a38 100644
--- a/Makefile
+++ b/Makefile
@@ -224,9 +224,13 @@ no-dot-config-targets := clean mrproper distclean \
 			 $(version_h) headers_% archheaders archscripts \
 			 kernelversion %src-pkg
 
+no-compiler-targets := $(no-dot-config-targets) config %config \
+		       kernelrelease image_name
+
 config-targets := 0
 mixed-targets  := 0
 dot-config     := 1
+need-compiler  := 1
 
 ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),)
 	ifeq ($(filter-out $(no-dot-config-targets), $(MAKECMDGOALS)),)
@@ -234,6 +238,12 @@ ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),)
 	endif
 endif
 
+ifneq ($(filter $(no-compiler-targets), $(MAKECMDGOALS)),)
+	ifeq ($(filter-out $(no-compiler-targets), $(MAKECMDGOALS)),)
+		need-compiler := 0
+	endif
+endif
+
 ifeq ($(KBUILD_EXTMOD),)
         ifneq ($(filter config %config,$(MAKECMDGOALS)),)
                 config-targets := 1
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 9ffd3dd..222d0a2 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -96,6 +96,13 @@ try-run = $(shell set -e;		\
 	fi;				\
 	rm -f "$$TMP" "$$TMPO")
 
+# hostcc-option
+# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
+hostcc-option = $(call __cc-option, $(HOSTCC),\
+	$(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))
+
+ifeq ($(need-compiler),1)
+
 # as-option
 # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
 
@@ -123,11 +130,6 @@ CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
 cc-option = $(call __cc-option, $(CC),\
 	$(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),$(1),$(2))
 
-# hostcc-option
-# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
-hostcc-option = $(call __cc-option, $(HOSTCC),\
-	$(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))
-
 # cc-option-yn
 # Usage: flag := $(call cc-option-yn,-march=winchip-c6)
 cc-option-yn = $(call try-run,\
@@ -180,6 +182,8 @@ ld-version = $(shell $(LD) --version | $(srctree)/scripts/ld-version.sh)
 # Usage:  $(call ld-ifversion, -ge, 22252, y)
 ld-ifversion = $(shell [ $(ld-version) $(1) $(2) ] && echo $(3) || echo $(4))
 
+endif
+
 ######
 
 ###
-- 
2.7.4

Re: [PATCH 1/4] kbuild: replace $(hdr-arch) with $(SRCARCH)

[Doug Anderson]

Hi,

On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Since commit 5e53879008b9 ("sparc,sparc64: unify Makefile"), hdr-arch
> and SRCARCH always match.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  Makefile                     | 21 +++++++++------------
>  scripts/Makefile.headersinst |  2 +-
>  2 files changed, 10 insertions(+), 13 deletions(-)

Looks sane/correct to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/4] kbuild: move "_all" target out of $(KBUILD_SRC) conditional

[Doug Anderson]

Hi,

On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> The first "_all" occurrence around line 120 is only visible when
> KBUILD_SRC is unset.
>
> If O=... is specified, the working directory is relocated, then the
> only second occurrence around line 193 is visible, that is not set
> to PHONY.
>
> Move the first one to an always visible place.  This clarifies "_all"
> is our default target and it is always set to PHONY.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  Makefile | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I'm a professed non-expert on the kernel build so take my review FWIW...

I'd definitely agree that it looks like it was a bug (though not a
huge one) that it was possible for "_all" not to get marked PHONY.
Other than that this change just makes things a little more readable
since (if I followed all the Makefile code properly) prior to your
change "_all" always ended up being the default rule, just in a very
roundabout way.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 3/4] kbuild: re-order the code to not parse unnecessary variables

[Doug Anderson]

Hi,

On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> The top Makefile is divided into some sections such as mixed targets,
> config targets, build targets, etc.
>
> When we build mixed targets, Kbuild just invokes submake to process
> them one by one.  In this case, compiler-related variables like CC,
> KBUILD_CFLAGS, etc. are unneeded.
>
> Check what kind of targets we are building first, then parse necessary
> variables for building them.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  Makefile | 233 ++++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 118 insertions(+), 115 deletions(-)

I'm even further outside my comfort zone with this big of a change,
but I will say that as far as I can tell this seems like a good
change.  If it were me I'd have probably broken it up into several
tinier changes that were each massively easy to check/verify, but
presumably for those who know the Makefile better then rolling them
together is fine.  ;)

I're spent some time reviewing this (not tons--maybe an hour or so),
but IMHO I don't know this well enough to add a Reviewed-by tag.


> diff --git a/Makefile b/Makefile
> index 39a7c03..a4fd682 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -186,15 +186,6 @@ ifeq ("$(origin M)", "command line")
>    KBUILD_EXTMOD := $(M)
>  endif
>
> -# If building an external module we do not care about the all: rule
> -# but instead _all depend on modules
> -PHONY += all
> -ifeq ($(KBUILD_EXTMOD),)
> -_all: all
> -else
> -_all: modules
> -endif
> -
>  ifeq ($(KBUILD_SRC),)
>          # building in the source tree
>          srctree := .
> @@ -206,6 +197,9 @@ else
>                  srctree := $(KBUILD_SRC)
>          endif
>  endif
> +
> +export KBUILD_CHECKSRC KBUILD_EXTMOD KBUILD_SRC

The old Makefile also used to export KBUILD_SRC, but I'm not sure why.
Shouldn't this be implicit because KBUILD_SRC always comes from a
command line parameter or environment variable?


-Doug

Re: [RFC PATCH 4/4] kbuild: evaluate cc-option and friends only when building kernel

[Doug Anderson]

Hi,

On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index 9ffd3dd..222d0a2 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -96,6 +96,13 @@ try-run = $(shell set -e;            \
>         fi;                             \
>         rm -f "$$TMP" "$$TMPO")
>
> +# hostcc-option
> +# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
> +hostcc-option = $(call __cc-option, $(HOSTCC),\
> +       $(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))

I believe you've got a bug here.  You're calling "__cc-option" which
isn't defined if "need-compiler" is not 1, right?

> +
> +ifeq ($(need-compiler),1)

The way this patch works is a bit non-obvious I think.  I wonder if
anyone else will be confused like I am...

Basically if "need-compiler" is not 1 then things like "cc-option"
won't be defined at all.  ...but we'll still _call_ them in other
Makefiles.  This call of an undefined variable will just evaluate to
an empty string.  Thus, for instance:

CFLAGS_KCOV := $(call cc-option,-fsanitize-coverage=trace-pc,)

...will just set CFLAGS_KCOV to the empty string if "need-compiler"
isn't 1, right?


I guess that's fine, but maybe at least document it somewhere?  IMHO
it would be even better if somehow you still defined each of these to
something bogus in an else clause, like:

as-option = --err_noncompile_target
as-instr = --err_noncompile_target
cc-option = --err_noncompile_target
...
...

The idea being that if someone accidentally invoked the C compiler (or
if there was some other conditional code in the Makefile based on the
result of one of these functions) it would be obvious what was going
on.


-Doug

Re: [PATCH 3/4] kbuild: re-order the code to not parse unnecessary variables

[Masahiro Yamada]

2017-10-10 7:03 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> The top Makefile is divided into some sections such as mixed targets,
>> config targets, build targets, etc.
>>
>> When we build mixed targets, Kbuild just invokes submake to process
>> them one by one.  In this case, compiler-related variables like CC,
>> KBUILD_CFLAGS, etc. are unneeded.
>>
>> Check what kind of targets we are building first, then parse necessary
>> variables for building them.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  Makefile | 233 ++++++++++++++++++++++++++++++++-------------------------------
>>  1 file changed, 118 insertions(+), 115 deletions(-)
>
> I'm even further outside my comfort zone with this big of a change,
> but I will say that as far as I can tell this seems like a good
> change.  If it were me I'd have probably broken it up into several
> tinier changes that were each massively easy to check/verify, but
> presumably for those who know the Makefile better then rolling them
> together is fine.  ;)
>
> I're spent some time reviewing this (not tons--maybe an hour or so),
> but IMHO I don't know this well enough to add a Reviewed-by tag.
>
>
>> diff --git a/Makefile b/Makefile
>> index 39a7c03..a4fd682 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -186,15 +186,6 @@ ifeq ("$(origin M)", "command line")
>>    KBUILD_EXTMOD := $(M)
>>  endif
>>
>> -# If building an external module we do not care about the all: rule
>> -# but instead _all depend on modules
>> -PHONY += all
>> -ifeq ($(KBUILD_EXTMOD),)
>> -_all: all
>> -else
>> -_all: modules
>> -endif
>> -
>>  ifeq ($(KBUILD_SRC),)
>>          # building in the source tree
>>          srctree := .
>> @@ -206,6 +197,9 @@ else
>>                  srctree := $(KBUILD_SRC)
>>          endif
>>  endif
>> +
>> +export KBUILD_CHECKSRC KBUILD_EXTMOD KBUILD_SRC
>
> The old Makefile also used to export KBUILD_SRC, but I'm not sure why.
> Shouldn't this be implicit because KBUILD_SRC always comes from a
> command line parameter or environment variable?


As the comment line around line 109 says,
"KBUILD_SRC is not intended to be used by the regular user (for now)"


It is set by "sub-make" around line 143:

sub-make:
         $(Q)$(MAKE) -C $(KBUILD_OUTPUT) KBUILD_SRC=$(CURDIR) \
         -f $(CURDIR)/Makefile $(filter-out _all sub-make,$(MAKECMDGOALS))



I'd like to improve it in the future,
but currently KBUILD_SRC works like that.

But, you are right.
I also thought "export KBUILD_SRC" is redundant.
KBUILD_SRC=$(CURDIR) implies exporting it.


I see some more redundant exporting
for variables we know they only come from command line or environment.


I think cleaning-up is OK, but should be
separated to another patch just in case.
(my commit claims I am just changing the code order...)




-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH 4/4] kbuild: evaluate cc-option and friends only when building kernel

[Masahiro Yamada]

2017-10-10 7:04 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
>> index 9ffd3dd..222d0a2 100644
>> --- a/scripts/Kbuild.include
>> +++ b/scripts/Kbuild.include
>> @@ -96,6 +96,13 @@ try-run = $(shell set -e;            \
>>         fi;                             \
>>         rm -f "$$TMP" "$$TMPO")
>>
>> +# hostcc-option
>> +# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
>> +hostcc-option = $(call __cc-option, $(HOSTCC),\
>> +       $(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))
>
> I believe you've got a bug here.  You're calling "__cc-option" which
> isn't defined if "need-compiler" is not 1, right?


Good catch!


>> +
>> +ifeq ($(need-compiler),1)
>
> The way this patch works is a bit non-obvious I think.  I wonder if
> anyone else will be confused like I am...
>
> Basically if "need-compiler" is not 1 then things like "cc-option"
> won't be defined at all.  ...but we'll still _call_ them in other
> Makefiles.  This call of an undefined variable will just evaluate to
> an empty string.  Thus, for instance:
>
> CFLAGS_KCOV := $(call cc-option,-fsanitize-coverage=trace-pc,)
>
> ...will just set CFLAGS_KCOV to the empty string if "need-compiler"
> isn't 1, right?
>
>
> I guess that's fine, but maybe at least document it somewhere?  IMHO
> it would be even better if somehow you still defined each of these to
> something bogus in an else clause, like:
>
> as-option = --err_noncompile_target
> as-instr = --err_noncompile_target
> cc-option = --err_noncompile_target
> ...
> ...
>
> The idea being that if someone accidentally invoked the C compiler (or
> if there was some other conditional code in the Makefile based on the
> result of one of these functions) it would be obvious what was going
> on.


Make sense.  Thanks for your idea!



I am still wondering if I should apply this one
because your cache approach provides much more benefits.

(that is why I prefixed this 4/4 with RFC.)


Maybe I will send v2, or maybe not.




-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH 4/4] kbuild: evaluate cc-option and friends only when building kernel

[Doug Anderson]

Hi,

On Mon, Oct 9, 2017 at 5:23 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> I am still wondering if I should apply this one
> because your cache approach provides much more benefits.
>
> (that is why I prefixed this 4/4 with RFC.)
>
>
> Maybe I will send v2, or maybe not.

I'm not sure I have a strong opinion either way.  In general your idea
makes a lot of sense--no need to do extra work if you don't need to.
It's all a question of how clean / maintainable it will be I guess.

-Doug

Re: [PATCH 1/4] kbuild: replace $(hdr-arch) with $(SRCARCH)

[Masahiro Yamada]

2017-10-04 12:56 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Since commit 5e53879008b9 ("sparc,sparc64: unify Makefile"), hdr-arch
> and SRCARCH always match.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Applied to linux-kbuild/kbuild.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/4] kbuild: move "_all" target out of $(KBUILD_SRC) conditional

[Masahiro Yamada]

2017-10-04 12:56 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> The first "_all" occurrence around line 120 is only visible when
> KBUILD_SRC is unset.
>
> If O=... is specified, the working directory is relocated, then the
> only second occurrence around line 193 is visible, that is not set
> to PHONY.
>
> Move the first one to an always visible place.  This clarifies "_all"
> is our default target and it is always set to PHONY.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Applied to linux-kbuild/kbuild.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 3/4] kbuild: re-order the code to not parse unnecessary variables

[Masahiro Yamada]

2017-10-04 12:56 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> The top Makefile is divided into some sections such as mixed targets,
> config targets, build targets, etc.
>
> When we build mixed targets, Kbuild just invokes submake to process
> them one by one.  In this case, compiler-related variables like CC,
> KBUILD_CFLAGS, etc. are unneeded.
>
> Check what kind of targets we are building first, then parse necessary
> variables for building them.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  Makefile | 233 ++++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 118 insertions(+), 115 deletions(-)


Applied to linux-kbuild/kbuild.  Thanks.

-- 
Best Regards
Masahiro Yamada