diff mbox series

[2/5] ramips: mt7621: drop custom uImage function

Message ID 20201104092117.86343-3-sander@svanheule.net
State Accepted
Headers show
Series Make Build/uImage more flexible | expand

Commit Message

Sander Vanheule Nov. 4, 2020, 9:21 a.m. UTC 
Use the mkimage argument overrides provided by uImage to implement the
customisations required for the initramfs, instead of the near-identical
custom function.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
Build tested for iodata_wn1167gr2, where
  uImage lzma -M 0x434f4d42 -n '3.10(XBC.1)b10'

... results in the following call made to mkimage:
  mkimage -A mips -O linux -T kernel -C lzma -a 0x80001000
    -e 0x80001000 -n 'MIPS OpenWrt Linux-5.4.74'  -M 0x434f4d42 
    -n '3.10(XBC.1)b10' -d initramfs-kernel.bin initramfs-kernel.bin.new

 target/linux/ramips/image/mt7621.mk | 31 ++++++++++-------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

Comments

Adrian Schmutzler Nov. 4, 2020, 1:42 p.m. UTC | #1 
Hi Sander,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Sander Vanheule
> Sent: Mittwoch, 4. November 2020 10:21
> To: openwrt-devel@lists.openwrt.org
> Cc: Sander Vanheule <sander@svanheule.net>
> Subject: [PATCH 2/5] ramips: mt7621: drop custom uImage function
> 
> Use the mkimage argument overrides provided by uImage to implement the
> customisations required for the initramfs, instead of the near-identical
> custom function.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> Build tested for iodata_wn1167gr2, where
>   uImage lzma -M 0x434f4d42 -n '3.10(XBC.1)b10'
> 
> ... results in the following call made to mkimage:
>   mkimage -A mips -O linux -T kernel -C lzma -a 0x80001000
>     -e 0x80001000 -n 'MIPS OpenWrt Linux-5.4.74'  -M 0x434f4d42
>     -n '3.10(XBC.1)b10' -d initramfs-kernel.bin initramfs-kernel.bin.new

And you are perfectly sure that there is no harm from specifying -n twice?

Apart from that, the question is whether we should rely on such behavior? (As much as I would like to have the redundancy gone, too).

Best

Adrian

> 
>  target/linux/ramips/image/mt7621.mk | 31 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/target/linux/ramips/image/mt7621.mk
> b/target/linux/ramips/image/mt7621.mk
> index bb7e5a0d48..9b6c7d2311 100644
> --- a/target/linux/ramips/image/mt7621.mk
> +++ b/target/linux/ramips/image/mt7621.mk
> @@ -7,21 +7,7 @@ include ./common-tp-link.mk  DEFAULT_SOC := mt7621
> 
>  KERNEL_DTB += -d21
> -DEVICE_VARS += UIMAGE_MAGIC ELECOM_HWNAME LINKSYS_HWNAME
> -
> -# The OEM webinterface expects an kernel with initramfs which has the
> uImage -# header field ih_name.
> -# We don't want to set the header name field for the kernel include in the -
> # sysupgrade image as well, as this image shouldn't be accepted by the OEM
> -# webinterface. It will soft-brick the board.
> -define Build/custom-initramfs-uimage
> -	mkimage -A $(LINUX_KARCH) \
> -		-O linux -T kernel \
> -		-C lzma -a $(KERNEL_LOADADDR) $(if $(UIMAGE_MAGIC),-M
> $(UIMAGE_MAGIC),) \
> -		-e $(if
> $(KERNEL_ENTRY),$(KERNEL_ENTRY),$(KERNEL_LOADADDR)) \
> -		-n '$(1)' -d $@ $@.new
> -	mv $@.new $@
> -endef
> +DEVICE_VARS += ELECOM_HWNAME LINKSYS_HWNAME
> 
>  define Build/elecom-wrc-gs-factory
>  	$(eval product=$(word 1,$(1)))
> @@ -531,32 +517,35 @@ define Device/iodata_nand
>    IMAGE/sysupgrade.bin := sysupgrade-tar | append-metadata  endef
> 
> +# The OEM webinterface expects an kernel with initramfs which has the
> +uImage # header field ih_name.
> +# We don't want to set the header name field for the kernel include in
> +the # sysupgrade image as well, as this image shouldn't be accepted by
> +the OEM # webinterface. It will soft-brick the board.
> +
>  define Device/iodata_wn-ax1167gr2
>    $(Device/iodata_nand)
> -  UIMAGE_MAGIC := 0x434f4d42
>    DEVICE_MODEL := WN-AX1167GR2
>    KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
> -	custom-initramfs-uimage 3.10(XBC.1)b10 | iodata-mstc-header
> +	uImage lzma -M 0x434f4d42 -n '3.10(XBC.1)b10' | iodata-mstc-header
>    DEVICE_PACKAGES := kmod-mt7615e kmod-mt7615-firmware  endef
> TARGET_DEVICES += iodata_wn-ax1167gr2
> 
>  define Device/iodata_wn-ax2033gr
>    $(Device/iodata_nand)
> -  UIMAGE_MAGIC := 0x434f4d42
>    DEVICE_MODEL := WN-AX2033GR
>    KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
> -	custom-initramfs-uimage 3.10(VST.1)C10 | iodata-mstc-header
> +	uImage lzma -M 0x434f4d42 -n '3.10(VST.1)C10' | iodata-mstc-header
>    DEVICE_PACKAGES := kmod-mt7603 kmod-mt7615e kmod-mt7615-
> firmware  endef  TARGET_DEVICES += iodata_wn-ax2033gr
> 
>  define Device/iodata_wn-dx1167r
>    $(Device/iodata_nand)
> -  UIMAGE_MAGIC := 0x434f4d43
>    DEVICE_MODEL := WN-DX1167R
>    KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
> -	custom-initramfs-uimage 3.10(XIK.1)b10 | iodata-mstc-header
> +	uImage lzma -M 0x434f4d43 -n '3.10(XIK.1)b10' | iodata-mstc-header
>    DEVICE_PACKAGES := kmod-mt7615e kmod-mt7615-firmware  endef
> TARGET_DEVICES += iodata_wn-dx1167r
> --
> 2.28.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Sander Vanheule Nov. 4, 2020, 7:17 p.m. UTC | #2 
Hi Adrian,

