Skip to contentSkip to conversation

linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

PATCH New Sebastian Frias 2 participants 9 messages Apr 25, 2016 – Apr 26, 2016

lore
Share & tools
lore

Sebastian Frias

author
#0 lore raw

PATCH 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS provided by (or updated with) 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") and thus is not operating properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB appended.

This patch replaces the 'cp' command with $(LINUX_INSTALL_IMAGES_CMDS) which handles APPENDED_DTB. It also makes the copy verbose so that the user can see how many images were copied.

Fixes 055e6162bba7 ("linux: don't build appended DTB image in place and

support multiple images")

Signed-off-by Sebastian Frias <[email protected]>
---
 linux/linux.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
linux/linux.mk +1 -1
index 317587f..1f93074 100644
@@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
 # appended DTBs.
 define LINUX_INSTALL_IMAGE
 	mkdir -p $(1)
-	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
+	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
 endef
 else
 # Otherwise, just install the unique image generated by the kernel

@@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt:

$(LINUX_DIR)/.stamp_target_installed $(LI
# Build the kernel.
$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)
$(LINUX_TARGET_NAME)
$(LINUX_APPEND_DTB)
- # Copy the kernel image to its final destination
- cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
+ # Copy the kernel image(s) to its(their) final destination
+ $(LINUX_INSTALL_IMAGES_CMDS)
# If there is a .ub file copy it to the final destination test ! -f $(LINUX_IMAGE_PATH).ub || cp $(LINUX_IMAGE_PATH).ub
$(BINARIES_DIR)
$(Q)touch $@

Signature · 1 line
1.7.11.2
#1 lore raw

PATCH 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

Hello,

Quoted 9 lines · Sebastian Frias

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS provided by (or updated with) 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") and thus is not operating properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB appended.

Quoted 6 lines

This patch replaces the 'cp' command with $(LINUX_INSTALL_IMAGES_CMDS) which handles APPENDED_DTB. It also makes the copy verbose so that the user can see how many images were copied.

Fixes 055e6162bba7 ("linux: don't build appended DTB image in place and

support multiple images")

Thanks for reporting the problem, and thanks for your patch!

linux/linux.mk ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) · line 333

Quoted 10 lines
linux/linux.mk +1 -1
index 317587f..1f93074 100644
@@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
 # appended DTBs.
 define LINUX_INSTALL_IMAGE
 	mkdir -p $(1)
-	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
+	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)

I think you did this change for debugging (which is OK), but it should be kept in your patch.

linux/linux.mk

Quoted 13 lines
linux/linux.mk +0 -0
 endef
 else
 # Otherwise, just install the unique image generated by the kernel

@@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt:

$(LINUX_DIR)/.stamp_target_installed $(LI

linux/linux.mk +0 -0
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)

$(LINUX_TARGET_NAME)

linux/linux.mk +2 -2
 	$(LINUX_APPEND_DTB)
-	# Copy the kernel image to its final destination
-	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
+	# Copy the kernel image(s) to its(their) final destination
+	$(LINUX_INSTALL_IMAGES_CMDS)

I think I would prefer to have:

	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#2 lore raw

PATCH 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

Hello,

Quoted 5 lines · Thomas Petazzoni

linux/linux.mk ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) · line 333

Quoted 2 lines
linux/linux.mk +1 -1
-	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
+	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)

I think you did this change for debugging (which is OK), but it should be kept in your patch.

Obviously: it should be *removed* from your patch. Sorry for the mess :-/

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#3 lore raw

PATCH 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

In reply to Thomas Petazzoni

Hi Thomas,

Quoted 3 lines · Thomas Petazzoni

Hello,

Thanks for reporting the problem, and thanks for your patch!

Sure! That's what Open Source is for! :-)

Quoted 14 lines

linux/linux.mk ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) · line 333

Quoted 11 lines
linux/linux.mk +1 -1
index 317587f..1f93074 100644
@@ -330,7 +330,7 @@ ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y)
 # appended DTBs.
 define LINUX_INSTALL_IMAGE
 	mkdir -p $(1)
-	cp $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)
+	cp -v $(KERNEL_ARCH_PATH)/boot/$(LINUX_IMAGE_NAME).* $(1)

I think you did this change for debugging (which is OK), but it should be kept in your patch.

Ok, I will remove it.

