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

Wed Nov 4 14:17:10 EST 2020

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 at lists.openwrt.org]
> > On Behalf Of Sander Vanheule
> > Sent: Mittwoch, 4. November 2020 10:21
> > To: openwrt-devel at lists.openwrt.org
> > Cc: Sander Vanheule <sander at 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 at 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 at lists.openwrt.org
> > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel