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