diff mbox series

[OpenWrt-Devel] kernel: drop downstream support for mtd unaligned writes

Message ID 20200309114302.14383-1-zajec5@gmail.com
State Rejected
Delegated to: Rafał Miłecki
Headers show
Series [OpenWrt-Devel] kernel: drop downstream support for mtd unaligned writes | expand

Commit Message

Rafał Miłecki March 9, 2020, 11:43 a.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

This code was developed 10+ years ago to support few specific devices.
It doesn't match current mtd architecture and is probably/hopefully not
needed anymore.

Problems of this code with recent kernels:
1. It assumes that parent mtd boundaries always allow accessing whole
   block data. It was true only for master MTD device and non-tree
   partitions.
2. MTD_ERASE_PARTIAL affects all partition blocks - even those fully
   accessible in a standard way. This code was probably designed for
   tiny partitions only.
3. It seems broken with subpartitions. Unintended usage of this code
   triggered by run_parsers_by_type() rewrite caused in erasing content
   of some unrelated partitions. It may be caused by incorrect offsets
   calculation.

It seems this code was originally needed by some Gateworks devices from
Avila and Cambria families using RedBoot. Those devices were supported
by ixp4xx target which was dropped recently. It's also likely that new
ixp4xx devices didn't require that code at all. Some recent examples:

1. Gateworks Avila GW2342:
0x000000000000-0x000000040000 : "RedBoot"
0x000000040000-0x000000160000 : "linux"
0x000000160000-0x000000fc0000 : "rootfs"
0x000000360000-0x000000fc0000 : "rootfs_data"
0x000000fc0000-0x000000fc1000 : "RedBoot config"
0x000000fe0000-0x000001000000 : "FIS directory"

2. Gateworks Cambria GW2350:
0x000000000000-0x000000080000 : "RedBoot"
0x000000080000-0x000000180000 : "linux"
0x000000180000-0x000001fe0000 : "rootfs"
0x000000320000-0x000001fe0000 : "rootfs_data"
0x000001fe0000-0x000001fff000 : "FIS directory"
0x000001fff000-0x000002000000 : "RedBoot config"

All above partitions seem to be aligned properly.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../411-mtd-partial_eraseblock_write.patch    | 131 ------------------
 .../412-mtd-partial_eraseblock_unlock.patch   |  40 ------
 .../411-mtd-partial_eraseblock_write.patch    | 131 ------------------
 .../412-mtd-partial_eraseblock_unlock.patch   |  40 ------
 4 files changed, 342 deletions(-)
 delete mode 100644 target/linux/generic/pending-4.19/411-mtd-partial_eraseblock_write.patch
 delete mode 100644 target/linux/generic/pending-4.19/412-mtd-partial_eraseblock_unlock.patch
 delete mode 100644 target/linux/generic/pending-5.4/411-mtd-partial_eraseblock_write.patch
 delete mode 100644 target/linux/generic/pending-5.4/412-mtd-partial_eraseblock_unlock.patch

Comments

Tomasz Maciej Nowak March 12, 2020, 4:32 p.m. UTC | #1 
Hi Rafał.

W dniu 09.03.2020 o 12:43, Rafał Miłecki pisze:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This code was developed 10+ years ago to support few specific devices.
> It doesn't match current mtd architecture and is probably/hopefully not
> needed anymore.

Unfortunately it is needed for currently supported devices if we want to
provide sysupgrade images. The popular example would be Ubiquiti RouterStation
(Pro). It has "FIS directory" partition and "RedBoot config" on one erase
block. Also greping the tree reveals that other target use RedBoot, ath25,
bcm63xx and gemini. If that issue affects them? I don't know, since RedBoot can
be compiled with both of these partitions on one erase block or separate.

> 
> Problems of this code with recent kernels:
> 1. It assumes that parent mtd boundaries always allow accessing whole
>    block data. It was true only for master MTD device and non-tree
>    partitions.
> 2. MTD_ERASE_PARTIAL affects all partition blocks - even those fully
>    accessible in a standard way. This code was probably designed for
>    tiny partitions only.
> 3. It seems broken with subpartitions. Unintended usage of this code
>    triggered by run_parsers_by_type() rewrite caused in erasing content
>    of some unrelated partitions. It may be caused by incorrect offsets
>    calculation.

This is probably the cause of FS#2428[1] bug. Because of it I disabled
sysupgrade images[2]. I'm fine with dropping these patches, since now they
cause only problems. Would be nice if someone could write newer/better
implementation of that feature, gladly will test any implementation.

1. https://bugs.openwrt.org/index.php?do=details&task_id=2428
2. https://git.openwrt.org/cc5256a8bfa0bd5fff5ff42e6b2febea011e1c59
diff mbox series

Patch

