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 <[email protected]> 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 <[email protected]> > > 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 < > > > [email protected]> > > > > 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 <[email protected]> > > > wrote: > > > > > > Bruno Pena <[email protected]> [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 [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
