Ramips sunvalley, Permit Larger sysupgrade

arrmo · August 18, 2021, 2:36am

Hi,

Related to PR #4410, but a thought on allowing sysupgrade to be larger (as the flash partitions are using mtd-concat, sysupgrade should support a larger size than TFTP ... right @xabolcs???).

To not break TFTP images, which will typically (or can be ) built with less packages / additions, but once up ... allow sysupgrade to be larger, make use of the full combined (concatenated) flash partition size, how about this slight change to the Device definition?

define Device/sunvalley_filehub_common
  SOC := mt7620n
  IMAGE_SIZE := 6144k
  SYSUPGRADE_SIZE := 7744k
  DEVICE_PACKAGES := kmod-usb2 kmod-usb-ohci kmod-i2c-ralink
  LOADER_TYPE := bin
  LOADER_FLASH_OFFS := 0x200000
  COMPILE := loader-$(1).bin
  COMPILE/loader-$(1).bin := loader-okli-compile | pad-to 64k | lzma | \
        uImage lzma
  KERNEL := $(KERNEL_DTB) | uImage lzma -M 0x4f4b4c49
  KERNEL_INITRAMFS := $(KERNEL_DTB) | uImage lzma
  IMAGES += kernel.bin rootfs.bin
  IMAGE/kernel.bin := append-loader-okli $(1) | check-size 64k
  IMAGE/rootfs.bin := $$(sysupgrade_bin) | check-size
  IMAGE/sysupgrade.bin := append-kernel | append-rootfs | pad-rootfs | \
        check-size 7744k | append-metadata
endef

I tried to replace the hard coded 7744k with $(SYSUPGRADE_SIZE) ... but it's not working for some reason - likely me . But I did try this build - and it does work => allows sysupgrade up to 7744k, but if rootfs.bin tries to cross 6144k ... nope! This is using the already existing check-size argument (i.e. it can be passed a value, if not it defaults to IMAGE_SIZE).

Thoughts?

Thanks!

xabolcs · August 18, 2021, 6:07am

As you can see, this slight change isn't enough.
Re-read my comments on the PR, and you will know why this slight change would never work!

Why do you assume that these images (TFTP recovery, initramfs, sysupgrade) have different package sets?
They use the same kernel and rootfs! Just the packaging is different!

arrmo · August 18, 2021, 11:45am

Hmmm ... OK, let me re-read. I did twice already, perhaps third time is a charm ... . This doesn't touch anything but sysupgrade (agreed?), and allows it to be written out by sysupgrade (verified that does work). But I'll recheck again, thanks.

To your note earlier, about the smaller size of the stock builds stored in (OpenWrt) downloads. Thinking that folks aren't normally wanting to TFTP in a large build, rather load a smaller one that doesn't require all sorts of special tinkering - as sysupgrade allows larger sizes later. In other words - don't add grief, just for the recovery situation.

Thanks!

arrmo · August 20, 2021, 7:43pm

I apologize, but I'm just not quite seeing the issue here. With the (latest) updates, I'm not touching anything except sysupgrade (i.e. the same size limit still applies, except for sysupgrade). And I just checked the upgrade files on the router (that I never touched ),

upgrade/platform.sh
PART_NAME=firmware

OK, good, so it writes sysupgrade to firmware. And,

cat /proc/mtd
mtd8: 00790000 00001000 "firmware"

So the size of the partition that sysupgrade is writing to is 0x00790000.

I admit, not sure why it's an issue then to increase ONLY the sysupgrade file size.

Sorry if I'm just being thick!

bobafetthotmail · August 20, 2021, 8:32pm

Yes this should work, you are changing sysupgrade image limits only while leaving tftp image limits in place.

bobafetthotmail · August 20, 2021, 8:39pm

He is increasing the size limit only for the sysupgrade image,

  IMAGE/sysupgrade.bin := append-kernel | append-rootfs | pad-rootfs | \
        check-size 7744k | append-metadata

Afaik the firmware upgrade is done from inside OpenWrt and it is using mtd devices not hardcoded hardware addresses like the TFTP, so it should flash jumping over the factory partition because that's what the mtd partition does, or am I wrong?

Other devices did a similar thing where factory firmware image has different limits from sysupgrade.

see this commit https://github.com/openwrt/openwrt/commit/2a5460b11b9718ed5f398fcc381edc89bce72695

Why do you assume that these images (TFTP recovery, initramfs, sysupgrade) have different package sets?

