Re: [PATCH v2] ARM: decompressor: simplify libfdt builds
From: Masahiro Yamada
Date: Sun Jan 19 2020 - 23:30:27 EST
Hi Russell,
On Mon, Jan 20, 2020 at 7:48 AM Russell King - ARM Linux admin
<linux@xxxxxxxxxxxxxxx> wrote:
>
> On Sun, Jan 19, 2020 at 10:08:22AM +0900, Masahiro Yamada wrote:
> > Copying source files during the build time may not end up with
> > as clean code as expected.
> >
> > lib/fdt*.c simply wrap scripts/dtc/libfdt/fdt*.c, and it works
> > nicely. Let's follow that approach for the arm decompressor, too.
> >
> > Add four wrappers, arch/arm/boot/compressed/fdt*.c and remove
> > the Makefile messes. Another nice thing is we no longer need to
> > maintain the own libfdt_env.h because the decompressor can include
> > <linux/libfdt_env.h>.
>
> Hi,
>
> This is a nice idea, but as Stephen's build has found, it is a very
> fragile change, particularly if you're doing a rebuild of an existing
> tree.
>
> Stephen's issue appears to be that - he has stale "shipped" copies
> that the old Makefile implementation created, which were attempted
> to be built with this patch applied. The result of that is we
> try and pick up scripts/dtc/libfdt/libfdt_env.h.
>
> The whole point of the kernel build system is so that we can make
> changes to the kernel tree, and then build the kernel, and have the
> build system work out how to rebuild the kernel in a proper and safe
> way without us having to endlessly clean the build tree just because
> a few patches have been added. This patch breaks that expectation.
>
> At the very least, this build-breaking nature needs to be mentioned,
> preferably telling people what they should be doing to fix the issue.
>
> An even better would be to find some way to avoid the issue in the
> first place, or find some way to warn about it - maybe by leaving a
> libfdt_env.h behind that has an appropriate #warning in it telling
> people what to do. Or something.
>
> Thanks.
Sorry for causing a trouble again.
I will add the following code in arch/arm/boot/compressed/Makefile:
# These were previously generated C files. When you are building the kernel
# with O=, make sure to remove the stale files in the output tree. Otherwise,
# the build system would wrongly compile the stale ones.
ifdef building_out_of_srctree
$(shell rm -f $(addprefix $(obj)/, fdt_rw.c fdt_ro.c fdt_wip.c fdt.c))
endif
Maybe we can remove this code in the future,
but we should keep it long enough.
If the out-of-tree "git bisect" crosses this commit,
the same build error would happen.
People usually do not clean the tree while git-bisecting.
I explained it in the commit description too.
I tested the out-of-tree build
with this patch applied/dropped.
For in-tree build,
"git checkout" seems to nicely overwrite
the stale generated files with the new
check-in files.
I attached the v3 patch.
If it is OK with you,
I will put it in the patch tracker.
Thank you.
--
Best Regards
Masahiro Yamada
From d6c8678a9a2c518c454d7e68ac69ea2ac435934f Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <masahiroy@xxxxxxxxxx>
Date: Sun, 19 Jan 2020 10:08:22 +0900
Subject: [PATCH v3] ARM: decompressor: simplify libfdt builds
Copying source files during the build time may not end up with
as clean code as expected.
lib/fdt*.c simply wrap scripts/dtc/libfdt/fdt*.c, and it works
nicely. Let's follow that approach for the arm decompressor, too.
Add four wrappers, arch/arm/boot/compressed/fdt*.c and remove
the Makefile messes. Another nice thing is we no longer need to
maintain the own libfdt_env.h because the decompressor can include
<linux/libfdt_env.h>.
There is a subtle problem when generated files are turned into
check-in files.
When you are doing a rebuild of an existing object tree with O=
option, there exists stale "shipped" copies that the old Makefile
implementation created. The build system ends up with compiling the
stale generated C files because Make searches for prerequisites
in the current directory (objtree) first, and then the directory
listed in VPATH (srctree).
To mend this issue, I added the following code:
ifdef building_out_of_srctree
$(shell rm -f $(addprefix $(obj)/, fdt_rw.c fdt_ro.c fdt_wip.c fdt.c))
endif
This will need to stay for a while because "git bisect" crossing this
commit, otherwise, would result in a build error.
Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
---
KernelVersion: v5.5-rc1
Changes in v3:
- remove stale files in the output tree
Changes in v2:
- fix build error
arch/arm/boot/compressed/.gitignore | 9 ------
arch/arm/boot/compressed/Makefile | 39 ++++++++++---------------
arch/arm/boot/compressed/atags_to_fdt.c | 1 +
arch/arm/boot/compressed/fdt.c | 1 +
arch/arm/boot/compressed/fdt_ro.c | 1 +
arch/arm/boot/compressed/fdt_rw.c | 1 +
arch/arm/boot/compressed/fdt_wip.c | 1 +
arch/arm/boot/compressed/libfdt_env.h | 24 ---------------
8 files changed, 21 insertions(+), 56 deletions(-)
create mode 100644 arch/arm/boot/compressed/fdt.c
create mode 100644 arch/arm/boot/compressed/fdt_ro.c
create mode 100644 arch/arm/boot/compressed/fdt_rw.c
create mode 100644 arch/arm/boot/compressed/fdt_wip.c
delete mode 100644 arch/arm/boot/compressed/libfdt_env.h
diff --git a/arch/arm/boot/compressed/.gitignore b/arch/arm/boot/compressed/.gitignore
index 86b2f5d28240..2fdb4885846b 100644
--- a/arch/arm/boot/compressed/.gitignore
+++ b/arch/arm/boot/compressed/.gitignore
@@ -6,12 +6,3 @@ hyp-stub.S
piggy_data
vmlinux
vmlinux.lds
-
-# borrowed libfdt files
-fdt.c
-fdt.h
-fdt_ro.c
-fdt_rw.c
-fdt_wip.c
-libfdt.h
-libfdt_internal.h
diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index da599c3a1193..1bef7995107a 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -76,29 +76,31 @@ compress-$(CONFIG_KERNEL_LZMA) = lzma
compress-$(CONFIG_KERNEL_XZ) = xzkern
compress-$(CONFIG_KERNEL_LZ4) = lz4
-# Borrowed libfdt files for the ATAG compatibility mode
-
-libfdt := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c
-libfdt_hdrs := fdt.h libfdt.h libfdt_internal.h
-
-libfdt_objs := $(addsuffix .o, $(basename $(libfdt)))
+ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
+libfdt_objs = fdt_rw.o fdt_ro.o fdt_wip.o fdt.o atags_to_fdt.o
-$(addprefix $(obj)/,$(libfdt) $(libfdt_hdrs)): $(obj)/%: $(srctree)/scripts/dtc/libfdt/%
- $(call cmd,shipped)
+OBJS += $(libfdt_objs)
-$(addprefix $(obj)/,$(libfdt_objs) atags_to_fdt.o): \
- $(addprefix $(obj)/,$(libfdt_hdrs))
+# -fstack-protector-strong triggers protection checks in this code,
+# but it is being used too early to link to meaningful stack_chk logic.
+nossp-flags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE) := -fno-stack-protector
+$(foreach o, $(libfdt_objs), \
+ $(eval CFLAGS_$(o) := -I $(srctree)/scripts/dtc/libfdt $(nossp-flags-y)))
+
+# These were previously generated C files. When you are building the kernel
+# with O=, make sure to remove the stale files in the output tree. Otherwise,
+# the build system wrongly compiles the stale ones.
+ifdef building_out_of_srctree
+$(shell rm -f $(addprefix $(obj)/, fdt_rw.c fdt_ro.c fdt_wip.c fdt.c))
+endif
-ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
-OBJS += $(libfdt_objs) atags_to_fdt.o
endif
targets := vmlinux vmlinux.lds piggy_data piggy.o \
lib1funcs.o ashldi3.o bswapsdi2.o \
head.o $(OBJS)
-clean-files += piggy_data lib1funcs.S ashldi3.S bswapsdi2.S \
- $(libfdt) $(libfdt_hdrs) hyp-stub.S
+clean-files += piggy_data lib1funcs.S ashldi3.S bswapsdi2.S hyp-stub.S
KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
KBUILD_CFLAGS += $(DISABLE_ARM_SSP_PER_TASK_PLUGIN)
@@ -108,15 +110,6 @@ ORIG_CFLAGS := $(KBUILD_CFLAGS)
KBUILD_CFLAGS = $(subst -pg, , $(ORIG_CFLAGS))
endif
-# -fstack-protector-strong triggers protection checks in this code,
-# but it is being used too early to link to meaningful stack_chk logic.
-nossp-flags-$(CONFIG_CC_HAS_STACKPROTECTOR_NONE) := -fno-stack-protector
-CFLAGS_atags_to_fdt.o := $(nossp-flags-y)
-CFLAGS_fdt.o := $(nossp-flags-y)
-CFLAGS_fdt_ro.o := $(nossp-flags-y)
-CFLAGS_fdt_rw.o := $(nossp-flags-y)
-CFLAGS_fdt_wip.o := $(nossp-flags-y)
-
ccflags-y := -fpic $(call cc-option,-mno-single-pic-base,) -fno-builtin -I$(obj)
asflags-y := -DZIMAGE
diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
index 64c49747f8a3..8452753efebe 100644
--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/libfdt_env.h>
#include <asm/setup.h>
#include <libfdt.h>
diff --git a/arch/arm/boot/compressed/fdt.c b/arch/arm/boot/compressed/fdt.c
new file mode 100644
index 000000000000..49bc1fc1e273
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt.c
@@ -0,0 +1 @@
+#include "../../../../lib/fdt.c"
diff --git a/arch/arm/boot/compressed/fdt_ro.c b/arch/arm/boot/compressed/fdt_ro.c
new file mode 100644
index 000000000000..fc7f8313e93e
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt_ro.c
@@ -0,0 +1 @@
+#include "../../../../lib/fdt_ro.c"
diff --git a/arch/arm/boot/compressed/fdt_rw.c b/arch/arm/boot/compressed/fdt_rw.c
new file mode 100644
index 000000000000..7e9777da2708
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt_rw.c
@@ -0,0 +1 @@
+#include "../../../../lib/fdt_rw.c"
diff --git a/arch/arm/boot/compressed/fdt_wip.c b/arch/arm/boot/compressed/fdt_wip.c
new file mode 100644
index 000000000000..f0b580e760a7
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt_wip.c
@@ -0,0 +1 @@
+#include "../../../../lib/fdt_wip.c"
diff --git a/arch/arm/boot/compressed/libfdt_env.h b/arch/arm/boot/compressed/libfdt_env.h
deleted file mode 100644
index 6a0f1f524466..000000000000
--- a/arch/arm/boot/compressed/libfdt_env.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ARM_LIBFDT_ENV_H
-#define _ARM_LIBFDT_ENV_H
-
-#include <linux/limits.h>
-#include <linux/types.h>
-#include <linux/string.h>
-#include <asm/byteorder.h>
-
-#define INT32_MAX S32_MAX
-#define UINT32_MAX U32_MAX
-
-typedef __be16 fdt16_t;
-typedef __be32 fdt32_t;
-typedef __be64 fdt64_t;
-
-#define fdt16_to_cpu(x) be16_to_cpu(x)
-#define cpu_to_fdt16(x) cpu_to_be16(x)
-#define fdt32_to_cpu(x) be32_to_cpu(x)
-#define cpu_to_fdt32(x) cpu_to_be32(x)
-#define fdt64_to_cpu(x) be64_to_cpu(x)
-#define cpu_to_fdt64(x) cpu_to_be64(x)
-
-#endif
--
2.17.1