On Wed, 2020-11-04 at 14:42 +0100, Adrian Schmutzler wrote:
> Hi Sander,
> 
> > -----Original Message-----
> > From: openwrt-devel
> > [mailto:openwrt-devel-bounces@lists.openwrt.org]
> > On Behalf Of Sander Vanheule
> > Sent: Mittwoch, 4. November 2020 10:21
> > To: openwrt-devel@lists.openwrt.org
> > Cc: Sander Vanheule <sander@svanheule.net>
> > Subject: [PATCH 2/5] ramips: mt7621: drop custom uImage function
> > 
> > Use the mkimage argument overrides provided by uImage to implement
> > the
> > customisations required for the initramfs, instead of the near-
> > identical
> > custom function.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> > Build tested for iodata_wn1167gr2, where
> >   uImage lzma -M 0x434f4d42 -n '3.10(XBC.1)b10'
> > 
> > ... results in the following call made to mkimage:
> >   mkimage -A mips -O linux -T kernel -C lzma -a 0x80001000
> >     -e 0x80001000 -n 'MIPS OpenWrt Linux-5.4.74'  -M 0x434f4d42
> >     -n '3.10(XBC.1)b10' -d initramfs-kernel.bin initramfs-
> > kernel.bin.new
> 
> And you are perfectly sure that there is no harm from specifying -n
> twice?

If -n is found multiple times, only the last value returned by getopt
is stored.
See: https://github.com/u-boot/u-boot/blob/master/tools/mkimage.c#L240

With "uImage lzma":
00000000: 2705 1956 c8d1 4824 5fa1 c601 0037 aec2  '..V..H$_....7..
00000010: 8000 0000 8000 0400 cb50 87d5 0505 0203  .........P......
00000020: 4d49 5053 204f 7065 6e57 7274 204c 696e  MIPS OpenWrt Lin
00000030: 7578 2d35 2e34 2e37 3400 0000 0000 0000  ux-5.4.74.......

With "uImage lzma -n 'test image'":
00000000: 2705 1956 dd5c 05e1 5fa1 c601 001d 3613  '..V.\.._.....6.
00000010: 8000 0000 8000 0400 25d0 53ae 0505 0203  ........%.S.....
00000020: 7465 7374 2069 6d61 6765 0000 0000 0000  test image......
00000030: 0000 0000 0000 0000 0000 0000 0000 0000  ................


> Apart from that, the question is whether we should rely on such
> behavior? (As much as I would like to have the redundancy gone, too).

From getopt(3):
   If getopt() finds another option character, it returns that character,
   updating the external variable optind and a static variable nextchar so
   that the next call to getopt() can resume the scan with the following
   option character or argv-element.

Which I understand to mean that options are processed in-order,
matching the implementation.
https://code.woboq.org/userspace/glibc/posix/getopt.c.html