Quoted 9 lines
Quoted 9 lines
linux/linux.mk +0 -0
 endef
 else
 # Otherwise, just install the unique image generated by the kernel

@@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt:

$(LINUX_DIR)/.stamp_target_installed $(LI

linux/linux.mk +0 -0
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)

$(LINUX_TARGET_NAME)

This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it? LINUX_BUILD_CMDS does a couple of more things than what is done here, like like running LINUX_BUILD_DTB or 'make modules', and maybe that is not wanted?

Quoted 10 lines

linux/linux.mk

Quoted 5 lines
linux/linux.mk +2 -2
 	$(LINUX_APPEND_DTB)
-	# Copy the kernel image to its final destination
-	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
+	# Copy the kernel image(s) to its(their) final destination
+	$(LINUX_INSTALL_IMAGES_CMDS)

I think I would prefer to have:

	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))

Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also handles the DTB copy when APPENDED_DTB is not set, right?

Best regards,

Sebastian

#4 lore raw

PATCH 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

Hello,

Quoted 8 lines · Sebastian Frias
Quoted 3 lines
Quoted 3 lines
(quoted diff) +0 -0
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)

$(LINUX_TARGET_NAME)

This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it? LINUX_BUILD_CMDS does a couple of more things than what is done here, like like running LINUX_BUILD_DTB or 'make modules', and maybe that is not wanted?

We don't want to do "make modules" again, as this will change the root filesystem (surely by reinstalling the same thing, but still), while we are exactly in the process of adding the filesystem to the kernel image.

Quoted 6 lines
Quoted 3 lines

I think I would prefer to have:

	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))

Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also handles the DTB copy when APPENDED_DTB is not set, right?

It also copies the DTB to $(BINARIES_DIR), which is useless here. And in general, I don't like too much seeing the _CMDS variables called from other variables.

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#5 lore raw

PATCH 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

Hi Thomas,

Quoted 8 lines · Thomas Petazzoni

Hello,

Quoted 4 lines · Sebastian Frias
Quoted 3 lines
Quoted 3 lines
(quoted diff) +0 -0
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D)

$(LINUX_TARGET_NAME)

Quoted 8 lines
Quoted 4 lines

This line is also present on LINUX_BUILD_CMDS, any reason not to reuse it? LINUX_BUILD_CMDS does a couple of more things than what is done here, like like running LINUX_BUILD_DTB or 'make modules', and maybe that is not wanted?

We don't want to do "make modules" again, as this will change the root filesystem (surely by reinstalling the same thing, but still), while we are exactly in the process of adding the filesystem to the kernel image.

Ok, thanks for the explanation.

Quoted 11 lines
Quoted 7 lines
Quoted 4 lines

I think I would prefer to have:

	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))

Ok, just wondering, why? I mean, IIUC, LINUX_INSTALL_IMAGES_CMDS also handles the DTB copy when APPENDED_DTB is not set, right?

It also copies the DTB to $(BINARIES_DIR), which is useless here. And in general, I don't like too much seeing the _CMDS variables called from other variables.

Ok, just one last question because I seem to be missing something here.

In your commit 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") you indicated:

-----
<snip>
Some of the tested configuration:

- Normal uImage with several DTBs

BR2_LINUX_KERNEL_DEFCONFIG="mvebu_v7" BR2_LINUX_KERNEL_UIMAGE=y BR2_LINUX_KERNEL_UIMAGE_LOADADDR="0x200000" BR2_LINUX_KERNEL_DTS_SUPPORT=y BR2_LINUX_KERNEL_INTREE_DTS_NAME="armada-xp-matrix armada-xp-gp
armada-370-mirabox"

Contents of output/images/:

armada-370-mirabox.dtb armada-xp-gp.dtb armada-xp-matrix.dtb
uImage
<snip>
-----

Hence, the DTB images are intended to be copied to output/images/ under some circumstances, right?
How would that work? I mean, there's no call to LINUX_INSTALL_DTB on "$(LINUX_DIR)/.stamp_initramfs_rebuilt".
Or does that happens through LINUX_INSTALL_TARGET_CMDS and package/pkg-generic.mk's "$(BUILD_DIR)/%/.stamp_target_installed" target? In that case I think I don't understand why we enter in "$(LINUX_DIR)/.stamp_initramfs_rebuilt" target, given there's a target $(BUILD_DIR)/%/.stamp_images_installed that calls LINUX_INSTALL_IMAGES_CMDS.

