linux: avoid multiple dtb append on uImages
PATCH 1/1 linux: avoid multiple dtb append on uImages
From: Steve deRosier <[email protected]>
If using appended dtbs on uImages, a 'make linux-rebuild' will end up just adding the new dtb after the previous dtb if all you did was modify the device tree without a modification that would regenerate the zImage. This results in a surprising effect when you boot your device: your DT changes don't take effect because linux just loads the first dtb.
This patch fixes this little edge case by using an intermediate file for the append work if using BR2_LINUX_KERNEL_APPEND_UIMAGE. Otherwise it must retain the zImage naming, so the situation is unchanged for the case where this flag is not set.
--- linux/linux.mk | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
linux/linux.mk +13 -4
@@ -245,14 +245,23 @@ endif endif ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) -# dtbs moved from arch/$ARCH/boot to arch/$ARCH/boot/dts since 3.8-rc1 +# If we append and create the uImage, we want to be sure not to multiple-append on +# rebuilds, so use a temporary intermediate "zImage-dtb" and then append, but if we +# don't use the APPENDED_UIMAGE option, we need to be sure it stays named as zImage. +IMAGE_PLUS_DTB = zImage +ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y) + IMAGE_PLUS_DTB = zImage-dtb define LINUX_APPEND_DTB + cp $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB); +endef +endif +# dtbs moved from arch/$ARCH/boot to arch/$ARCH/boot/dts since 3.8-rc1 +LINUX_APPEND_DTB += \ if [ -e $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb ]; then \ cat $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb; \ else \ cat $(KERNEL_ARCH_PATH)/boot/dts/$(KERNEL_DTS_NAME).dtb; \ - fi >> $(KERNEL_ARCH_PATH)/boot/zImage -endef + fi >> $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB) ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y) # We need to generate a new u-boot image that takes into # account the extra-size added by the device tree at the end
@@ -263,7 +272,7 @@ LINUX_APPEND_DTB += $(sep) MKIMAGE_ARGS=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) |\ sed -n -e 's/Image Name:[ ]*\(.*\)/-n \1/p' -e 's/Load Address:/-a/p' -e 's/Entry Point:/-e/p'`; \ $(MKIMAGE) -A $(MKIMAGE_ARCH) -O linux \ -T kernel -C none $${MKIMAGE_ARGS} \ - -d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH); + -d $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB) $(LINUX_IMAGE_PATH); endif endif
Signature · 1 line
1.7.9.5
PATCH 1/1 linux: avoid multiple dtb append on uImages
Hi Steve,
Quoted 8 lines · Steve deRosier
From: Steve deRosier <[email protected]>
If using appended dtbs on uImages, a 'make linux-rebuild' will end up just adding the new dtb after the previous dtb if all you did was modify the device tree without a modification that would regenerate the zImage. This results in a surprising effect when you boot your device: your DT changes don't take effect because linux just loads the first dtb.
Quoted 4 lines
This patch fixes this little edge case by using an intermediate file for the append work if using
BR2_LINUX_KERNEL_APPEND_UIMAGE. Otherwise it must retain the zImage naming, so the situation is unchanged for the case where this flag is not set.
See http://patchwork.ozlabs.org/patch/394002/ (our oldest patchwork entry) for a different, arguably simpler, approach.
baruch
Quoted 44 lines
Signed-off-by Steve deRosier <[email protected]>--- linux/linux.mk | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)linux/linux.mk +13
-4@@ -245,14 +245,23 @@ endif endif ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) -# dtbs moved from arch/$ARCH/boot to arch/$ARCH/boot/dts since 3.8-rc1 +# If we append and create the uImage, we want to be sure not to multiple-append on +# rebuilds, so use a temporary intermediate "zImage-dtb" and then append, but if we +# don't use the APPENDED_UIMAGE option, we need to be sure it stays named as zImage. +IMAGE_PLUS_DTB = zImage +ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y) + IMAGE_PLUS_DTB = zImage-dtb define LINUX_APPEND_DTB + cp $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB); +endef +endif +# dtbs moved from arch/$ARCH/boot to arch/$ARCH/boot/dts since 3.8-rc1 +LINUX_APPEND_DTB += \ if [ -e $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb ]; then \ cat $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb; \ else \ cat $(KERNEL_ARCH_PATH)/boot/dts/$(KERNEL_DTS_NAME).dtb; \ - fi >> $(KERNEL_ARCH_PATH)/boot/zImage -endef + fi >> $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB) ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y) # We need to generate a new u-boot image that takes into # account the extra-size added by the device tree at the end@@ -263,7 +272,7 @@ LINUX_APPEND_DTB += $(sep) MKIMAGE_ARGS=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) |\ sed -n -e 's/Image Name:[ ]*\(.*\)/-n \1/p' -e 's/Load Address:/-a/p' -e 's/Entry Point:/-e/p'`; \ $(MKIMAGE) -A $(MKIMAGE_ARCH) -O linux \ -T kernel -C none $${MKIMAGE_ARGS} \ - -d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH); + -d $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB) $(LINUX_IMAGE_PATH); endif endif
Signature · 3 lines
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -PATCH 1/1 linux: avoid multiple dtb append on uImages
Hi Baruch,
Quoted 7 lines · Baruch Siach
Hi Steve,
See http://patchwork.ozlabs.org/patch/394002/ (our oldest patchwork entry) for a different, arguably simpler, approach.
baruch
Hi Baruch,
Thanks for bringing that thread to my attention. I couldn't quite tell if you appreciated the simpler approach of deleting the zImage (thus requiring the kernel to rebuild it) or prefered the more complex approach; either way, since that approach hasn't been merged in the year since it was sent, Thomas cleary didn't prefer it.
In reading that thread, I'm concerned about two things:
1. A fix (arguably good or bad) was proposed for a clear bug. And it was basically chosen to not fix the bug in favor of a somewhat dubious goal (my opinion) of increasing the feature set to "generalize this appended DTB mechanism to make it possible to generate several appended images". Seems to me that it would've been appropriate to fix the bug first, then expand the feature set.
2. The amount of time taken to fix said bug.
I encountered this bug for the first time over two years ago. At the time I was simply told that was a bug in the kernel build system (!):
http://article.gmane.org/gmane.comp.lib.uclibc.buildroot/63814
So I just continued what I was doing: made a trivial change in a driver source file, rebuilt and was able to get stuff working. I've since encountered this problem every couple of months. It wasn't until this week I just had enough and poked around and found the double-append root-cause. Hence the patch. Oddly enough I figured if there wasn't a patch in master, and I hadn't noticed any buzz about it, it was still an unsolved issue.
So I'm happy to help move this forward, including collaborating with Guido on this as being affected by this bug has been a significant pain for me over the last couple of years.
I'd like a specific pointer on how to move forward or even if I should scrap the current approach entirely. I won't be able to rewrite or restructure the entire linux.mk portion of the buildsystem, but I'm happy to fix the bug.
One obvious thing I can change is the naming I chose to correspond with Thomas's suggestion to Guido. But I'm thinking this won't be sufficient. For one, I don't really like my fall-over to the old behavior in the non uImage case, but I didn't see a great way forward there (not wanting to disturb the rest of the engine). But I was sufficiently happy that it fixed the issue in at least one case, but did no further harm to other cases so I decided to submit the patch.
So, @Thomas and @Guido, how can we get this moving forward? Can either of you (or anyone else) suggest some specific ways of moving forward?
Quoted 36 lines
Quoted 36 lines
Signed-off-by Steve deRosier <[email protected]>--- linux/linux.mk | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)linux/linux.mk +12
-3@@ -245,14 +245,23 @@ endif endif ifeq ($(BR2_LINUX_KERNEL_APPENDED_DTB),y) -# dtbs moved from arch/$ARCH/boot to arch/$ARCH/boot/dts since 3.8-rc1 +# If we append and create the uImage, we want to be sure not to multiple-append on +# rebuilds, so use a temporary intermediate "zImage-dtb" and then append, but if we +# don't use the APPENDED_UIMAGE option, we need to be sure it stays named as zImage. +IMAGE_PLUS_DTB = zImage +ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y) + IMAGE_PLUS_DTB = zImage-dtb define LINUX_APPEND_DTB + cp $(KERNEL_ARCH_PATH)/boot/zImage $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB); +endef +endif +# dtbs moved from arch/$ARCH/boot to arch/$ARCH/boot/dts since 3.8-rc1 +LINUX_APPEND_DTB += \ if [ -e $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb ]; then \ cat $(KERNEL_ARCH_PATH)/boot/$(KERNEL_DTS_NAME).dtb; \ else \ cat $(KERNEL_ARCH_PATH)/boot/dts/$(KERNEL_DTS_NAME).dtb; \ - fi >> $(KERNEL_ARCH_PATH)/boot/zImage -endef + fi >> $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB) ifeq ($(BR2_LINUX_KERNEL_APPENDED_UIMAGE),y) # We need to generate a new u-boot image that takes into # account the extra-size added by the device tree at the end
Quoted 8 lines
linux/linux.mk LINUX_APPEND_DTB += $(sep) MKIMAGE_ARGS=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) |\ · line 277
Quoted 8 lines
linux/linux.mk +1
-1@@ -263,7 +272,7 @@ LINUX_APPEND_DTB += $(sep) MKIMAGE_ARGS=`$(MKIMAGE) -l $(LINUX_IMAGE_PATH) |\ sed -n -e 's/Image Name:[ ]*\(.*\)/-n \1/p' -e 's/Load Address:/-a/p' -e 's/Entry Point:/-e/p'`; \ $(MKIMAGE) -A $(MKIMAGE_ARCH) -O linux \ -T kernel -C none $${MKIMAGE_ARGS} \ - -d $(KERNEL_ARCH_PATH)/boot/zImage $(LINUX_IMAGE_PATH); + -d $(KERNEL_ARCH_PATH)/boot/$(IMAGE_PLUS_DTB) $(LINUX_IMAGE_PATH); endif endif
Thanks,
- Steve
PATCH 1/1 linux: avoid multiple dtb append on uImages
In reply to derosier at gmail.comHello,
Quoted 8 lines · derosier at gmail.com
From: Steve deRosier <[email protected]>
If using appended dtbs on uImages, a 'make linux-rebuild' will end up just adding the new dtb after the previous dtb if all you did was modify the device tree without a modification that would regenerate the zImage. This results in a surprising effect when you boot your device: your DT changes don't take effect because linux just loads the first dtb.
Quoted 6 lines
This patch fixes this little edge case by using an intermediate file for the append work if using
BR2_LINUX_KERNEL_APPEND_UIMAGE. Otherwise it must retain the zImage naming, so the situation is unchanged for the case where this flag is not set.Signed-off-by Steve deRosier <[email protected]>
FWIW, this bug was finally fixed by
https://git.busybox.net/buildroot/commit/linux?id=055e6162bba7edb1db78458e089e151abde625b6.
Best regards,
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com