There may still be some reordering of the arguments:
   By default, getopt() permutes the contents of argv as it scans, so that
   eventually all the nonoptions are at the end. Two other modes are also
   implemented. If the first character of optstring is '+' or the
   environment variable POSIXLY_CORRECT is set, then option processing
   stops as soon as a nonoption argument is encountered.

The reordering is implemented such that blocks of non-option arguments
are swapped with blocks of option argument immediately following them.
The end result is that all arguments are processed as if the option
arguments were all provided before the non-option arguments. Both will
maintain the order within their respective subsets.

mkimage only supports one non-option argument. If someone would supply
a non-option argument with uImage, the build would fail. This is
because mkimage only uses the first non-option argument it encounters,
ignoring the expected output file name.

In conclusion, I feel that the behaviour of getopt can be relied on,
and it comes down to whether mkimage will ever process all values
provided by identical option arguments as a list. I personally don't
expect this to happen, certainly not for existing options.

Best,
Sander

> Best
> 
> Adrian
> 
> > 
> >  target/linux/ramips/image/mt7621.mk | 31 ++++++++++---------------
> > ----
> >  1 file changed, 10 insertions(+), 21 deletions(-)
> > 
> > diff --git a/target/linux/ramips/image/mt7621.mk
> > b/target/linux/ramips/image/mt7621.mk
> > index bb7e5a0d48..9b6c7d2311 100644
> > --- a/target/linux/ramips/image/mt7621.mk
> > +++ b/target/linux/ramips/image/mt7621.mk
> > @@ -7,21 +7,7 @@ include ./common-tp-link.mk  DEFAULT_SOC := mt7621
> > 
> >  KERNEL_DTB += -d21
> > -DEVICE_VARS += UIMAGE_MAGIC ELECOM_HWNAME LINKSYS_HWNAME
> > -
> > -# The OEM webinterface expects an kernel with initramfs which has
> > the
> > uImage -# header field ih_name.
> > -# We don't want to set the header name field for the kernel
> > include in the -
> > # sysupgrade image as well, as this image shouldn't be accepted by
> > the OEM
> > -# webinterface. It will soft-brick the board.
> > -define Build/custom-initramfs-uimage
> > -       mkimage -A $(LINUX_KARCH) \
> > -               -O linux -T kernel \
> > -               -C lzma -a $(KERNEL_LOADADDR) $(if
> > $(UIMAGE_MAGIC),-M
> > $(UIMAGE_MAGIC),) \
> > -               -e $(if
> > $(KERNEL_ENTRY),$(KERNEL_ENTRY),$(KERNEL_LOADADDR)) \
> > -               -n '$(1)' -d $@ $@.new
> > -       mv $@.new $@
> > -endef
> > +DEVICE_VARS += ELECOM_HWNAME LINKSYS_HWNAME
> > 
> >  define Build/elecom-wrc-gs-factory
> >         $(eval product=$(word 1,$(1)))
> > @@ -531,32 +517,35 @@ define Device/iodata_nand
> >    IMAGE/sysupgrade.bin := sysupgrade-tar | append-metadata  endef
> > 
> > +# The OEM webinterface expects an kernel with initramfs which has
> > the
> > +uImage # header field ih_name.
> > +# We don't want to set the header name field for the kernel
> > include in
> > +the # sysupgrade image as well, as this image shouldn't be
> > accepted by
> > +the OEM # webinterface. It will soft-brick the board.
> > +
> >  define Device/iodata_wn-ax1167gr2
> >    $(Device/iodata_nand)
> > -  UIMAGE_MAGIC := 0x434f4d42
> >    DEVICE_MODEL := WN-AX1167GR2
> >    KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
> > -       custom-initramfs-uimage 3.10(XBC.1)b10 | iodata-mstc-header
> > +       uImage lzma -M 0x434f4d42 -n '3.10(XBC.1)b10' | iodata-
> > mstc-header
> >    DEVICE_PACKAGES := kmod-mt7615e kmod-mt7615-firmware  endef
> > TARGET_DEVICES += iodata_wn-ax1167gr2
> > 
> >  define Device/iodata_wn-ax2033gr
> >    $(Device/iodata_nand)
> > -  UIMAGE_MAGIC := 0x434f4d42
> >    DEVICE_MODEL := WN-AX2033GR
> >    KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
> > -       custom-initramfs-uimage 3.10(VST.1)C10 | iodata-mstc-header
> > +       uImage lzma -M 0x434f4d42 -n '3.10(VST.1)C10' | iodata-
> > mstc-header
> >    DEVICE_PACKAGES := kmod-mt7603 kmod-mt7615e kmod-mt7615-
> > firmware  endef  TARGET_DEVICES += iodata_wn-ax2033gr
> > 
> >  define Device/iodata_wn-dx1167r
> >    $(Device/iodata_nand)
> > -  UIMAGE_MAGIC := 0x434f4d43
> >    DEVICE_MODEL := WN-DX1167R
> >    KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
> > -       custom-initramfs-uimage 3.10(XIK.1)b10 | iodata-mstc-header
> > +       uImage lzma -M 0x434f4d43 -n '3.10(XIK.1)b10' | iodata-
> > mstc-header
> >    DEVICE_PACKAGES := kmod-mt7615e kmod-mt7615-firmware  endef
> > TARGET_DEVICES += iodata_wn-dx1167r
> > --
> > 2.28.0
> > 
> > 
> > _______________________________________________
> > openwrt-devel mailing list
> > openwrt-devel@lists.openwrt.org
> > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff mbox series