If you are building an image from source or from ImageBuilder to flash on a device that is running OpenWrt already, do you care if the image is too big for TFTP? You are not installing from TFTP.

He wants to be able to make a bigger sysupgrade image for this device because he is integrating packages in his custom image.

arrmo · August 20, 2021, 8:51pm

Yes, that's exactly right! And sysupgrade writes to the partition named "firmware" - which is concatenated, so larger (allowing to include integrated packages).

Thanks!

arrmo · August 20, 2021, 8:53pm

BTW, to make sure, I also checked (after "larger" sysupgrade file), and the u-boot-env partition (from below) - it's fine, not damaged in any way

mtd4: 00170000 00001000 "fwconcat1"
mtd5: 00010000 00001000 "u-boot-env"
mtd6: 00020000 00001000 "fwconcat2"
mtd7: 00600000 00001000 "fwconcat0"
mtd8: 00790000 00001000 "firmware"

bobafetthotmail · August 20, 2021, 8:54pm

and here is the sysupgrade script, it's calling the standard default_do_upgrade function for this device and most others https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/mt7620/base-files/lib/upgrade/platform.sh
which is defined here

github.com

openwrt/openwrt/blob/master/package/base-files/files/lib/upgrade/common.sh#L288


      
          
          				[ "$type" = "00000000000000000000000000000000" ] && continue
          
          				printf "%2d %5d %7d\n" $part $lba $num >> "/tmp/partmap.$filename"
          			done
          		} || {
          			for part in 1 2 3 4; do
          				set -- $(hexdump -v -n 12 -s "$((0x1B2 + $part * 16))" -e '3/4 "0x%08X "' "$disk")
          
          				local type="$(( $(hex_le32_to_cpu $1) % 256))"
          				local lba="$(( $(hex_le32_to_cpu $2) ))"
          				local num="$(( $(hex_le32_to_cpu $3) ))"
          
          				[ $type -gt 0 ] || continue
          
          				printf "%2d %5d %7d\n" $part $lba $num >> "/tmp/partmap.$filename"
          			done
          		}
          	fi
          }

so yes it is working with mtd partitions so if you use mtdsplit like in this device the mtd partition is already "joined" and written as if it was one without overwriting things in the middle.

arrmo · August 20, 2021, 9:00pm

Agreed. I tried to say that above, but not as well . Thanks for digging in to this, much appreciated!

xabolcs · August 22, 2021, 7:03am

You aren't thick!
You are just on the wrong path!

Did you test what happens when the image explodes?
Do you know what it means when your slight change makes it way to master and the 6144k size check would kick in?

dd if=/openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7620/root.squashfs >> /openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7620/tmp/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin
9362+1 records in
9362+1 records out
4793351 bytes (4.8 MB, 4.6 MiB) copied, 0.0599641 s, 79.9 MB/s
/openwrt/staging_dir/host/bin/padjffs2 /openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7620/tmp/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin  4 8 16 64 128 256
padding image to 00658000
padding image to 00660000
padding image to 00680000
WARNING: Image file /openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7620/tmp/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin is too big: 6815748 > 6291456
cp /openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7620/tmp/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin /openwrt/bin/targets/ramips/mt7620/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin
cp: cannot stat '/openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7620/tmp/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin': No such file or directory
make[5]: [Makefile:206: /openwrt/bin/targets/ramips/mt7620/openwrt-ramips-mt7620-ravpower_rp-wd03-squashfs-rootfs.bin] Error 1 (ignored)

The above doesn't looks good at all.

bobafetthotmail · August 22, 2021, 2:29pm

Why that is bad? The TFTP images are checked for the right size and fail if too big. This is correct, this way the user cannot destroy his device by flashing too big images.

The sysupgrade image where the size limit is different should still be generated correctly.

See the current commit in the PR https://github.com/openwrt/openwrt/pull/4410/commits/60ac15d770f8dc28744217b5a97b1f3d8ee675f8

he just added this to override default sysupgrade image size checks.

  IMAGE/sysupgrade.bin := append-kernel | append-rootfs | pad-rootfs | \
       check-size 7744k | append-metadata

So even if the TFTP images fail it is still going to be generated

xabolcs · August 22, 2021, 2:44pm

Because it's not upstreamable (to OpenWrt trunk).
It's a nice change in an own fork, but not suitable for OpenWrt master.

bobafetthotmail · August 22, 2021, 2:47pm

why not upstreamable. He is not increasing image size, he is increasing only sysupgrade image size LIMIT.

