[OpenWrt-Devel] [PATCH v2] fstools: Add support to read-only MTD partitions (eg. recovery images)
Steve Brown
sbrown at ewol.com
Wed Jan 22 06:21:12 EST 2020
Hi Bruno,
That fixed it.
[ 0.403826] m25p80 spi0.0: w25q128 (16384 Kbytes)
[ 0.408770] 7 fixed-partitions partitions found on MTD device spi0.0
[ 0.415333] Creating 7 MTD partitions on "spi0.0":
[ 0.420310] 0x000000000000-0x000000020000 : "factory-uboot"
[ 0.426739] 0x000000020000-0x000000040000 : "u-boot"
[ 0.432566] 0x000000040000-0x000000f00000 : "firmware"
[ 0.440477] 2 uimage-fw partitions found on MTD device firmware
[ 0.446595] Creating 2 MTD partitions on "firmware":
[ 0.451771] 0x000000000000-0x0000001b0000 : "kernel"
[ 0.457554] 0x0000001b0000-0x000000ec0000 : "rootfs"
[ 0.463355] mtd: device 4 (rootfs) set to be root filesystem
[ 0.470477] 1 squashfs-split partitions found on MTD device rootfs
[ 0.476875] 0x0000004e0000-0x000000ec0000 : "rootfs_data"
[ 0.483182] 0x000000f40000-0x000000f60000 : "info"
[ 0.488853] 0x000000f60000-0x000000fb0000 : "config"
[ 0.494619] 0x000000fc0000-0x000000fd0000 : "partition-table"
[ 0.501256] 0x000000ff0000-0x000001000000 : "art"
dev: size erasesize name
mtd0: 00020000 00010000 "factory-uboot"
mtd1: 00020000 00010000 "u-boot"
mtd2: 00ec0000 00010000 "firmware"
mtd3: 001b0000 00010000 "kernel"
mtd4: 00d10000 00010000 "rootfs"
mtd5: 009e0000 00010000 "rootfs_data"
mtd6: 00020000 00010000 "info"
mtd7: 00050000 00010000 "config"
mtd8: 00010000 00010000 "partition-table"
mtd9: 00010000 00010000 "art"
index 8aa6a5a2be..9048e8340c 100644
--- a/target/linux/ath79/image/common-tp-link.mk
+++ b/target/linux/ath79/image/common-tp-link.mk
@@ -71,7 +71,7 @@ endef
define Device/tplink-safeloader-uimage
$(Device/tplink-safeloader)
- KERNEL := kernel-bin | append-dtb | lzma | uImageArcher lzma
+ KERNEL := kernel-bin | append-dtb | lzma | uImageArcher lzma | pad-to 64k
endef
Steve
On Wed, 2020-01-22 at 10:22 +0100, Bruno Pena wrote:
> Hi Steve,
>
> I just noticed that I have just suggested you to change the wrong
> section... sorry!
> It's clearly stated on target/linux/ath79/image/generic-tp-link.mk
> that the A7-v5 uses the tplink-safeloader-uimage section but I
> somehow managed to copy the wrong one (doh!).
> define Device/tplink_archer-a7-v5
> $(Device/tplink-safeloader-uimage)
> SOC := qca9563
> [...]
>
> On the bright side, today I actually got some free minutes to spare
> and I used them to test some changes to the makefiles.
> I have added the following entries to get an overview of final
> partition size:
> IMAGES += kernel_1.bin kernel_2.bin kernel_3.bin kernel_4.bin
> kernel_5.bin kernel_6.bin
> IMAGE/kernel_1.bin := kernel-bin
> IMAGE/kernel_2.bin := kernel-bin | append-dtb
> IMAGE/kernel_3.bin := kernel-bin | append-dtb | lzma
> IMAGE/kernel_4.bin := kernel-bin | append-dtb | lzma |
> uImageArcher lzma
> IMAGE/kernel_5.bin := kernel-bin | append-dtb | lzma | pad-to 64k
> | uImageArcher lzma
> IMAGE/kernel_6.bin := kernel-bin | append-dtb | lzma |
> uImageArcher lzma | pad-to 64k
>
> And these were the results:
> kernel_1.bin : 1804719 bytes [0x1b89af]
> kernel_2.bin : 1813487 bytes [0x1babef]
> kernel_3.bin : 1831548 bytes [0x1bf27c]
> kernel_4.bin : 1831612 bytes [0x1bf2bc]
> kernel_5.bin : 1835072 bytes [0x1c0040]
> kernel_6.bin : 1835008 bytes [0x1c0000]
>
> I have then proceeded to generate the corresponding sysupgrade.bin
> image with the settings of kernel_5 and kernel_6 so I could manually
> inspect the resulting files.
>
> Original sysupgrade.bin (rootfs starts at 0x001b89af):
> 001b89a0 68 81 9d 3b bd c9 86 98 76 c4 f0 1e 0e 94 e0 68
> |h..;....v......h|
> 001b89b0 73 71 73 57 03 00 00 00 6e 27 5e 00 00 04 00 0f
> |sqsW....n'^.....|
> 001b89c0 00 00 00 04 00 12 00 c0 06 01 00 04 00 00 00 ff
> |................|
>
> Settings from kernel_5 sysupgrade.bin (rootfs starts at 0x001c0040):
> 001b89b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> |................|
> *
> 001c0040 68 73 71 73 57 03 00 00 00 6e 27 5e 00 00 04 00
> |hsqsW....n'^....|
>
> Settings from kernel_6 sysupgrade.bin (rootfs starts at 0x001c0000):
> 001b89b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> |................|
> *
> 001c0000 68 73 71 73 57 03 00 00 00 6e 27 5e 00 00 04 00
> |hsqsW....n'^....|
>
> Based on these I would ask if you could test again with the following
> change to target/linux/ath79/image/common-tp-link.mk (append "| pad-
> to 64k" to the KERNEL line):
> define Device/tplink-safeloader-uimage
> $(Device/tplink-safeloader)
> KERNEL := kernel-bin | append-dtb | lzma | uImageArcher lzma |
> pad-to 64k
> endef
>
> I would also like to take the opportunity to ask if it's worth
> pursuing this patch if it means that all devices (using mtd) will
> require their partitions to be padded to the blocksize?
>
> Thank you and best regards,
> Bruno Pena
>
> On Wed, Jan 22, 2020, 07:13 Bruno Pena <brunompena at gmail.com> wrote:
> > Hi Steve,
> >
> > Thank you for testing.
> > You implemented my suggestion correctly but it seems that it didn't
> > actually pad anything (the sizes of the partitions should be
> > rounded to the next 0x10000 block).
> > Please allow me a few days (real-life constraints) to test some
> > changes to the common-tp-link.mk before I waste more of your time
> > with these tests.
> >
> > Best regards,
> > Bruno Pena
> >
> > On Wed, Jan 22, 2020 at 12:16 AM Steve Brown <sbrown at ewol.com>
> > wrote:
> > > Hi Bruno,
> > >
> > > That didn't seem to solve the problem. The padding didn't seem to
> > > take
> > > effect.
> > >
> > > I reverted 0f81a0979 and 0c707d37b.
> > >
> > > dev: size erasesize name
> > > mtd0: 00020000 00010000 "factory-uboot"
> > > mtd1: 00020000 00010000 "u-boot"
> > > mtd2: 00ec0000 00010000 "firmware"
> > > mtd3: 001a3a07 00010000 "kernel"
> > > mtd4: 00d1c5f9 00010000 "rootfs"
> > > mtd5: 009f0000 00010000 "rootfs_data"
> > > mtd6: 00020000 00010000 "info"
> > > mtd7: 00050000 00010000 "config"
> > > mtd8: 00010000 00010000 "partition-table"
> > > mtd9: 00010000 00010000 "art"
> > >
> > > [ 0.414677] Creating 7 MTD partitions on "spi0.0":
> > > [ 0.419655] 0x000000000000-0x000000020000 : "factory-uboot"
> > > [ 0.426092] 0x000000020000-0x000000040000 : "u-boot"
> > > [ 0.431906] 0x000000040000-0x000000f00000 : "firmware"
> > > [ 0.439772] 2 uimage-fw partitions found on MTD device
> > > firmware
> > > [ 0.445891] Creating 2 MTD partitions on "firmware":
> > > [ 0.451065] 0x000000000000-0x0000001a3a07 : "kernel"
> > > [ 0.456840] 0x0000001a3a07-0x000000ec0000 : "rootfs"
> > > [ 0.462643] mtd: device 4 (rootfs) set to be root filesystem
> > > [ 0.469746] 1 squashfs-split partitions found on MTD device
> > > rootfs
> > > [ 0.476142] 0x0000004d0000-0x000000ec0000 : "rootfs_data"
> > > [ 0.482441] 0x000000f40000-0x000000f60000 : "info"
> > > [ 0.488033] 0x000000f60000-0x000000fb0000 : "config"
> > > [ 0.493856] 0x000000fc0000-0x000000fd0000 : "partition-table"
> > > [ 0.500494] 0x000000ff0000-0x000001000000 : "art"
> > >
> > >
> > > diff --git a/target/linux/ath79/image/common-tp-link.mk
> > > b/target/linux/ath79/image/common-tp-link.mk
> > > index 8aa6a5a2be..89e73a85f3 100644
> > > --- a/target/linux/ath79/image/common-tp-link.mk
> > > +++ b/target/linux/ath79/image/common-tp-link.mk
> > > @@ -63,7 +63,7 @@ endef
> > >
> > > define Device/tplink-safeloader
> > > $(Device/tplink)
> > > - KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header -O
> > > + KERNEL := kernel-bin | append-dtb | lzma | pad-to
> > > $$$$(BLOCKSIZE) | tplink-v1-header -O
> > > IMAGE/sysupgrade.bin := append-rootfs | tplink-safeloader
> > > sysupgrade | \
> > > append-metadata | check-size $$$$(IMAGE_SIZE)
> > > IMAGE/factory.bin := append-rootfs | tplink-safeloader factory
> > >
> > > Can you verify that I did this right?
> > >
> > > Steve
> > >
> > >
> > > On Tue, 2020-01-21 at 22:10 +0100, Bruno Pena wrote:
> > > > Hi everyone,
> > > >
> > > > I was finally able to replicate the issue you are observing.
> > > >
> > > > The problem comes from the fact that the kernel partition on
> > > the TP-
> > > > Link images is not padded to the write blocksize - which can be
> > > > observed on the dmesg from Steve:
> > > > [ 0.450987] 0x000000000000-0x0000001a39ea : "kernel"
> > > > [ 0.456772] 0x0000001a39ea-0x000000ec0000 : "rootfs"
> > > > The blocksize can be seen observed on the /proc/mtd and for
> > > that
> > > > device is 0x10000:
> > > > mtd3: 001a38de 00010000 "kernel"
> > > > mtd4: 00d1c722 00010000 "rootfs"
> > > > This triggers the following code on drivers/mtd/mtdpart.c that
> > > > removes the write flag from the rootfs partition:
> > > > tmp = part_absolute_offset(parent) + slave->offset;
> > > > remainder = do_div(tmp, wr_alignment);
> > > > if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {
> > > > /* Doesn't start on a boundary of major erase
> > > size */
> > > > slave->mtd.flags |= MTD_ERASE_PARTIAL;
> > > > if (((u32)slave->mtd.size) > parent->erasesize)
> > > > slave->mtd.flags &= ~MTD_WRITEABLE;
> > > > else
> > > > slave->mtd.erasesize = slave->mtd.size;
> > > > }
> > > >
> > > > tmp = part_absolute_offset(parent) + slave->offset +
> > > slave-
> > > > >mtd.size;
> > > > remainder = do_div(tmp, wr_alignment);
> > > > if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {
> > > > slave->mtd.flags |= MTD_ERASE_PARTIAL;
> > > >
> > > > if ((u32)slave->mtd.size > parent->erasesize)
> > > > slave->mtd.flags &= ~MTD_WRITEABLE;
> > > > else
> > > > slave->mtd.erasesize = slave->mtd.size;
> > > > }
> > > > Now we have a rootfs partition that is set to read-only, and
> > > with the
> > > > kernel patch that forces sub-partitions to match the access
> > > mode of
> > > > the parent, when the split runs it will force the rootfs_data
> > > > partition to match the parent access mode (read-only).
> > > >
> > > > My suggestion is to change the target/linux/ath79/image/common-
> > > tp-
> > > > link.mk so it pads the kernel partition to the blocksize
> > > (untested
> > > > suggestion below):
> > > > define Device/tplink-safeloader
> > > > $(Device/tplink)
> > > > KERNEL := kernel-bin | append-dtb | lzma | pad-to
> > > $$$$(BLOCKSIZE) |
> > > > tplink-v1-header -O
> > > > [...]
> > > > Would any of you be willing to spend some time testing this
> > > change on
> > > > your TP-Link?
> > > >
> > > > Thank you and best regards,
> > > > Bruno Pena
> > > >
> > > > On Tue, Jan 21, 2020 at 8:15 PM Bruno Pena <
> > > brunompena at gmail.com>
> > > > wrote:
> > > > > Hi Petr,
> > > > >
> > > > > Thank you for reverting the patches.
> > > > >
> > > > > I'm trying to replicate the issue but so far I haven't had
> > > any
> > > > > luck, and just by looking at the code I'm not seeing
> > > where/what is
> > > > > could be breaking.
> > > > >
> > > > > Regarding the upstream patch, that one is just an enabler
> > > (read: it
> > > > > only extends the "mtd_add_partition" function but it does not
> > > add
> > > > > any code to force the access mode on sub-partitions).
> > > > > The reason for this is because this enforcement is done on
> > > the mtd
> > > > > parser code that is OpenWrt specific and implemented via
> > > kernel
> > > > > patches (not present on upstream).
> > > > >
> > > > > Best regards,
> > > > > Bruno Pena
> > > > >
> > > > > On Tue, Jan 21, 2020 at 7:57 PM Petr Štetiar <ynezz at true.cz>
> > > wrote:
> > > > > > Bruno Pena <brunompena at gmail.com> [2020-01-21 14:53:54]:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > > Based on the last comment from Steve (fstools patch was
> > > not
> > > > > > reverted), I'm
> > > > > > > not sure if that's the root cause.
> > > > > >
> > > > > > you need to find out, but that Daniel's remark seems legit
> > > to me,
> > > > > > your patch
> > > > > > likely changed the mtd device open order/flags.
> > > > > >
> > > > > > > The kernel patch (which when reverted makes rootfs_data
> > > > > > writable again)
> > > > > > > only enforces the parent mtd access mode on the sub-
> > > partitions.
> > > > > >
> > > > > > FYI I currently dont have time to fix that regression
> > > myself and
> > > > > > since its
> > > > > > likely affecting a lot of users, so I've reverted those
> > > changes.
> > > > > > I think, that
> > > > > > this change is useful, so I'm still willing to merge it
> > > once
> > > > > > fixed, no
> > > > > > worries. Having some upstream feedback beforehand would be
> > > a
> > > > > > plus.
> > > > > >
> > > > > > BTW it would be fair to inform upstream about this possible
> > > issue
> > > > > > as well, so
> > > > > > the patch wont get merged in its current state, unless its
> > > double
> > > > > > checked (or
> > > > > > just write them, that you're planning v2, so the patch is
> > > removed
> > > > > > from their
> > > > > > Patchwork).
> > > > > >
> > > > > > -- ynezz
> > >
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list