Patch

diff --git a/target/linux/ramips/image/mt7621.mk b/target/linux/ramips/image/mt7621.mk
index bb7e5a0d48..9b6c7d2311 100644
--- a/target/linux/ramips/image/mt7621.mk
+++ b/target/linux/ramips/image/mt7621.mk
@@ -7,21 +7,7 @@  include ./common-tp-link.mk
 DEFAULT_SOC := mt7621
 
 KERNEL_DTB += -d21
-DEVICE_VARS += UIMAGE_MAGIC ELECOM_HWNAME LINKSYS_HWNAME
-
-# The OEM webinterface expects an kernel with initramfs which has the uImage
-# header field ih_name.
-# We don't want to set the header name field for the kernel include in the
-# sysupgrade image as well, as this image shouldn't be accepted by the OEM
-# webinterface. It will soft-brick the board.
-define Build/custom-initramfs-uimage
-	mkimage -A $(LINUX_KARCH) \
-		-O linux -T kernel \
-		-C lzma -a $(KERNEL_LOADADDR) $(if $(UIMAGE_MAGIC),-M $(UIMAGE_MAGIC),) \
-		-e $(if $(KERNEL_ENTRY),$(KERNEL_ENTRY),$(KERNEL_LOADADDR)) \
-		-n '$(1)' -d $@ $@.new
-	mv $@.new $@
-endef
+DEVICE_VARS += ELECOM_HWNAME LINKSYS_HWNAME
 
 define Build/elecom-wrc-gs-factory
 	$(eval product=$(word 1,$(1)))
@@ -531,32 +517,35 @@  define Device/iodata_nand
   IMAGE/sysupgrade.bin := sysupgrade-tar | append-metadata
 endef
 
+# The OEM webinterface expects an kernel with initramfs which has the uImage
+# header field ih_name.
+# We don't want to set the header name field for the kernel include in the
+# sysupgrade image as well, as this image shouldn't be accepted by the OEM
+# webinterface. It will soft-brick the board.
+
 define Device/iodata_wn-ax1167gr2
   $(Device/iodata_nand)
-  UIMAGE_MAGIC := 0x434f4d42
   DEVICE_MODEL := WN-AX1167GR2
   KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
-	custom-initramfs-uimage 3.10(XBC.1)b10 | iodata-mstc-header
+	uImage lzma -M 0x434f4d42 -n '3.10(XBC.1)b10' | iodata-mstc-header
   DEVICE_PACKAGES := kmod-mt7615e kmod-mt7615-firmware
 endef
 TARGET_DEVICES += iodata_wn-ax1167gr2
 
 define Device/iodata_wn-ax2033gr
   $(Device/iodata_nand)
-  UIMAGE_MAGIC := 0x434f4d42
   DEVICE_MODEL := WN-AX2033GR
   KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
-	custom-initramfs-uimage 3.10(VST.1)C10 | iodata-mstc-header
+	uImage lzma -M 0x434f4d42 -n '3.10(VST.1)C10' | iodata-mstc-header
   DEVICE_PACKAGES := kmod-mt7603 kmod-mt7615e kmod-mt7615-firmware
 endef
 TARGET_DEVICES += iodata_wn-ax2033gr
 
 define Device/iodata_wn-dx1167r
   $(Device/iodata_nand)
-  UIMAGE_MAGIC := 0x434f4d43
   DEVICE_MODEL := WN-DX1167R
   KERNEL_INITRAMFS := $(KERNEL_DTB) | loader-kernel | lzma | \
-	custom-initramfs-uimage 3.10(XIK.1)b10 | iodata-mstc-header
+	uImage lzma -M 0x434f4d43 -n '3.10(XIK.1)b10' | iodata-mstc-header
   DEVICE_PACKAGES := kmod-mt7615e kmod-mt7615-firmware
 endef
 TARGET_DEVICES += iodata_wn-dx1167r