<div dir="auto"><div dir="auto">Hi Steve,</div><div dir="auto"><br></div><div dir="auto">I just noticed that I have just suggested you to change the wrong section... sorry!</div><div dir="auto">It's clearly stated on target/linux/ath79/image/<a href="http://generic-tp-link.mk">generic-tp-link.mk</a> that the A7-v5 uses the tplink-safeloader-uimage section but I somehow managed to copy the wrong one (doh!).</div><div dir="auto"> define Device/tplink_archer-a7-v5</div><div dir="auto"> $(Device/tplink-safeloader-uimage)</div><div dir="auto"> SOC := qca9563</div><div dir="auto"> [...]</div><div dir="auto"><br></div><div dir="auto">On the bright side, today I actually got some free minutes to spare and I used them to test some changes to the makefiles.</div><div dir="auto">I have added the following entries to get an overview of final partition size:</div><div dir="auto"> IMAGES += kernel_1.bin kernel_2.bin kernel_3.bin kernel_4.bin kernel_5.bin kernel_6.bin</div><div dir="auto"> IMAGE/kernel_1.bin := kernel-bin</div><div dir="auto"> IMAGE/kernel_2.bin := kernel-bin | append-dtb</div><div dir="auto"> IMAGE/kernel_3.bin := kernel-bin | append-dtb | lzma</div><div dir="auto"> IMAGE/kernel_4.bin := kernel-bin | append-dtb | lzma | uImageArcher lzma</div><div dir="auto"> IMAGE/kernel_5.bin := kernel-bin | append-dtb | lzma | pad-to 64k | uImageArcher lzma</div><div dir="auto"> IMAGE/kernel_6.bin := kernel-bin | append-dtb | lzma | uImageArcher lzma | pad-to 64k</div><div dir="auto"><br></div><div dir="auto">And these were the results:</div><div dir="auto"> kernel_1.bin : 1804719 bytes [0x1b89af]</div><div dir="auto"> kernel_2.bin : 1813487 bytes [0x1babef]</div><div dir="auto"> kernel_3.bin : 1831548 bytes [0x1bf27c]</div><div dir="auto"> kernel_4.bin : 1831612 bytes [0x1bf2bc]</div><div dir="auto"> kernel_5.bin : 1835072 bytes [0x1c0040]</div><div dir="auto"> kernel_6.bin : 1835008 bytes [0x1c0000]</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">Original sysupgrade.bin (rootfs starts at 0x001b89af):</div><div dir="auto"> 001b89a0 68 81 9d 3b bd c9 86 98 76 c4 f0 1e 0e 94 e0 68 |h..;....v......h|</div><div dir="auto"> 001b89b0 73 71 73 57 03 00 00 00 6e 27 5e 00 00 04 00 0f |sqsW....n'^.....|</div><div dir="auto"> 001b89c0 00 00 00 04 00 12 00 c0 06 01 00 04 00 00 00 ff |................|</div><div dir="auto"><br></div><div dir="auto">Settings from kernel_5 sysupgrade.bin (rootfs starts at 0x001c0040):</div><div dir="auto"> 001b89b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|</div><div dir="auto"> *</div><div dir="auto"> 001c0040 68 73 71 73 57 03 00 00 00 6e 27 5e 00 00 04 00 |hsqsW....n'^....|</div><div dir="auto"><br></div><div dir="auto">Settings from kernel_6 sysupgrade.bin (rootfs starts at 0x001c0000):</div><div dir="auto"> 001b89b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|</div><div dir="auto"> *</div><div dir="auto"> 001c0000 68 73 71 73 57 03 00 00 00 6e 27 5e 00 00 04 00 |hsqsW....n'^....|</div><div dir="auto"><br></div><div dir="auto">Based on these I would ask if you could test again with the following change to target/linux/ath79/image/<a href="http://common-tp-link.mk">common-tp-link.mk</a> (append "| pad-to 64k" to the KERNEL line):</div><div dir="auto"> define Device/tplink-safeloader-uimage</div><div dir="auto"> $(Device/tplink-safeloader)</div><div dir="auto"> KERNEL := kernel-bin | append-dtb | lzma | uImageArcher lzma | pad-to 64k</div><div dir="auto"> endef</div><div dir="auto"><br></div><div dir="auto">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?</div><div dir="auto"><br></div><div dir="auto">Thank you and best regards,</div><div dir="auto">Bruno Pena</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 22, 2020, 07:13 Bruno Pena <<a href="mailto:brunompena@gmail.com">brunompena@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Steve,<div><br></div><div>Thank you for testing.</div><div>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).</div><div>Please allow me a few days (real-life constraints) to test some changes to the <a href="http://common-tp-link.mk" target="_blank" rel="noreferrer">common-tp-link.mk</a> before I waste more of your time with these tests.</div><div><br></div><div>Best regards,</div><div>Bruno Pena</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">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>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Bruno,<br>
<br>
That didn't seem to solve the problem. The padding didn't seem to 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 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 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> 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 $$$$(BLOCKSIZE) | tplink-v1-header -O<br>
IMAGE/sysupgrade.bin := append-rootfs | tplink-safeloader 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 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 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 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 + 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 with the<br>
> kernel patch that forces sub-partitions to match the access 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-tp-<br>
> <a href="http://link.mk" rel="noreferrer noreferrer" target="_blank">link.mk</a> so it pads the kernel partition to the blocksize (untested<br>
> suggestion below):<br>
> define Device/tplink-safeloader<br>
> $(Device/tplink)<br>
> KERNEL := kernel-bin | append-dtb | lzma | pad-to $$$$(BLOCKSIZE) |<br>
> tplink-v1-header -O<br>
> [...]<br>
> Would any of you be willing to spend some time testing this 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 <<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 any<br>
> > luck, and just by looking at the code I'm not seeing where/what is<br>
> > could be breaking.<br>
> > <br>
> > Regarding the upstream patch, that one is just an enabler (read: it<br>
> > only extends the "mtd_add_partition" function but it does not add<br>
> > any code to force the access mode on sub-partitions).<br>
> > The reason for this is because this enforcement is done on the mtd<br>
> > parser code that is OpenWrt specific and implemented via 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>> 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 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 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-partitions.<br>
> > > <br>
> > > FYI I currently dont have time to fix that regression myself and<br>
> > > since its<br>
> > > likely affecting a lot of users, so I've reverted those changes.<br>
> > > I think, that<br>
> > > this change is useful, so I'm still willing to merge it once<br>
> > > fixed, no<br>
> > > worries. Having some upstream feedback beforehand would be a<br>
> > > plus.<br>
> > > <br>
> > > BTW it would be fair to inform upstream about this possible issue<br>
> > > as well, so<br>
> > > the patch wont get merged in its current state, unless its double<br>
> > > checked (or<br>
> > > just write them, that you're planning v2, so the patch is removed<br>
> > > from their<br>
> > > Patchwork).<br>
> > > <br>
> > > -- ynezz<br>
<br>
</blockquote></div>
</blockquote></div>