[PATCH RFC] kernel: mtd spi-nor: allow use of mixed erasesizes (E.g. 64K & 4K)
Robert Marko
robimarko at gmail.com
Sun Oct 11 14:45:00 EDT 2020
On Thu, 24 Sep 2020 at 00:04, John Thomson
<git at johnthomson.fastmail.com.au> wrote:
>
> Allow SPI NOR code to be configured to use the multiple erase-regions
> code path for a uniform erase-region device. This code path allows an
> erase request to select and run a list of erase operations (including 4K
> erases where supported by the device, and needed by the request). The
> multi erase-regions code path does additional checks before the erase
> starts, therefore may take slightly longer before the erase runs on device.
>
> Kernel config options:
> Introduces:
> CONFIG_MTD_SPI_NOR_USE_VARIABLE_ERASE=y
> Used with:
> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS=n
>
> Previous users of partial or 4K erase in Openwrt:
> - Mikrotik routerboot bootloader configuration
> target/linux/generic/files/drivers/platform/mikrotik/rb_softconfig.c
> Will work with this patch, with the 4K_SECTORS test removed.
> - RedBoot "FIS directory" partition scheme layout update
> package/system/mtd/src/fis.c
> Will need to be changed to use the size of the FIS directory partition,
> rather than the partition erasesize.
> The partial erase patchset set erasesize to partition size where
> partition boundary was not major erasesize aligned. This patch sets
> partition erasesize to minor erasesize only where the boundary is
> minor erasesize aligned.
>
> An erasesize_minor variable is added to the MTD struct mtd_info, so that
> the mtd level does not need access to the SPI NOR level to calculate the
> smallest (minor) universal erasesize. This is set in spi-nor.c device
> initialisation.
>
> mtdpart.c is modified to not prevent writing to partitions with a
> boundary on a minor eraseblock. As mtd partitions are currently a stack
> of structs, the erasesize_minor need to be copied (from parent) to each
> partition. MTD partitions use a list in kernel 5.7, but the erasesize
> boundary code remains the same.
>
> There appears to be a bug in spi-nor.c for univeral erase-region devices,
> in a code path that only this patch uses: A pointer to an address within
> a struct is not updated following a memcpy of the struct.
>
> Signed-off-by: John Thomson <git at johnthomson.fastmail.com.au>
>
> ---
>
> Replaces Openwrt generic patches:
> These cuts are not included in this patch to reduce RFC length.
> - partial erase 411 & 412
> - 4K limit 470 (non-functional on 5.4)
Hi, I have been following your work on this issue as it affects pretty
much all of Mikrotik devices with SPI-NOR.
I can test this on hAP ac2 (IPQ40xx), but can you provide all of the
patches needed as you mention partially removing 411 and 412 patches,
and removing 470 completely?
Regards
Robert
>
> Any suggestions welcome!
>
> Further discussion here: https://github.com/openwrt/openwrt/pull/3271
>
> WIP: Do not run this without a full NOR backup,
> and a means to restore it.
> E.g. SOIC8 clip? + single board computer with extra SPI.
>
> Run tested with Mikrotik soft_config writes on ath79 & ramips.
> Not tested on Redboot FIS layout update (fis code needs changes).
>
> An earlier patch which modified the default universal erase-region code
> path in spi-nor.c was sent RFC to linux-mtd, but received no comments.
>
> The update pointer to memcpy'd address patch has been recently sent RFC
> to linux-mtd and maintainers. My intention is that once this gets addressed,
> I send through the erasesize_minor and mtdpart changes.
>
> - I am not sure that testing if (wr_alignment_minor) is enough to check
> that the device has an erasesize_minor set?
> - I am not sure that the picked erasesize_minor would be universal for
> multi erase-regions devices.
>
> It is my opinion that code requesting an mtd_erase should manage the
> whole eraseblock. The partial erase patchset currently manages this in
> kernel, and a recently fixed merge bug in it was able to cause data loss
> and bootloader corruption on some Mikrotik and Redboot devices. In
> kernel 5.7, the mtd partition code is changed and would require a
> rewrite of the partial erase, as part_erase is no longer used.
>
> There may be devices that currently use partial erase which do not support 4K
> erase. They would lose functionaly with this patch.
>
> If there are any other user of 4K or partial erase, please let me know.
>
> Cheers!
> ---
> ...9-0-mtd-spi-nor-allow-variable-erase.patch | 172 ++++++++++++++++++
> 1 file changed, 172 insertions(+)
> create mode 100644 target/linux/generic/pending-5.4/499-0-mtd-spi-nor-allow-variable-erase.patch
>
> diff --git a/target/linux/generic/pending-5.4/499-0-mtd-spi-nor-allow-variable-erase.patch b/target/linux/generic/pending-5.4/499-0-mtd-spi-nor-allow-variable-erase.patch
> new file mode 100644
> index 0000000000..a7d727f2b0
> --- /dev/null
> +++ b/target/linux/generic/pending-5.4/499-0-mtd-spi-nor-allow-variable-erase.patch
> @@ -0,0 +1,172 @@
> +--- a/drivers/mtd/mtdpart.c
> ++++ b/drivers/mtd/mtdpart.c
> +@@ -331,8 +331,9 @@ static struct mtd_part *allocate_partiti
> + {
> + int wr_alignment = (parent->flags & MTD_NO_ERASE) ? parent->writesize :
> + parent->erasesize;
> ++ int wr_alignment_minor = parent->erasesize_minor;
> + struct mtd_part *slave;
> +- u32 remainder;
> ++ u32 remainder, remainder_minor;
> + char *name;
> + u64 tmp;
> +
> +@@ -433,6 +434,9 @@ static struct mtd_part *allocate_partiti
> + if (parent->_put_device)
> + slave->mtd._put_device = part_put_device;
> +
> ++ if (parent->erasesize_minor)
> ++ slave->mtd.erasesize_minor = parent->erasesize_minor;
> ++
> + slave->mtd._erase = part_erase;
> + slave->parent = parent;
> + slave->offset = part->offset;
> +@@ -523,21 +527,41 @@ static struct mtd_part *allocate_partiti
> +
> + tmp = part_absolute_offset(parent) + slave->offset;
> + 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);
> ++ if (remainder && wr_alignment_minor) {
> ++ tmp = part_absolute_offset(parent) + slave->offset;
> ++ remainder_minor = do_div(tmp, wr_alignment_minor);
> ++ }
> ++ if (slave->mtd.flags & MTD_WRITEABLE){
> ++ if (!remainder){
> ++ /* Does start on a boundary of major erase size */
> ++ } else if (wr_alignment_minor && !remainder_minor){
> ++ /* Does start on a boundary of minor erase size */
> ++ slave->mtd.erasesize = parent->erasesize_minor;
> ++ } else {
> ++ 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);
> ++ }
> + }
> +
> + 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);
> ++ if (remainder && wr_alignment_minor) {
> ++ tmp = part_absolute_offset(parent) +
> ++ slave->offset + slave->mtd.size;
> ++ remainder_minor = do_div(tmp, wr_alignment_minor);
> ++ }
> ++ if (slave->mtd.flags & MTD_WRITEABLE){
> ++ if (!remainder){
> ++ /* Does end on a boundary of major erase size */
> ++ } else if (wr_alignment_minor && !remainder_minor){
> ++ /* Does end on a boundary of minor erase size */
> ++ slave->mtd.erasesize = parent->erasesize_minor;
> ++ } else {
> ++ 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);
> ++ }
> + }
> +
> + mtd_set_ooblayout(&slave->mtd, &part_ooblayout_ops);
> +--- a/drivers/mtd/spi-nor/Kconfig
> ++++ b/drivers/mtd/spi-nor/Kconfig
> +@@ -24,6 +24,14 @@ config MTD_SPI_NOR_USE_4K_SECTORS
> + Please note that some tools/drivers/filesystems may not work with
> + 4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
> +
> ++config MTD_SPI_NOR_USE_VARIABLE_ERASE
> ++ bool "Disable uniform_erase"
> ++ default n
> ++ help
> ++ Return false with spi_nor_has_uniform_erase,
> ++ so that the multiple eraseregions code path is used.
> ++ This allows an erase to use 4K erase where supported and needed.
> ++
> + config SPI_ASPEED_SMC
> + tristate "Aspeed flash controllers in SPI mode"
> + depends on ARCH_ASPEED || COMPILE_TEST
> +--- a/drivers/mtd/spi-nor/spi-nor.c
> ++++ b/drivers/mtd/spi-nor/spi-nor.c
> +@@ -4346,6 +4346,7 @@ static int spi_nor_select_erase(struct s
> + {
> + struct spi_nor_erase_map *map = &nor->params.erase_map;
> + const struct spi_nor_erase_type *erase = NULL;
> ++ const struct spi_nor_erase_type *erase_minor = NULL;
> + struct mtd_info *mtd = &nor->mtd;
> + u32 wanted_size = nor->info->sector_size;
> + int i;
> +@@ -4378,15 +4379,20 @@ static int spi_nor_select_erase(struct s
> + */
> + for (i = SNOR_ERASE_TYPE_MAX - 1; i >= 0; i--) {
> + if (map->erase_type[i].size) {
> +- erase = &map->erase_type[i];
> +- break;
> ++ if (!erase)
> ++ erase = &map->erase_type[i];
> ++ erase_minor = &map->erase_type[i];
> + }
> + }
> +
> + if (!erase)
> + return -EINVAL;
> +
> ++ if (!erase_minor)
> ++ erase_minor = erase;
> ++
> + mtd->erasesize = erase->size;
> ++ mtd->erasesize_minor = erase_minor->size;
> + return 0;
> + }
> +
> +@@ -4522,12 +4528,20 @@ static void spi_nor_sfdp_init_params(str
> + struct spi_nor_flash_parameter sfdp_params;
> +
> + memcpy(&sfdp_params, &nor->params, sizeof(sfdp_params));
> ++ if (nor->params.erase_map.regions ==
> ++ &nor->params.erase_map.uniform_region)
> ++ sfdp_params.erase_map.regions = (
> ++ &sfdp_params.erase_map.uniform_region);
> +
> + if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> + nor->addr_width = 0;
> + nor->flags &= ~SNOR_F_4B_OPCODES;
> + } else {
> + memcpy(&nor->params, &sfdp_params, sizeof(nor->params));
> ++ if (sfdp_params.erase_map.regions ==
> ++ &sfdp_params.erase_map.uniform_region)
> ++ nor->params.erase_map.regions = (
> ++ &nor->params.erase_map.uniform_region);
> + }
> + }
> +
> +--- a/include/linux/mtd/mtd.h
> ++++ b/include/linux/mtd/mtd.h
> +@@ -205,6 +205,8 @@ struct mtd_info {
> + * information below if they desire
> + */
> + uint32_t erasesize;
> ++ /* "Minor" erase size supported by the whole device */
> ++ uint32_t erasesize_minor;
> + /* Minimal writable flash unit size. In case of NOR flash it is 1 (even
> + * though individual bits can be cleared), in case of NAND flash it is
> + * one NAND page (or half, or one-fourths of it), in case of ECC-ed NOR
> +--- a/include/linux/mtd/spi-nor.h
> ++++ b/include/linux/mtd/spi-nor.h
> +@@ -631,7 +631,11 @@ spi_nor_region_mark_overlay(struct spi_n
> +
> + static bool __maybe_unused spi_nor_has_uniform_erase(const struct spi_nor *nor)
> + {
> ++#ifdef CONFIG_MTD_SPI_NOR_USE_VARIABLE_ERASE
> ++ return false;
> ++#else
> + return !!nor->params.erase_map.uniform_erase_type;
> ++#endif
> + }
> +
> + static inline void spi_nor_set_flash_node(struct spi_nor *nor,
> --
> 2.28.0
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list