As I said in posts above, other devices have already different size limits in different image types.

xabolcs · August 22, 2021, 3:01pm

Let's say:

21.02 is out
this change arrives at master
some stuff also arrives at master and inflates the image between rootfs checksize (6144k) and sysupgrade checksize (7744k)
major OpenWrt release happens

At this point the only OEM installation method is gone due to the missing rootfs. .

bobafetthotmail · August 22, 2021, 3:27pm

That 6144k check is already in OpenWrt. https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/image/mt7620.mk#L498

The OEM installation will be gone if the rootfs becomes bigger than this limit. This does not change. This always happens.

Without his change, these are the events.

21.02 is out
some stuff also arrives at master and inflates the image higher than rootfs checksize (6144k)
major OpenWrt release happens
At this point the only OEM installation method is gone due to the missing rootfs.

This is not different at all. The same thing happens. Why is his change not upstreamable.

xabolcs · August 22, 2021, 4:04pm

Without this change the sysupgrade fades away also: the new release is not usable explicitly.

Because it doesn't make any sense, value for OpenWrt developers, IMHO:

introduces an invisible change: the OEM installation would stop working at some point in the future for these devices (when would you update the Wiki?)
these are 8/32 devices
they will fade away (at least: source-only) before this image inflation would happen
if the minimal image hits 6144k then these devices would have a very bad time running it

A better solution would be to explicitly remove those kernel.bin and rootfs.bin files (and keep sysupgrade.bin only) with a detailed commit message, how to install from OEM.

The best solution (IMHO) would be to improve those kernel.bin and rootfs.bin recipes to allow 7680k image space.

(I'm not an OpenWrt developer.)

bobafetthotmail · August 22, 2021, 4:45pm

Older OpenWrt images and packages are still available in download servers and will be for a long time. LEDE 17.xx release is still available in download servers.
Just install old OpenWrt image that fits, and then do a sysupgrade with new release.

But this is not why he is making the change, see below

The OEM installation will always stop working in the future. His change is not making it stop working.

We can update it right now with the information for future users. Written text in the wiki does not go away.

I think you do not understand why he wants this. He is not increasing the size to make this device last longer.

He is increasing the size because he is making a custom image with many packages that are NOT in the default image.
So his image RIGHT NOW is too big for TFTP but can do sysupgrade fine.
This can be done with Image Builder https://openwrt.org/docs/guide-user/additional-software/imagebuilder or if you compile from source, it is a supported official tool for a official OpenWrt feature.
There are also web interfaces for servers that offer this function to people that are not able to run Image Builder on their PC (as it still requires Linux system) https://openwrt.org/docs/guide-developer/imagebuilder_frontends

You select all packages you want and you create a larger firmware image with all packages integrated. This allows to save a lot of space because packages are put in the /rom partition that is squashfs compressed, and avoids the need to reinstall packages every time you upgrade the firmware.

As I said, other devices have similar limits for sysupgrade image and it is allowed, because integrating packages to make your own customized image is an official OpenWrt feature https://github.com/openwrt/openwrt/commit/2a5460b11b9718ed5f398fcc381edc89bce72695

This is not a recipe problem and OpenWrt cannot fix this. The limitation is in the device bootloader and OpenWrt is not changing the bootloader. The device bootloader is used for TFTP install and it does not know that OpenWrt is using two partitions as one skipping a partition in the middle.
Bootloader is stupid, it will just flash the image from starting flash address to end of file. This overwrites all blocks, it does not skip the partition like OpenWrt does.

The only way for TFTP images to work is to be smaller than bootloader limits. Period. Can be necessary to make a TFTP image with different default packages so it is smaller and it can fit, or just tell users to use older OpenWrt version for first installation and then do a sysupgrade to latest version.

orangepizza · August 22, 2021, 4:51pm

Why fight on forum for if openwrt devs will accept this or not. Just post this patch on openwrt-devel mailing list and see the result

bobafetthotmail · August 22, 2021, 4:55pm

They won't even look at it if someone is screaming NAK, posting meme images and arguing that this does not work.

This is what happened on the PR on github https://github.com/openwrt/openwrt/pull/4410 and that's why the OP came here to discuss it

Admittedly, the original change in the PR was wrong and he realized only later that he should have changed the size ONLY for sysupgrade image and not for all images.

Now the change is correct, and there is no risk of erasing wrong flash partitions in the device, but the other person is still not convinced.

Developers have no time for drama, if they see drama they close the page