Skip to content
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

[RFC/RFT] ath79: uboot-envtools: make package optional where possible, add missing devices #3557

Closed
wants to merge 9 commits into from

Conversation

Leo-PL
Copy link
Contributor

@Leo-PL Leo-PL commented Nov 2, 2020

Since most NOR-based devices (all currently supported save for one) do not require uboot-envtools during operation or upgrade, it's possible to make the package optional on them. The same is true for NOR-based Mikrotiks - all NAND devices are kept intact.
Primary use case for uboot-envtools package on NOR devices is hacking, so it can be enabled during build-time or even installed separately as package, if needed with kmod-mtd-rw, to override u-boot-env read-only lock.

After ensuring the package is set optional per profile, exceptions from specific device definitions are cleaned up.
Next, for consistency - all Ubiquiti devices have set u-boot-env partition as read-only for consistency, as it isn't used during bootup or upgrade on any of the devices, and only some of them had the protection.

Finally after this is done, it is possible to add missing configurations to the devices on which uboot-envtools is optional, as it won't increase their image size by default, and still be useful for hacking. However, they are based on device tree contents, and mostly untested, save for Ubiquiti XM devices, which I could test on my Nanobridge M5. And that's why PR is marked RFT and RFC.

Regarding NOR devices, I have one doubt regarding Qihoo C301, which is the only NOR device using uboot-envtools, to set boot count, which is likely required to pacify stock redundant boot mechanism. However in the device tree, u-boot-env partition is marked read-only, so call of fw_setenv in /etc/init.d/bootcount would likely fail anyway. @981213 could you please take a look at this? Maybe this can be dropped, or the other way - u-boot-env MTD partition may need to be set writable.

At least for now I kept the commits separate for easier reviewing - maybe the device tree cleanup can be done separately, as well as adding missing support.
This supersedes #3298, so closing that one.

Build tested on: ath79 (generic, mikrotik nand) - all devices.
Run tested on: TP-Link WDR4300 V1, TP-Link Archer C7 V2, TP-Link WR1043ND V1, Ubiquiti Nanobridge M5.

This partition isn't normally modified during boot process. Make it
read-only to prevent accidental overwrite.
If needed this can be overriden with installing kmod-mtd-rw; the same
way as for installing modified U-boot.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
This partition isn't normally modified during boot process. Make it
read-only to prevent accidental overwrite.
If needed this can be overriden with installing kmod-mtd-rw; the same
way as for installing modified U-boot.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
All currently supported Ubiquiti devices have read-only u-boot-env
partition, which itself isn't used during OpenWrt boot process.
Therefore uboot-envtools package isn't required - drop it to save a
few kB from images.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
All currently supported TP-Link devices lack separate U-boot
environment, nor is it used in OpenWrt boot process.
As uboot-envtools package isn't needed on them by default, drop it
to save a few kB from images.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
…rgets

Now that we have subtargets separated, we can drop uboot-envtools
package from generic and tiny subtargets, which don't really need this
package, except for hacking.
Only one device (Qihoo C301) uses this package, but it already depends
on it explicitly.

For mikrotik subtarget, add uboot-envtools to DEVICE_PACKAGES for
"mikrotik_nand" device class, as it is only needed there, and not on
whole subtarget.

If needed for hacking, it can be added manually during build time, or
intalled runtime. Possibly along with kmod-mtd-rw to unlock u-boot-env
partition, if write support is needed.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
Now that uboot-envtools is gone from DEFAULT_PACKAGES for this target,
drop all exceptions from device definitions.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
@981213
Copy link
Member

981213 commented Nov 2, 2020

@981213 could you please take a look at this? Maybe this can be dropped, or the other way - u-boot-env MTD partition may need to be set writable.

The bootcount isn't in the u-boot-env partition:

qihoo,c301)
	ubootenv_add_uci_config "/dev/mtd9" "0x0" "0x10000" "0x10000"
	;;

@adschm
Copy link
Member

adschm commented Nov 2, 2020

After having read your "motivation", I actually wonder whether uboot-envtools should be added as default package at all, at least target-wide. What would everybody think of not including uboot-envtools for the entire target, but just for particular devices needing it and/or nand subtarget ...?

One should read the whole thing before commenting ...

@adschm
Copy link
Member

adschm commented Nov 2, 2020

I like this, particularly the removal of "default" uboot-envtools (one could rearrange the patches there, will come back to this later).

