<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">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">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" target="_blank">common-tp-link.mk</a> b/target/linux/ath79/image/<a href="http://common-tp-link.mk" rel="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" target="_blank">common-tp-link.mk</a><br>
+++ b/target/linux/ath79/image/<a href="http://common-tp-link.mk" rel="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" 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">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">ynezz@true.cz</a>> wrote:<br>
> > > Bruno Pena <<a href="mailto:brunompena@gmail.com" target="_blank">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>