Best regards,

Sebastian

author
#6 lore raw

PATCH v2 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

In reply to Thomas Petazzoni

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS provided by (or updated with) commit 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") and thus is not operating properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB appended.

This patch replaces the 'cp' command with LINUX_INSTALL_IMAGE which handles APPENDED_DTB.

Fixes 055e6162bba7 ("linux: don't build appended DTB image in place and

support multiple images")

Signed-off-by Sebastian Frias <[email protected]>

---
Changes v1 -> v2:
- remove '-v' from cp (as suggested by thomas.petazzoni at free-electrons.com)
- use LINUX_INSTALL_IMAGE instead of LINUX_INSTALL_IMAGES_CMDS (also suggested by thomas.petazzoni at free-electrons.com)

---
 linux/linux.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
linux/linux.mk +2 -2
index 317587f..1f93074 100644
@@ -447,8 +447,8 @@ $(LINUX_DIR)/.stamp_initramfs_rebuilt: $(LINUX_DIR)/.stamp_target_installed $(LI
 	# Build the kernel.
 	$(LINUX_MAKE_ENV) $(MAKE) $(LINUX_MAKE_FLAGS) -C $(@D) $(LINUX_TARGET_NAME)
 	$(LINUX_APPEND_DTB)
-	# Copy the kernel image to its final destination
-	cp $(LINUX_IMAGE_PATH) $(BINARIES_DIR)
+	# Copy the kernel image(s) to its(their) final destination
+	$(call LINUX_INSTALL_IMAGE,$(BINARIES_DIR))
 	# If there is a .ub file copy it to the final destination
 	test ! -f $(LINUX_IMAGE_PATH).ub || cp $(LINUX_IMAGE_PATH).ub $(BINARIES_DIR)
 	$(Q)touch $@
Signature · 1 line
1.7.11.2
#7 lore raw

PATCH v2 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

Hello,

Quoted 15 lines · Sebastian Frias

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS provided by (or updated with) commit 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") and thus is not operating properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB appended.

This patch replaces the 'cp' command with LINUX_INSTALL_IMAGE which handles APPENDED_DTB.

Fixes 055e6162bba7 ("linux: don't build appended DTB image in place and

support multiple images")

Quoted 8 lines
Signed-off-by Sebastian Frias <[email protected]>

---
Changes v1 -> v2:
- remove '-v' from cp (as suggested by thomas.petazzoni at free-electrons.com)
- use LINUX_INSTALL_IMAGE instead of LINUX_INSTALL_IMAGES_CMDS (also suggested by thomas.petazzoni at free-electrons.com)

---
 linux/linux.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

The commit title was no longer matching what the commit was doing, so I've tweaked it, and applied your patch. Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#8 lore raw

PATCH v2 1/1 linux: use $(LINUX_INSTALL_IMAGE_CMDS) instead of plain 'cp'

Hi Thomas,

Quoted 27 lines · Thomas Petazzoni

Hello,

Quoted 15 lines · Sebastian Frias

The target "$(LINUX_DIR)/.stamp_initramfs_rebuilt" uses its own 'cp' command, instead of LINUX_INSTALL_IMAGE/LINUX_INSTALL_IMAGES_CMDS provided by (or updated with) commit 055e6162bba7 ("linux: don't build appended DTB image in place and support multiple images") and thus is not operating properly when APPENDED_DTB is used.

Indeed, it copies a single image, and does not copy the one with the DTB appended.

This patch replaces the 'cp' command with LINUX_INSTALL_IMAGE which handles APPENDED_DTB.

Fixes 055e6162bba7 ("linux: don't build appended DTB image in place and

support multiple images")

Quoted 8 lines
Signed-off-by Sebastian Frias <[email protected]>

---
Changes v1 -> v2:
- remove '-v' from cp (as suggested by thomas.petazzoni at free-electrons.com)
- use LINUX_INSTALL_IMAGE instead of LINUX_INSTALL_IMAGES_CMDS (also suggested by thomas.petazzoni at free-electrons.com)

---
 linux/linux.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 3 lines

The commit title was no longer matching what the commit was doing, so I've tweaked it, and applied your patch. Thanks!

Oh, sorry about that (I had edited the patch manually), and thanks for fixing it! :-)

Best regards,

Sebastian