[OpenWrt-Devel] [PATCH v2] fstools: Add support to read-only MTD partitions (eg. recovery images)
Bruno Pena
brunompena at gmail.com
Wed Jan 22 06:46:34 EST 2020
Hi Steve,
Glad to hear that and thank you for testing it!
I will probably ask you to do one additional test to see if we're able to
remove the padding requirement (as mentioned on the previous emails).
But let me test it first so I can confirm that it actually works.
Best regards,
Bruno Pena
On Wed, Jan 22, 2020, 12:21 Steve Brown <sbrown at ewol.com> wrote:
> 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
> > > >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20200122/0156c984/attachment.htm>
-------------- next part --------------
_______________________________________________
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