Received-SPF: pass (google.com: domain of linux-kernel+bounces-171196-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 7 May 2024 13:15:55 +0200
From: Nicolas Schier <nicolas@fjasle.eu>
To: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
	Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH 2/2] kbuild: add 'private' to target-specific variables
Message-ID: <ZjoNa434si-Hk0Cs@buildd.core.avm.de>
References: <20240427153253.2809911-1-masahiroy@kernel.org>
 <20240427153253.2809911-2-masahiroy@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240427153253.2809911-2-masahiroy@kernel.org>

On Sun, Apr 28, 2024 at 12:32:53AM +0900, Masahiro Yamada wrote:
> Currently, Kbuild produces inconsistent results in some cases.
> 
> You can do an interesting experiment using the --shuffle option, which
> is supported by GNU Make 4.4 or later.
> 
> Set CONFIG_KVM_INTEL=y and CONFIG_KVM_AMD=m (or vice versa), and repeat
> incremental builds w/wo --shuffle=reverse.
> 
>   $ make
>     [ snip ]
>     CC      arch/x86/kvm/kvm-asm-offsets.s
> 
>   $ make --shuffle=reverse
>     [ snip ]
>     CC [M]  arch/x86/kvm/kvm-asm-offsets.s
> 
>   $ make
>     [ snip ]
>     CC      arch/x86/kvm/kvm-asm-offsets.s
> 
> arch/x86/kvm/kvm-asm-offsets.s is rebuilt every time w/wo the [M] marker.
> 
> arch/x86/kvm/kvm-asm-offsets.s is built as built-in when it is built as
> a prerequisite of arch/x86/kvm/kvm-intel.o, which is built-in.
> 
> arch/x86/kvm/kvm-asm-offsets.s is built as modular when it is built as
> a prerequisite of arch/x86/kvm/kvm-amd.o, which is a module.
> 
> Another odd example is single target builds.
> 
> When CONFIG_LKDTM=m, drivers/misc/lkdtm/rodata.o can be built as
> built-in or modular, depending on how it is built.
> 
>   $ make drivers/misc/lkdtm/lkdtm.o
>     [ snip ]
>     CC [M]  drivers/misc/lkdtm/rodata.o
> 
>   $ make drivers/misc/lkdtm/rodata.o
>     [ snip ]
>     CC      drivers/misc/lkdtm/rodata.o
> 
> drivers/misc/lkdtm/rodata.o is built as modular when it is built as a
> prerequisite of another, but built as built-in when it is a final
> target.
> 
> The same thing happens to drivers/memory/emif-asm-offsets.s when
> CONFIG_TI_EMIF_SRAM=m.
> 
>   $ make drivers/memory/ti-emif-sram.o
>     [ snip ]
>     CC [M]  drivers/memory/emif-asm-offsets.s
> 
>   $ make drivers/memory/emif-asm-offsets.s
>     [ snip ]
>     CC      drivers/memory/emif-asm-offsets.s
> 
> This is because the part-of-module=y flag defined for the modules is
> inherited by its prerequisites.
> 
> Target-specific variables are likely intended only for local use.
> This commit adds 'private' to them.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

uh, thanks for fixing this!  (And for the bug documentation, as always!)

I have just one question below.

> 
>  Makefile               | 8 ++++----
>  scripts/Makefile.build | 6 +++---
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 62557fabfee5..25dcc7ead330 100644
> --- a/Makefile
> +++ b/Makefile
[...]
> @@ -1500,7 +1500,7 @@ MRPROPER_FILES += include/config include/generated          \
>  
>  # clean - Delete most, but leave enough to build external modules
>  #
> -clean: rm-files := $(CLEAN_FILES)
> +clean: private rm-files := $(CLEAN_FILES)

Did you leave 'clean: rm-files := $(KBUILD_EXTMOD)/...' for oot kmods
the way it is (w/o 'private') by intention?

Even though I cannot think of a possible problem without the 'private',
I think it makes sense to change the line as well.

W/ or w/o the 'clean'-update for oot kmods:

Reviewed-by: Nicolas Schier <n.schier@avm.de>

Kind regards,
Nicolas