I will pick the first two commits for now, and leave the PR for commenting ...

…W env

Some (mostly NOR-based) devices with R/W environment have had uboot-envtools
configuration missing, making it difficult to modify the environment if
needed. Now that this package is optional on ath79 (save for a few
devices) and won't clobber overlay if not selected, enable the configurations.

The configurations were deduced solely from respective device tree contents, so I
cannot fully guarantee correctness of operation on them.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
Some (All NOR-based) devices with RO environment have had uboot-envtools
configuration missing, making it difficult to modify the environment if
needed. Now that this package is optional on ath79 (save for a few
devices) and won't clobber overlay if not selected, enable the configurations.

The configurations were deduced solely from respective device tree contents, so I
cannot fully guarantee correctness of operation on them.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
All the devices fit the typical layout of 64kB 1-sector environment on
/dev/mtd1 and are read-only by default.

Signed-off-by: Lech Perczak <lech.perczak@gmail.com>
@Leo-PL Leo-PL force-pushed the ath79-uboot-envtools-cleanup branch from 4500a59 to 5d8b68b Compare November 3, 2020 00:02
@Leo-PL
Copy link
Contributor Author

Leo-PL commented Nov 3, 2020

@981213 could you please take a look at this? Maybe this can be dropped, or the other way - u-boot-env MTD partition may need to be set writable.

The bootcount isn't in the u-boot-env partition:

qihoo,c301)
	ubootenv_add_uci_config "/dev/mtd9" "0x0" "0x10000" "0x10000"
	;;

A very unusual design indeed, /dev/mtd9 resides on flash@1 node. Now I understand.

@blocktrron
Copy link
Member

@Leo-PL @adschm

uboot-envtools is required for the installation paths of multiple boards in the ath79, which removing would break. Especially ones where uboot-envtools is required being used from the initramfs image, where only the subtarget package-set is available.

So this is a NAK from my side for removing uboot-envtools by default.

@Leo-PL
Copy link
Contributor Author

Leo-PL commented Nov 3, 2020

@Leo-PL @adschm

uboot-envtools is required for the installation paths of multiple boards in the ath79, which removing would break. Especially ones where uboot-envtools is required being used from the initramfs image, where only the subtarget package-set is available.

So this is a NAK from my side for removing uboot-envtools by default.

Listing such boards would be a royal PITA, but I'll try anyway. @blocktrron do you have some examples on hand?
This way it could be kept in DEVICE_PACKAGES for ones that actually need them at this stage. I don't see a reason to keep this, for example, for most TP-link and Ubiquiti boards, which make up a lot of supported devices, which install via factory image.

@blocktrron
Copy link
Member

@Leo-PL

As stated in my prior commit, initramfs images will only contain the subtarget package-set, so adding it to the device-packages will not mitigate this issue.

All Riverbed boards are affected, as well as Aerohive ones. Other boards which have to circumvent dual-boot techniques implemented on the bootcmd level also use this to my knowledge. This applies to Datto boards for example.

FWIW, i don't see the benefit. The 24k build of the package consumes <10KiB when packed to squashfs, so breaking boards by "optimizing" others (even if their share is disproportional) isn't worth it from my perspective.

@adschm adschm added RFC pull request ready for comments RFT pull request ready for testing target/ath79 pull request/issue for ath79 target labels Nov 3, 2020
@Leo-PL
Copy link
Contributor Author

Leo-PL commented Nov 3, 2020

I fully agree that "we never break the userspace" is the top priority.
I'll think how this device-packages vs minimal-initramfs situation can be improved without breaking things. Too bad that I don't have devices you mentioned to test.

For sure I'd like to avoid maintaining list of exceptions in DEVICE_PACKAGES, like now, because it's going to be very painful. IMO, exceptions could be acceptable at most on a device class level, like "TP-link devices".

I think it's a good moment to split off a patchset providing previously-missing configurations for uboot-envtools, as it won't hurt to handle that one separately either.

Also, this <10k difference can sometimes make a bigger one, if it lands on an erase block boundary, for most ath79 devices still at 64k.

@Leo-PL
Copy link
Contributor Author

Leo-PL commented Nov 27, 2020

Lets's close this for now - I will open separate PRs for providing missing configuration and cleaning up the conditions for inclusion of the package in images.

@Leo-PL Leo-PL closed this Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC pull request ready for comments RFT pull request ready for testing target/ath79 pull request/issue for ath79 target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants