<div dir="ltr">Hi everyone,<div><br></div><div>I was finally able to replicate the issue you are observing.</div><div><br></div><div>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:</div><div><pre class="gmail-content" style="box-sizing:border-box;overflow:auto;font-family:Menlo,Monaco,Consolas,"Courier New",monospace;font-size:13px;padding:9.5px;margin-top:0px;margin-bottom:10px;line-height:14.3px;color:rgb(51,51,51);word-break:break-all;border:0px;border-radius:0px">[ 0.450987] 0x000000000000-0x0000001a39ea : "kernel"
[ 0.456772] 0x0000001a39ea-0x000000ec0000 : "rootfs"</pre></div><div>The blocksize can be seen observed on the /proc/mtd and for that device is 0x10000:</div><div><pre class="gmail-content" style="box-sizing:border-box;overflow:auto;font-family:Menlo,Monaco,Consolas,"Courier New",monospace;font-size:13px;padding:9.5px;margin-top:0px;margin-bottom:10px;line-height:14.3px;color:rgb(51,51,51);word-break:break-all;border:0px;border-radius:0px">mtd3: 001a38de 00010000 "kernel"
mtd4: 00d1c722 00010000 "rootfs"</pre></div><div>This triggers the following code on drivers/mtd/mtdpart.c that removes the write flag from the rootfs partition:<br></div><div><div><pre class="gmail-content" style="box-sizing:border-box;overflow:auto;font-family:Menlo,Monaco,Consolas,"Courier New",monospace;font-size:13px;padding:9.5px;margin-top:0px;margin-bottom:10px;line-height:14.3px;color:rgb(51,51,51);word-break:break-all;border:0px;border-radius:0px"> 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->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> }
</pre></div><div>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).</div></div><div><br></div><div>My suggestion is to change the target/linux/ath79/image/<a href="http://common-tp-link.mk">common-tp-link.mk</a> so it pads the kernel partition to the blocksize (untested suggestion below):</div><div><pre class="gmail-content" style="box-sizing:border-box;overflow:auto;font-family:Menlo,Monaco,Consolas,"Courier New",monospace;font-size:13px;padding:9.5px;margin-top:0px;margin-bottom:10px;line-height:14.3px;color:rgb(51,51,51);word-break:break-all;border:0px;border-radius:0px">define Device/tplink-safeloader<br> $(Device/tplink)<br> KERNEL := kernel-bin | append-dtb | lzma | pad-to $$$$(BLOCKSIZE) | tplink-v1-header -O<br>[...]<br></pre></div><div>Would any of you be willing to spend some time testing this change on your TP-Link?</div><div><br></div><div>Thank you and best regards,</div><div>Bruno Pena</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 21, 2020 at 8:15 PM Bruno Pena <<a href="mailto:brunompena@gmail.com">brunompena@gmail.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"><div dir="ltr">Hi Petr,<div><br></div><div>Thank you for reverting the patches.</div><div><br></div><div>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.</div><div><br></div><div>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).</div><div>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).</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 Tue, Jan 21, 2020 at 7:57 PM Petr Štetiar <<a href="mailto:ynezz@true.cz" target="_blank">ynezz@true.cz</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">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 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, your patch<br>
likely changed the mtd device open order/flags.<br>
<br>
> The kernel patch (which when reverted makes rootfs_data 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 since its<br>
likely affecting a lot of users, so I've reverted those changes. I think, that<br>
this change is useful, so I'm still willing to merge it once fixed, no<br>
worries. Having some upstream feedback beforehand would be a plus.<br>
<br>
BTW it would be fair to inform upstream about this possible issue as well, so<br>
the patch wont get merged in its current state, unless its double checked (or<br>
just write them, that you're planning v2, so the patch is removed from their<br>
Patchwork).<br>
<br>
-- ynezz<br>
</blockquote></div>
</blockquote></div>