-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ipq807x: change 3 devices to use the standard eMMC sysupgrade code #16505
Conversation
after this change only two
i could move them to |
Qnap 301W is not broken, I have it and it preserves settings just fine. That being said, If it can be moved to a generic sysupgrade EMMC function instead of the custom one I am all for it (Though I am not a fan of F2FS) |
ok. but how is it not broken? AFAICT, fstools creates the loop device rounding up the start address to 64 KiB, and openwrt/target/linux/qualcommax/ipq807x/base-files/lib/upgrade/mmc.sh Lines 53 to 59 in 38bb47c
so unless the rootfs itself is somehow padded to 64 KiB for this device, i don't understand how this could be working. |
me neither, but the powers that be have chosen F2FS for overlay. (i don't think that replacing the generic emmc upgrade code with a custom one for each device is a rational way of dealing with this.) btw, here is a PR related to this: openwrt/fstools#9 |
after looking into it a bit more, i'm willing to bet you are wrong. the device has a
but so these are symptoms of sysupgrade failing:
this happens because |
given that i understood why sysupgrade appeared to work until now, i added a commit here fixing the problem. since you own the device, please verify that there is a discreet thanks! |
ok, and i pushed a commit for the haze, the last device using mmc.sh, given there is evidence that this last one does not use a discreet rootfs_data: ba415af so mmc.sh is still broken but now it's unused, AFAICT. i want to delete it but i don't know who is referencing it at runtime! EDIT: oh well i suppose the .sh files are sourced with a glob. i nuked the file in the last commit. |
fec13a1
to
12ae683
Compare
I already wrote in #16452 (comment) that sysupgrade + preserving config is working fine (at least for 301w and the nbg7815) The only thing that might not work is sysupgrade and not preserving config since mmc_do_flash is always calling cp backup config.
BTW IMHO your change for the prpl,haze would break the sysupgrade since this device doesn't have a mmc rootfs_data partition and therefore needs the roofs_data loop. |
i already wrote a detailed reply to your comment here, which you seem to have completely ignored. in it i explain how sysupgrade is supposed to work. i also explained why "sysupgrade + preserving config is working fine" is not proof that sysupgrade is working. in the case of 301w and nbg7815, sysupgrade is not even touching rootfs_data, which is a big bug: config is "preserved", but not in the correct way. also, "The only thing that might not work" is very probably not the only thing not working. again, see my reply for more info.
with all due respect, you don't seem to undertand how sysupgrade works. no, you do not need a loop device during sysugrade. you do need it after booting though, which is the responsibility of fstools to create. and note that any loop device you might create during sysupgrade will cease to exist on reboot, as loop devices exist in kernel ram and are not persistent. again, refer to my previous reply to you, especially the section mentioning the WHW03 v1. EDIT: @kirdesde i noticed that i didn't specifically mentioned you in my previous reply to you. sorry if that caused you to miss the reply. |
12ae683
to
bb29b6f
Compare
@dangowrt was that an intention to merge? |
I have 301w. |
thank you! yes, please. you can confirm that the current 301w sysupgrade code does not work by running these tests with the current official openwrt firmware:
thanks again. |
For now I've been able to test only the first one because I had a newly compiled build form 2 hours ago. |
please only test running official openwrt both before and after the test. forks are not supported. |
i haven't heard back from you regarding: qnap 301W sysupgrade in the current official builds is very probably broken and should be tested like this: there is a commit here to fix it. thanks! |
test on prpl,haze
root@OpenWrtuned:~# ubus call system board
{
"kernel": "6.6.58",
"hostname": "OpenWrtuned",
"system": "ARMv8 Processor rev 4",
"model": "prpl Foundation Haze",
"board_name": "prpl,haze",
"rootfs_type": "squashfs",
"release": {
"distribution": "OpenWrt",
"version": "SNAPSHOT",
"description": "OpenWrt SNAPSHOT",
"revision": "r27934-ff18576f84",
"target": "qualcommax/ipq807x",
"builddate": "1730108090"
}
}
The
root@OpenWrt:~# ubus call system board
{
"kernel": "6.6.58",
"hostname": "OpenWrt",
"system": "ARMv8 Processor rev 4",
"model": "prpl Foundation Haze",
"board_name": "prpl,haze",
"rootfs_type": "squashfs",
"release": {
"distribution": "OpenWrt",
"version": "SNAPSHOT",
"description": "OpenWrt SNAPSHOT",
"revision": "r27934-ff18576f84",
"target": "qualcommax/ipq807x",
"builddate": "1730108090"
}
} sysupgrade wiped settings ✅ |
thanks! unfortunately these tests were intended for qnap 301w, which uses a discrete rootfs_data partition for the overlay. these tests never applied to haze. haze is different and uses the trailing part of rootfs for the overlay. on haze the expectation was that sysupgrade would possibly always clear settings even if you wanted to keep them, because the update code does not align the loop device to 64KB boundaries. however your test shows that settings are kept. i suspect this means that rootfs is generated in such a way that causes it to always be a multiple of 64KB in size? idk whatever the reason, even if it worked, this is brittle. and haze should be migrated to the standard way of doing things anyway, which this commit does, even if it is currently working. my point is that i'm pretty sure qnap 301w does not work, and the others need to be migrated to the standard code even if they work. @robimarko has a qnap 301w but unfortunately he hasn't tested it. EDIT: spectrum sax1v1k was in exactly the same boat as qnap 301w, and sure as hell did not work before 3b221ba; i tested it on my sax1v1k. |
bb29b6f
to
69c2cc0
Compare
@@ -308,7 +308,7 @@ define Device/prpl_haze | |||
DEVICE_DTS_CONFIG := config@hk09 | |||
SOC := ipq8072 | |||
DEVICE_PACKAGES := ath11k-firmware-qcn9074 ipq-wifi-prpl_haze kmod-ath11k-pci \ | |||
mkf2fs f2fsck kmod-fs-f2fs kmod-leds-lp5562 | |||
kmod-fs-f2fs f2fs-tools kmod-leds-lp5562 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I miss description/reasoning about this change in the commit. If its unrelated, move it into separate commit with proper description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description/reasoning: mkf2fs, f2fsck are part of metapackage f2fs-tools. f2fs-tools will be extended/adapted as f2fs devels see fit, instead of stagnating in the past. including the metapackage is the standard practice on other devices in openwrt, AFAICT.
If its unrelated, move it into separate commit with proper description
i fixed the device i own. i don't have this device, i just wanted to help when i identified issues in other devices. you are more than welcome to take over these changes if you favor particular styles. in any case, thanks for the comment.
@@ -1,83 +0,0 @@ | |||
# | |||
# Copyright (C) 2016 lede-project.org |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go into separate commit as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change was lumped in this commit because it was the last device using this non-standard code, assuming the previous 2 commits were merged.
I agree and I actually like it, but I'm only able to test this on Haze, but if it works, I'm going to merge only Haze relevant commits, unless you're willing to split this into Haze only pull request. I'm not going to merge the other bits as I don't want to cause any regressions as I wont be able to handle them in timely manner. |
a bit of history: this PR was about the first commit only, affecting only nbg7815, as you can see in the original PR message. later i saw that there were 2 other similar devices with issues and added commits 2 and 3. but nbg7815 was already tested and the issue confirmed when i created the fix and this PR (again see OP, it details who tested). so you can definitely merge the first commit, and with your own fix to Haze, only qnap 301w would remain broken. |
@robimarko is probably very busy. |
it helps a lot, thank you for testing. i was wrong! i don't have the 301w, and when researching it, i came across what i now think might have been false info. as stated here, i was under the assumption that the 301w had a discrete now i doubt that that info is true. if the 301w had a rootfs_data, it would be failing in the same way as the spectrum sax1v1k did before i fixed it. your tests clearly show sysupgrade works, and make me think there is no discreet rootfs_data. i sincerely apologize for the mistake. under the new assumption, 301w would still benefit from a change to the new way of doing things. but my proposed commit is wrong and will break sysupgrade if merged. @sppmasterspp could you please install package |
Is this info enough?
I've added this info to the QNAP 301w wiki. |
Thanks! Rebased on top of main and merged! |
Thanks for digging in and fixing these issues |
thank you!! |
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Also note that the qnap 301w has a 'rootfs_data' partition in the eMMC that is being ignored by fstools during boot, presumably due to a bug. This is why the partition is also ignored in the sysupgrade code and there is no definition of CI_DATAPART="rootfs_data". Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Alphabetically sort devices in platform.sh Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
All ipq807x devices that were using the legacy 'mmc_do_upgrade' eMMC sysupgrade code were ported to the replacement 'emmc_do_upgrade' code. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Also note that the qnap 301w has a 'rootfs_data' partition in the eMMC that is being ignored by fstools during boot, presumably due to a bug. This is why the partition is also ignored in the sysupgrade code and there is no definition of CI_DATAPART="rootfs_data". Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Alphabetically sort devices in platform.sh Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
All ipq807x devices that were using the legacy 'mmc_do_upgrade' eMMC sysupgrade code were ported to the replacement 'emmc_do_upgrade' code. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
@Lanchon Can you give here for reference the commands to restore the original files from the archives to partition 9 using gzip and tar. |
or else but before you must make sure that partition p9 is unmounted!!! use restoring the tar backup requires formatting and mounting partition p9 beforehand. as backups go, it is less useful. but it allows easier access to the file contents from the backup file, if ever needed. the tar backup, strictly speaking, is redundant. |
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit 5583d23)
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Also note that the qnap 301w has a 'rootfs_data' partition in the eMMC that is being ignored by fstools during boot, presumably due to a bug. This is why the partition is also ignored in the sysupgrade code and there is no definition of CI_DATAPART="rootfs_data". Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit fe481c9)
Alphabetically sort devices in platform.sh Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit 17f84bb)
All ipq807x devices that were using the legacy 'mmc_do_upgrade' eMMC sysupgrade code were ported to the replacement 'emmc_do_upgrade' code. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit 4911212)
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit 5583d23) Link: openwrt#17097 Signed-off-by: Petr Štetiar <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Also note that the qnap 301w has a 'rootfs_data' partition in the eMMC that is being ignored by fstools during boot, presumably due to a bug. This is why the partition is also ignored in the sysupgrade code and there is no definition of CI_DATAPART="rootfs_data". Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit fe481c9) Link: openwrt#17097 Signed-off-by: Petr Štetiar <[email protected]>
Alphabetically sort devices in platform.sh Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit 17f84bb) Link: openwrt#17097 Signed-off-by: Petr Štetiar <[email protected]>
All ipq807x devices that were using the legacy 'mmc_do_upgrade' eMMC sysupgrade code were ported to the replacement 'emmc_do_upgrade' code. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]> (cherry picked from commit 4911212) Link: openwrt#17097 Signed-off-by: Petr Štetiar <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Also note that the qnap 301w has a 'rootfs_data' partition in the eMMC that is being ignored by fstools during boot, presumably due to a bug. This is why the partition is also ignored in the sysupgrade code and there is no definition of CI_DATAPART="rootfs_data". Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Alphabetically sort devices in platform.sh Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
All ipq807x devices that were using the legacy 'mmc_do_upgrade' eMMC sysupgrade code were ported to the replacement 'emmc_do_upgrade' code. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Note that the old ad-hoc method did not explicitly align backup data to 64 KiB boundaries. Also note that the qnap 301w has a 'rootfs_data' partition in the eMMC that is being ignored by fstools during boot, presumably due to a bug. This is why the partition is also ignored in the sysupgrade code and there is no definition of CI_DATAPART="rootfs_data". Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Alphabetically sort devices in platform.sh Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
All ipq807x devices that were using the legacy 'mmc_do_upgrade' eMMC sysupgrade code were ported to the replacement 'emmc_do_upgrade' code. Signed-off-by: Rodrigo Balerdi <[email protected]> Link: openwrt#16505 Signed-off-by: Robert Marko <[email protected]>
Settings were lost because target-specific
mmc_do_upgrade
does not align backup data to 64 KiB boundaries.This changes the device to use the generic
emmc_do_upgrade
andemmc_copy_config
.i don't have this device, but it seems all devices using
mmc_do_upgrade
should be affected.@pwned-pixel did the testing and reportedly this change fixes sysupgrade: settings are preserved while random files in the overlay (files not explicitly backed up) are deleted.
@dangowrt @robimarko
thank you!