diff --git a/target/linux/generic/pending-4.19/411-mtd-partial_eraseblock_write.patch b/target/linux/generic/pending-4.19/411-mtd-partial_eraseblock_write.patch
deleted file mode 100644
index f3a314ae02..0000000000
--- a/target/linux/generic/pending-4.19/411-mtd-partial_eraseblock_write.patch
+++ /dev/null
@@ -1,131 +0,0 @@ 
-From: Felix Fietkau <nbd@nbd.name>
-Subject: mtd: implement write support for partitions covering only a part of an eraseblock (buffer data that would otherwise be erased)
-
-lede-commit: 87a8e8ac1067f58ba831c4aae443f3655c31cd80
-Signed-off-by: Felix Fietkau <nbd@nbd.name>
----
- drivers/mtd/mtdpart.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++-----
- include/linux/mtd/mtd.h |  4 +++
- 2 files changed, 85 insertions(+), 9 deletions(-)
-
---- a/drivers/mtd/mtdpart.c
-+++ b/drivers/mtd/mtdpart.c
-@@ -36,6 +36,8 @@
- #include "mtdcore.h"
- #include "mtdsplit/mtdsplit.h"
- 
-+#define MTD_ERASE_PARTIAL	0x8000 /* partition only covers parts of an erase block */
-+
- /* Our partition linked list */
- static LIST_HEAD(mtd_partitions);
- static DEFINE_MUTEX(mtd_partitions_mutex);
-@@ -220,6 +222,53 @@ static int part_erase(struct mtd_info *m
- {
- 	struct mtd_part *part = mtd_to_part(mtd);
- 	int ret;
-+	size_t wrlen = 0;
-+	u8 *erase_buf = NULL;
-+	u32 erase_buf_ofs = 0;
-+	bool partial_start = false;
-+
-+	if (mtd->flags & MTD_ERASE_PARTIAL) {
-+		size_t readlen = 0;
-+		u64 mtd_ofs;
-+
-+		erase_buf = kmalloc(part->parent->erasesize, GFP_ATOMIC);
-+		if (!erase_buf)
-+			return -ENOMEM;
-+
-+		mtd_ofs = part->offset + instr->addr;
-+		erase_buf_ofs = do_div(mtd_ofs, part->parent->erasesize);
-+
-+		if (erase_buf_ofs > 0) {
-+			instr->addr -= erase_buf_ofs;
-+			ret = mtd_read(part->parent,
-+				instr->addr + part->offset,
-+				part->parent->erasesize,
-+				&readlen, erase_buf);
-+
-+			instr->len += erase_buf_ofs;
-+			partial_start = true;
-+		} else {
-+			mtd_ofs = part->offset + part->mtd.size;
-+			erase_buf_ofs = part->parent->erasesize -
-+				do_div(mtd_ofs, part->parent->erasesize);
-+
-+			if (erase_buf_ofs > 0) {
-+				instr->len += erase_buf_ofs;
-+				ret = mtd_read(part->parent,
-+					part->offset + instr->addr +
-+					instr->len - part->parent->erasesize,
-+					part->parent->erasesize, &readlen,
-+					erase_buf);
-+			} else {
-+				ret = 0;
-+			}
-+		}
-+		if (ret < 0) {
-+			kfree(erase_buf);
-+			return ret;
-+		}
-+
-+	}
- 
- 	instr->addr += part->offset;
- 	ret = part->parent->_erase(part->parent, instr);
-@@ -227,6 +276,24 @@ static int part_erase(struct mtd_info *m
- 		instr->fail_addr -= part->offset;
- 	instr->addr -= part->offset;
- 
-+	if (mtd->flags & MTD_ERASE_PARTIAL) {
-+		if (partial_start) {
-+			part->parent->_write(part->parent,
-+				instr->addr, erase_buf_ofs,
-+				&wrlen, erase_buf);
-+			instr->addr += erase_buf_ofs;
-+		} else {
-+			instr->len -= erase_buf_ofs;
-+			part->parent->_write(part->parent,
-+				instr->addr + instr->len,
-+				erase_buf_ofs, &wrlen,
-+				erase_buf +
-+				part->parent->erasesize -
-+				erase_buf_ofs);
-+		}
-+		kfree(erase_buf);
-+	}
-+
- 	return ret;
- }
- 
-@@ -539,19 +606,22 @@ static struct mtd_part *allocate_partiti
- 	remainder = do_div(tmp, wr_alignment);
- 	if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {
- 		/* Doesn't start on a boundary of major erase size */
--		/* FIXME: Let it be writable if it is on a boundary of
--		 * _minor_ erase size though */
--		slave->mtd.flags &= ~MTD_WRITEABLE;
--		printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase/write block boundary -- force read-only\n",
--			part->name);
-+		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_WRITEABLE;
--		printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase/write block -- force read-only\n",
--			part->name);
-+		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;
- 	}
- 
- 	mtd_set_ooblayout(&slave->mtd, &part_ooblayout_ops);
diff --git a/target/linux/generic/pending-4.19/412-mtd-partial_eraseblock_unlock.patch b/target/linux/generic/pending-4.19/412-mtd-partial_eraseblock_unlock.patch
deleted file mode 100644
index a54603a0f8..0000000000
--- a/target/linux/generic/pending-4.19/412-mtd-partial_eraseblock_unlock.patch
+++ /dev/null
@@ -1,40 +0,0 @@ 
-From: Tim Harvey <tharvey@gateworks.com>
-Subject: mtd: allow partial block unlock
-
-This allows sysupgrade for devices such as the Gateworks Avila/Cambria
-product families based on the ixp4xx using the redboot bootloader with
-combined FIS directory and RedBoot config partitions on larger FLASH
-devices with larger eraseblocks.
-
-This second iteration of this patch addresses previous issues:
-- whitespace breakage fixed
-- unlock in all scenarios
-- simplification and fix logic bug
-
-[john@phrozen.org: this should be moved to the ixp4xx folder]
-
-Signed-off-by: Tim Harvey <tharvey@gateworks.com>
----
- drivers/mtd/mtdpart.c | 11 ++++++++++-
- 1 file changed, 10 insertions(+), 1 deletion(-)
-
---- a/drivers/mtd/mtdpart.c
-+++ b/drivers/mtd/mtdpart.c
-@@ -306,7 +306,16 @@ static int part_lock(struct mtd_info *mt
- static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
- {
- 	struct mtd_part *part = mtd_to_part(mtd);
--	return part->parent->_unlock(part->parent, ofs + part->offset, len);
-+
-+	ofs += part->offset;
-+
-+	if (mtd->flags & MTD_ERASE_PARTIAL) {
-+		/* round up len to next erasesize and round down offset to prev block */
-+		len = (mtd_div_by_eb(len, part->parent) + 1) * part->parent->erasesize;
-+		ofs &= ~(part->parent->erasesize - 1);
-+	}
-+
-+	return part->parent->_unlock(part->parent, ofs, len);
- }
- 
- static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
diff --git a/target/linux/generic/pending-5.4/411-mtd-partial_eraseblock_write.patch b/target/linux/generic/pending-5.4/411-mtd-partial_eraseblock_write.patch
deleted file mode 100644
index b46c3f5ed4..0000000000
--- a/target/linux/generic/pending-5.4/411-mtd-partial_eraseblock_write.patch
+++ /dev/null
@@ -1,131 +0,0 @@ 
-From: Felix Fietkau <nbd@nbd.name>
-Subject: mtd: implement write support for partitions covering only a part of an eraseblock (buffer data that would otherwise be erased)
-
-lede-commit: 87a8e8ac1067f58ba831c4aae443f3655c31cd80
-Signed-off-by: Felix Fietkau <nbd@nbd.name>
----
- drivers/mtd/mtdpart.c   | 90 ++++++++++++++++++++++++++++++++++++++++++++-----
- include/linux/mtd/mtd.h |  4 +++
- 2 files changed, 85 insertions(+), 9 deletions(-)
-
---- a/drivers/mtd/mtdpart.c
-+++ b/drivers/mtd/mtdpart.c
-@@ -22,6 +22,8 @@
- #include "mtdcore.h"
- #include "mtdsplit/mtdsplit.h"
- 
-+#define MTD_ERASE_PARTIAL	0x8000 /* partition only covers parts of an erase block */
-+
- /* Our partition linked list */
- static LIST_HEAD(mtd_partitions);
- static DEFINE_MUTEX(mtd_partitions_mutex);
-@@ -206,6 +208,53 @@ static int part_erase(struct mtd_info *m
- {
- 	struct mtd_part *part = mtd_to_part(mtd);
- 	int ret;
-+	size_t wrlen = 0;
-+	u8 *erase_buf = NULL;
-+	u32 erase_buf_ofs = 0;
-+	bool partial_start = false;
-+
-+	if (mtd->flags & MTD_ERASE_PARTIAL) {
-+		size_t readlen = 0;
-+		u64 mtd_ofs;
-+
-+		erase_buf = kmalloc(part->parent->erasesize, GFP_ATOMIC);
-+		if (!erase_buf)
-+			return -ENOMEM;
-+
-+		mtd_ofs = part->offset + instr->addr;
-+		erase_buf_ofs = do_div(mtd_ofs, part->parent->erasesize);
-+
-+		if (erase_buf_ofs > 0) {
-+			instr->addr -= erase_buf_ofs;
-+			ret = mtd_read(part->parent,
-+				instr->addr + part->offset,
-+				part->parent->erasesize,
-+				&readlen, erase_buf);
-+
-+			instr->len += erase_buf_ofs;
-+			partial_start = true;
-+		} else {
-+			mtd_ofs = part->offset + part->mtd.size;
-+			erase_buf_ofs = part->parent->erasesize -
-+				do_div(mtd_ofs, part->parent->erasesize);
-+
-+			if (erase_buf_ofs > 0) {
-+				instr->len += erase_buf_ofs;
-+				ret = mtd_read(part->parent,
-+					part->offset + instr->addr +
-+					instr->len - part->parent->erasesize,
-+					part->parent->erasesize, &readlen,
-+					erase_buf);
-+			} else {
-+				ret = 0;
-+			}
-+		}
-+		if (ret < 0) {
-+			kfree(erase_buf);
-+			return ret;
-+		}
-+
-+	}
- 
- 	instr->addr += part->offset;
- 	ret = part->parent->_erase(part->parent, instr);
-@@ -213,6 +262,24 @@ static int part_erase(struct mtd_info *m
- 		instr->fail_addr -= part->offset;
- 	instr->addr -= part->offset;
- 
-+	if (mtd->flags & MTD_ERASE_PARTIAL) {
-+		if (partial_start) {
-+			part->parent->_write(part->parent,
-+				instr->addr, erase_buf_ofs,
-+				&wrlen, erase_buf);
-+			instr->addr += erase_buf_ofs;
-+		} else {
-+			instr->len -= erase_buf_ofs;
-+			part->parent->_write(part->parent,
-+				instr->addr + instr->len,
-+				erase_buf_ofs, &wrlen,
-+				erase_buf +
-+				part->parent->erasesize -
-+				erase_buf_ofs);
-+		}
-+		kfree(erase_buf);
-+	}
-+
- 	return ret;
- }
- 
-@@ -525,19 +592,22 @@ static struct mtd_part *allocate_partiti
- 	remainder = do_div(tmp, wr_alignment);
- 	if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {
- 		/* Doesn't start on a boundary of major erase size */
--		/* FIXME: Let it be writable if it is on a boundary of
--		 * _minor_ erase size though */
--		slave->mtd.flags &= ~MTD_WRITEABLE;
--		printk(KERN_WARNING"mtd: partition \"%s\" doesn't start on an erase/write block boundary -- force read-only\n",
--			part->name);
-+		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_WRITEABLE;
--		printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase/write block -- force read-only\n",
--			part->name);
-+		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;
- 	}
- 
- 	mtd_set_ooblayout(&slave->mtd, &part_ooblayout_ops);
diff --git a/target/linux/generic/pending-5.4/412-mtd-partial_eraseblock_unlock.patch b/target/linux/generic/pending-5.4/412-mtd-partial_eraseblock_unlock.patch
deleted file mode 100644
index 348fb9a842..0000000000
--- a/target/linux/generic/pending-5.4/412-mtd-partial_eraseblock_unlock.patch
+++ /dev/null
@@ -1,40 +0,0 @@ 
-From: Tim Harvey <tharvey@gateworks.com>
-Subject: mtd: allow partial block unlock
-
-This allows sysupgrade for devices such as the Gateworks Avila/Cambria
-product families based on the ixp4xx using the redboot bootloader with
-combined FIS directory and RedBoot config partitions on larger FLASH
-devices with larger eraseblocks.
-
-This second iteration of this patch addresses previous issues:
-- whitespace breakage fixed
-- unlock in all scenarios
-- simplification and fix logic bug
-
-[john@phrozen.org: this should be moved to the ixp4xx folder]
-
-Signed-off-by: Tim Harvey <tharvey@gateworks.com>
----
- drivers/mtd/mtdpart.c | 11 ++++++++++-
- 1 file changed, 10 insertions(+), 1 deletion(-)
-
---- a/drivers/mtd/mtdpart.c
-+++ b/drivers/mtd/mtdpart.c
-@@ -292,7 +292,16 @@ static int part_lock(struct mtd_info *mt
- static int part_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
- {
- 	struct mtd_part *part = mtd_to_part(mtd);
--	return part->parent->_unlock(part->parent, ofs + part->offset, len);
-+
-+	ofs += part->offset;
-+
-+	if (mtd->flags & MTD_ERASE_PARTIAL) {
-+		/* round up len to next erasesize and round down offset to prev block */
-+		len = (mtd_div_by_eb(len, part->parent) + 1) * part->parent->erasesize;
-+		ofs &= ~(part->parent->erasesize - 1);
-+	}
-+
-+	return part->parent->_unlock(part->parent, ofs, len);
- }
- 
- static int part_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)