Fix UBIFS recovery issues observed on NOR flash

PATCH In review Anatolij Gustschin 3 participants 22 messages Dec 16, 2010 – Jan 6, 2011 1 message not in the archive

Subsystems (3)

Memory technology devices (MTD) NAND flash Unsorted block images (UBI)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 292342500.2538.65.camel@localhost

Anatolij Gustschin

author

2010-12-16 22:42 UTC #1 lore raw

v2 cover Fix UBIFS recovery issues observed on NOR flash

This v2 patch series is the next attempt to fix UBIFS recovery issues on NOR flash partitions. Previously only one patch has been submitted. Now, as requested by Artem, the patch is split into several patches and additionally the setting of writebufsize for NAND/OneNAND flash is also done. Comments from Artem to the previous patch have been addressed, too.

Anatolij Gustschin (6):
mtd: add writebufsize field to mtd_info struct
mtd: cfi: add writebufsize initialization
mtd: nand: add mtd->writebufsize initialization
mtd: onenand: add mtd->writebufsize initialization
mtd: initialize writebufsize in the MTD object of a partition
UBI: use mtd->writebufsize to set minimal I/O unit size

 drivers/mtd/chips/cfi_cmdset_0001.c |    1 +
 drivers/mtd/chips/cfi_cmdset_0002.c |    4 ++++
 drivers/mtd/chips/cfi_cmdset_0020.c |    1 +
 drivers/mtd/mtdconcat.c             |    1 +
 drivers/mtd/mtdpart.c               |    1 +
 drivers/mtd/nand/nand_base.c        |    1 +
 drivers/mtd/onenand/onenand_base.c  |    1 +
 drivers/mtd/ubi/build.c             |   19 ++++++++++++++++++-
 include/linux/mtd/mtd.h             |   13 +++++++++++++
 9 files changed, 41 insertions(+), 1 deletions(-)

Anatolij Gustschin

author

2010-12-16 22:42 UTC #2 lore raw

PATCH v2 1/6 mtd: add writebufsize field to mtd_info struct

In reply to a message not in the archive

This field will be used to indicate the write buffer size of the MTD device. UBI will set it's minimal I/O unit size (min_io_size) to the indicated write buffer size. By this change we intend to fix failed recovery of UBIFS partitions we currently observe on NOR flash when mounting the partition after unclean unmount.

Currently the min_io_size is set to mtd->writesize (which is 1 byte for NOR flash). But flash programming is often done from prepared write buffer containing multiple bytes and is performed in one programming operation which could be interrupted by a power cut or a system reset causing corrupted (partially written) areas in a flash sector. Knowing the size of potentially corrupted areas UBIFS scanning and recovery algorithms are able to perform successful recovery.

In case of NOR flash minimal I/O size must be equal to the maximal size of the write buffer used by embedded flash programming algorithm. In case of NAND flash mtd->writebufsize should be equivalent to mtd->writesize.

The subsequent patches will add mtd->writebufsize initialization where needed.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 include/linux/mtd/mtd.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

include/linux/mtd/mtd.h +13 -0

index fe8d77e..3c6db0c 100644
@@ -144,6 +144,19 @@ struct mtd_info {
 	 */
 	uint32_t writesize;
 
+	/*
+	 * Size of the write buffer used by the MTD.
+	 * MTD devices having a write buffer can write multiple writesize
+	 * chunks at a time. E.g. while writing 4 * writesize bytes to a
+	 * device with 2 * writesize bytes buffer the MTD driver can (but
+	 * doesn't have to) do 2 writesize operations, but not 4.
+	 * In case of NAND flash writebufsize should be equivalent to
+	 * writesize. In case of NOR flash writebufsize is the size of the
+	 * write buffer used by the MTD device to program multiple bytes
+	 * at a time (in one programming operation).
+	 */
+	uint32_t writebufsize;
+
 	uint32_t oobsize;   // Amount of OOB data per block (e.g. 16)
 	uint32_t oobavail;  // Available OOB bytes per block

Signature · 1 line

1.7.1

Artem Bityutskiy

2010-12-19 17:45 UTC #3 lore raw

Re: [PATCH v2 1/6] mtd: add writebufsize field to mtd_info struct

Quoted 21 lines · Anatolij Gustschin

This field will be used to indicate the write buffer size of the MTD device. UBI will set it's minimal I/O unit size (min_io_size) to the indicated write buffer size. By this change we intend to fix failed recovery of UBIFS partitions we currently observe on NOR flash when mounting the partition after unclean unmount.
Currently the min_io_size is set to mtd->writesize (which is 1 byte for NOR flash). But flash programming is often done from prepared write buffer containing multiple bytes and is performed in one programming operation which could be interrupted by a power cut or a system reset causing corrupted (partially written) areas in a flash sector. Knowing the size of potentially corrupted areas UBIFS scanning and recovery algorithms are able to perform successful recovery.
In case of NOR flash minimal I/O size must be equal to the maximal size of the write buffer used by embedded flash programming algorithm. In case of NAND flash mtd->writebufsize should be equivalent to mtd->writesize.

Quoted 4 lines

The subsequent patches will add mtd->writebufsize initialization where needed.
Signed-off-by Anatolij Gustschin <[email protected]>

I've tweaked your commentary a bit and pushed to l2-mtd-2.6.git, please, check.

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Anatolij Gustschin

author

2010-12-20 09:18 UTC #4 lore raw

Re: [PATCH v2 1/6] mtd: add writebufsize field to mtd_info struct

Quoted 2 lines · On Sun, 19 Dec 2010 19:45:31 +0200 Artem Bityutskiy <[email protected]> wrote:

I've tweaked your commentary a bit and pushed to l2-mtd-2.6.git, please, check.

It is ok. Thanks!

Anatolij

Anatolij Gustschin

author

2010-12-16 22:42 UTC #5 lore raw

PATCH v2 2/6 mtd: cfi: add writebufsize initialization

In reply to a message not in the archive

Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 drivers/mtd/chips/cfi_cmdset_0001.c |    1 +
 drivers/mtd/chips/cfi_cmdset_0002.c |    4 ++++
 drivers/mtd/chips/cfi_cmdset_0020.c |    1 +
 3 files changed, 6 insertions(+), 0 deletions(-)

drivers/mtd/chips/cfi_cmdset_0001.c +1 -0

index 44cbfc0..a8c3e1c 100644
@@ -455,6 +455,7 @@ struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
+	mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;
 
 	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;

drivers/mtd/chips/cfi_cmdset_0002.c +4 -0

index 9d68ab9..54dbd28 100644
@@ -428,6 +428,10 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
+	mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "MTD %s(): write buffer size %d\n",
+		__func__, mtd->writebufsize);
 
 	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;

drivers/mtd/chips/cfi_cmdset_0020.c +1 -0

index 314af1f..c04b765 100644
@@ -238,6 +238,7 @@ static struct mtd_info *cfi_staa_setup(struct map_info *map)
 	mtd->resume = cfi_staa_resume;
 	mtd->flags = MTD_CAP_NORFLASH & ~MTD_BIT_WRITEABLE;
 	mtd->writesize = 8; /* FIXME: Should be 0 for STMicro flashes w/out ECC */
+	mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;
 	map->fldrv = &cfi_staa_chipdrv;
 	__module_get(THIS_MODULE);
 	mtd->name = map->name;

Signature · 1 line

1.7.1

Artem Bityutskiy

2010-12-19 17:46 UTC #6 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

Quoted 4 lines · Anatolij Gustschin

Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.
Signed-off-by Anatolij Gustschin <[email protected]>

Pushed patches 2-5 as is.

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Guillaume LECERF

2010-12-20 16:37 UTC #7 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

In reply to Anatolij Gustschin

Hello.

2010/12/16 Anatolij Gustschin <[email protected]>:

drivers/mtd/chips/cfi_cmdset_0002.c struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) · line 431

Quoted 12 lines

Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.
Signed-off-by Anatolij Gustschin <[email protected]>
---
drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
@@ -428,6 +428,10 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
        mtd->flags   = MTD_CAP_NORFLASH;
        mtd->name    = map->name;
        mtd->writesize = 1;
+       mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;

What about wbufsize initialized in cfi_amdstd_write_buffers() to "cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize" ?
I don't know this piece of code, but I just wanted to point it to you.

Signature · 2 lines

Guillaume LECERF
GeeXboX developer - www.geexbox.org

Artem Bityutskiy

2010-12-21 14:46 UTC #8 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

Quoted 6 lines · ext Guillaume LECERF

Hello.
2010/12/16 Anatolij Gustschin <[email protected]>:
Quoted 3 lines
Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.

Quoted 13 lines

drivers/mtd/chips/cfi_cmdset_0002.c struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) · line 431
Quoted 9 lines
Signed-off-by Anatolij Gustschin <[email protected]>
---
drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
@@ -428,6 +428,10 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
       mtd->flags   = MTD_CAP_NORFLASH;
       mtd->name    = map->name;
       mtd->writesize = 1;
+       mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;
What about wbufsize initialized in cfi_amdstd_write_buffers() to "cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize" ?
I don't know this piece of code, but I just wanted to point it to you.

Yes, fair question, Anatolij?

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Guillaume LECERF

2010-12-30 10:32 UTC #9 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

2010/12/21 Artem Bityutskiy <[email protected]>:

Quoted 1 line

Yes, fair question, Anatolij?

Hi all.

Anatolij, what about such patches ?
Note : this is totally untested ;)

Signature · 2 lines

Guillaume LECERF
GeeXboX developer - www.geexbox.org

[v2,2/6] mtd: cfi: add writebufsize initialization

From: Anatolij Gustschin <[email protected]>

Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.

Signed-off-by Anatolij Gustschin <[email protected]>

Signed-off-by Guillaume LECERF <[email protected]>

---
 drivers/mtd/chips/cfi_cmdset_0001.c |    1 +
 drivers/mtd/chips/cfi_cmdset_0002.c |    4 ++++
 drivers/mtd/chips/cfi_cmdset_0020.c |    1 +
 3 files changed, 6 insertions(+), 0 deletions(-)

drivers/mtd/chips/cfi_cmdset_0001.c +1 -0

index 44cbfc0..178f87b 100644
@@ -455,6 +455,7 @@ struct mtd_info *cfi_cmdset_0001(struct map_info *map, int primary)
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
+	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 
 	mtd->reboot_notifier.notifier_call = cfi_intelext_reboot;

drivers/mtd/chips/cfi_cmdset_0002.c +4 -0

index a43ab45..e94f483 100644
@@ -440,6 +440,10 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 	mtd->flags   = MTD_CAP_NORFLASH;
 	mtd->name    = map->name;
 	mtd->writesize = 1;
+	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
+
+	DEBUG(MTD_DEBUG_LEVEL3, "MTD %s(): write buffer size %d\n",
+	      __func__, mtd->writebufsize);
 
 	mtd->reboot_notifier.notifier_call = cfi_amdstd_reboot;

drivers/mtd/chips/cfi_cmdset_0020.c +1 -0

index 314af1f..ed56ad3 100644
@@ -238,6 +238,7 @@ static struct mtd_info *cfi_staa_setup(struct map_info *map)
 	mtd->resume = cfi_staa_resume;
 	mtd->flags = MTD_CAP_NORFLASH & ~MTD_BIT_WRITEABLE;
 	mtd->writesize = 8; /* FIXME: Should be 0 for STMicro flashes w/out ECC */
+	mtd->writebufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	map->fldrv = &cfi_staa_chipdrv;
 	__module_get(THIS_MODULE);
 	mtd->name = map->name;

From: Guillaume LECERF <[email protected]>

---
 drivers/mtd/chips/cfi_cmdset_0001.c |    8 +++-----
 drivers/mtd/chips/cfi_cmdset_0002.c |    3 +--
 drivers/mtd/chips/cfi_cmdset_0020.c |   11 +++++------
 drivers/mtd/chips/cfi_probe.c       |    3 ++-
 4 files changed, 11 insertions(+), 14 deletions(-)

drivers/mtd/chips/cfi_cmdset_0001.c +3 -5

index 178f87b..6dbc44a 100644
@@ -1650,16 +1650,15 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 	struct cfi_private *cfi = map->fldrv_priv;
 	map_word status, write_cmd, datum;
 	unsigned long cmd_adr;
-	int ret, wbufsize, word_gap, words;
+	int ret, word_gap, words;
 	const struct kvec *vec;
 	unsigned long vec_seek;
 	unsigned long initial_adr;
 	int initial_len = len;
 
-	wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	adr += chip->start;
 	initial_adr = adr;
-	cmd_adr = adr & ~(wbufsize-1);
+	cmd_adr = adr & ~(mtd->writebufsize - 1);
 
 	/* Let's determine this according to the interleave only once */
 	write_cmd = (cfi->cfiq->P_ID != P_ID_INTEL_PERFORMANCE) ? CMD(0xe8) : CMD(0xe9);

@@ -1807,7 +1806,6 @@ static int cfi_intelext_writev (struct mtd_info *mtd, const struct kvec *vecs,
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
-	int wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	int ret = 0;
 	int chipnum;
 	unsigned long ofs, vec_seek, i;

@@ -1826,7 +1824,7 @@ static int cfi_intelext_writev (struct mtd_info *mtd, const struct kvec *vecs,
 
 	do {
 		/* We must not cross write block boundaries */
-		int size = wbufsize - (ofs & (wbufsize-1));
+		int size = mtd->writebufsize - (ofs & (mtd->writebufsize - 1));
 
 		if (size > len)
 			size = len;

drivers/mtd/chips/cfi_cmdset_0002.c +1 -2

index e94f483..c4f8675 100644
@@ -1492,7 +1492,6 @@ static int cfi_amdstd_write_buffers(struct mtd_info *mtd, loff_t to, size_t len,
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
-	int wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	int ret = 0;
 	int chipnum;
 	unsigned long ofs;

@@ -1528,7 +1527,7 @@ static int cfi_amdstd_write_buffers(struct mtd_info *mtd, loff_t to, size_t len,
 	/* Write buffer is worth it only if more than one word to write... */
 	while (len >= map_bankwidth(map) * 2) {
 		/* We must not cross write block boundaries */
-		int size = wbufsize - (ofs & (wbufsize-1));
+		int size = mtd->writebufsize - (ofs & (mtd->writebufsize - 1));
 
 		if (size > len)
 			size = len;

drivers/mtd/chips/cfi_cmdset_0020.c +5 -6

index ed56ad3..bd8a550 100644
@@ -426,15 +426,14 @@ static inline int do_write_buffer(struct map_info *map, struct flchip *chip,
 	map_word status, status_OK;
 	unsigned long cmd_adr, timeo;
 	DECLARE_WAITQUEUE(wait, current);
-	int wbufsize, z;
+	int z;
 
         /* M58LW064A requires bus alignment for buffer wriets -- saw */
         if (adr & (map_bankwidth(map)-1))
             return -EINVAL;
 
-        wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
         adr += chip->start;
-	cmd_adr = adr & ~(wbufsize-1);
+	cmd_adr = adr & ~(mtd->writebufsize - 1);
 
 	/* Let's determine this according to the interleave only once */
         status_OK = CMD(0x80);

@@ -610,7 +609,6 @@ static int cfi_staa_write_buffers (struct mtd_info *mtd, loff_t to,
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
-	int wbufsize = cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize;
 	int ret = 0;
 	int chipnum;
 	unsigned long ofs;

@@ -624,14 +622,15 @@ static int cfi_staa_write_buffers (struct mtd_info *mtd, loff_t to,
 
 #ifdef DEBUG_CFI_FEATURES
 	printk("%s: map_bankwidth(map)[%x]\n", __func__, map_bankwidth(map));
-	printk("%s: chipnum[%x] wbufsize[%x]\n", __func__, chipnum, wbufsize);
+	printk(KERN_DEBUG "%s: chipnum[%x] mtd->writebufsize[%x]\n", __func__,
+	       chipnum, mtd->writebufsize);
 	printk("%s: ofs[%x] len[%x]\n", __func__, ofs, len);
 #endif
 
         /* Write buffer is worth it only if more than one word to write... */
         while (len > 0) {
 		/* We must not cross write block boundaries */
-		int size = wbufsize - (ofs & (wbufsize-1));
+		int size = mtd->writebufsize - (ofs & (mtd->writebufsize - 1));
 
                 if (size > len)
                     size = len;

drivers/mtd/chips/cfi_probe.c +2 -1

index d255352..8f3135f 100644
@@ -377,7 +377,8 @@ static void print_cfi_ident(struct cfi_ident *cfip)
 		break;
 	}
 
-	printk("Max. bytes in buffer write: 0x%x\n", 1<< cfip->MaxBufWriteSize);
+	printk(KERN_DEBUG "Max. bytes in buffer write: 0x%x\n",
+	       cfi_interleave(cfi) << cfip->MaxBufWriteSize);
 	printk("Number of Erase Block Regions: %d\n", cfip->NumEraseRegions);
 
 }

Guillaume LECERF

2010-12-30 11:49 UTC #10 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

2010/12/30 Guillaume LECERF <[email protected]>:

Quoted 2 lines

Anatolij, what about such patches ?
Note : this is totally untested ;)

Please disregard 08-patch.patch, I think it does not even compile :(

Signature · 2 lines

Guillaume LECERF
GeeXboX developer - www.geexbox.org

Artem Bityutskiy

2011-01-05 08:27 UTC #11 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

Quoted 5 lines · Guillaume LECERF

2010/12/30 Guillaume LECERF <[email protected]>:
Quoted 2 lines
Anatolij, what about such patches ?
Note : this is totally untested ;)
Please disregard 08-patch.patch, I think it does not even compile :(

Anatolij is not answering, probably he is just very busy or having holidays or something. I think it is better to wait a little with these patches and let Anatolij look at this. So I'm removing them from my l2 tree so far and save them in the 'anatolij' branch of the l2 tree.

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Anatolij Gustschin

author

2011-01-06 11:13 UTC #12 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

Hi,

Quoted 11 lines · On Wed, 05 Jan 2011 10:27:21 +0200 Artem Bityutskiy <[email protected]> wrote:

Quoted 5 lines · Guillaume LECERF
2010/12/30 Guillaume LECERF <[email protected]>:
Quoted 2 lines
Anatolij, what about such patches ?
Note : this is totally untested ;)
Please disregard 08-patch.patch, I think it does not even compile :(
Anatolij is not answering, probably he is just very busy or having holidays or something. I think it is better to wait a little with these patches and let Anatolij look at this. So I'm removing them from my l2 tree so far and save them in the 'anatolij' branch of the l2 tree.

Sorry for not answering quickly, I was very busy and then was very ill. The two patches from Guillaume are wrong. Please apply my patches again.

Thanks,
Anatolij

Artem Bityutskiy

2011-01-06 11:14 UTC #13 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

Quoted 12 lines · Anatolij Gustschin

Hi,
Quoted 7 lines · On Wed, 05 Jan 2011 10:27:21 +0200 Artem Bityutskiy <[email protected]> wrote:
Quoted 5 lines · Guillaume LECERF
2010/12/30 Guillaume LECERF <[email protected]>:
Quoted 2 lines
Anatolij, what about such patches ?
Note : this is totally untested ;)
Please disregard 08-patch.patch, I think it does not even compile :(

Quoted 8 lines

Quoted 4 lines
Anatolij is not answering, probably he is just very busy or having holidays or something. I think it is better to wait a little with these patches and let Anatolij look at this. So I'm removing them from my l2 tree so far and save them in the 'anatolij' branch of the l2 tree.
Sorry for not answering quickly, I was very busy and then was very ill. The two patches from Guillaume are wrong. Please apply my patches again.

Yes, done, thanks.

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Anatolij Gustschin

author

2011-01-06 11:00 UTC #14 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

In reply to Guillaume LECERF

Hi,

Quoted 7 lines · On Thu, 30 Dec 2010 11:32:11 +0100 Guillaume LECERF <[email protected]> wrote:

2010/12/21 Artem Bityutskiy <[email protected]>:
Quoted 1 line
Yes, fair question, Anatolij?
Hi all.
Anatolij, what about such patches ?
Note : this is totally untested ;)

NACK. This is wrong. We need to export the real physical write buffer size of _one_ chip to UBI. Otherwise we will run into the same trouble and will end up with an unmountable UBIFS partition again. It doesn't even make sense to test your two patches, it won't work.

Thanks,
Anatolij

Anatolij Gustschin

author

2011-01-06 10:49 UTC #15 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

In reply to Artem Bityutskiy

Hi,

Quoted 21 lines · On Tue, 21 Dec 2010 16:45:24 +0200 Artem Bityutskiy <[email protected]> wrote:

Quoted 6 lines · ext Guillaume LECERF
Hello.
2010/12/16 Anatolij Gustschin <[email protected]>:
Quoted 3 lines
Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.
Quoted 13 lines
drivers/mtd/chips/cfi_cmdset_0002.c struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) · line 431
Quoted 9 lines
Signed-off-by Anatolij Gustschin <[email protected]>
---
drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
@@ -428,6 +428,10 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
       mtd->flags   = MTD_CAP_NORFLASH;
       mtd->name    = map->name;
       mtd->writesize = 1;
+       mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;
What about wbufsize initialized in cfi_amdstd_write_buffers() to "cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize" ?
I don't know this piece of code, but I just wanted to point it to you.

Quoted 1 line

Yes, fair question, Anatolij?

Just answered it.

Thanks,
Anatolij

Anatolij Gustschin

author

2011-01-06 10:49 UTC #16 lore raw

Re: [PATCH v2 2/6] mtd: cfi: add writebufsize initialization

In reply to Guillaume LECERF

Quoted 1 line · On Mon, 20 Dec 2010 17:36:57 +0100 Guillaume LECERF <[email protected]> wrote:

Hello.

Hello.

Quoted 4 lines

2010/12/16 Anatolij Gustschin <[email protected]>:
Quoted 3 lines
Initialize mtd->writebufsize to the value obtained by CFI query command at probe time.

Quoted 13 lines

drivers/mtd/chips/cfi_cmdset_0002.c struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary) · line 431
Quoted 9 lines
Signed-off-by Anatolij Gustschin <[email protected]>
---
drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
@@ -428,6 +428,10 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
        mtd->flags   = MTD_CAP_NORFLASH;
        mtd->name    = map->name;
        mtd->writesize = 1;
+       mtd->writebufsize = 1 << cfi->cfiq->MaxBufWriteSize;
What about wbufsize initialized in cfi_amdstd_write_buffers() to "cfi_interleave(cfi) << cfi->cfiq->MaxBufWriteSize" ?
I don't know this piece of code, but I just wanted to point it to you.

wbufsize is initialized there so that it takes into account the chip interleaving. If we e.g. have 64 byte write buffer in each chip and two 16-bit chips in 32-bit bank, the wbufsize will be initialized to 128 since the cfi_interleave(cfi) will return 2 in this case. It is really local to the cfi driver. For UBI to work correctly we need to export the the real physical write buffer size of one chip, nothing else. So my patch is correct.

Anatolij

Anatolij Gustschin

author

2010-12-16 22:42 UTC #17 lore raw

PATCH v2 3/6 mtd: nand: add mtd->writebufsize initialization

In reply to a message not in the archive

Initialize mtd->writebufsize to be equal to mtd->writesize.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 drivers/mtd/nand/nand_base.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/mtd/nand/nand_base.c +1 -0

index 5dd7ae4..4915067 100644
@@ -3439,6 +3439,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->resume = nand_resume;
 	mtd->block_isbad = nand_block_isbad;
 	mtd->block_markbad = nand_block_markbad;
+	mtd->writebufsize = mtd->writesize;
 
 	/* propagate ecc.layout to mtd_info */
 	mtd->ecclayout = chip->ecc.layout;

Signature · 1 line

1.7.1

Anatolij Gustschin

author

2010-12-16 22:42 UTC #18 lore raw

PATCH v2 4/6 mtd: onenand: add mtd->writebufsize initialization

In reply to a message not in the archive

Initialize mtd->writebufsize to be equal to mtd->writesize.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 drivers/mtd/onenand/onenand_base.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/mtd/onenand/onenand_base.c +1 -0

index 2d7c90d..d552821 100644
@@ -4076,6 +4076,7 @@ int onenand_scan(struct mtd_info *mtd, int maxchips)
 	mtd->block_isbad = onenand_block_isbad;
 	mtd->block_markbad = onenand_block_markbad;
 	mtd->owner = THIS_MODULE;
+	mtd->writebufsize = mtd->writesize;
 
 	/* Unlock whole block */
 	this->unlock_all(mtd);

Signature · 1 line

1.7.1

Anatolij Gustschin

author

2010-12-16 22:42 UTC #19 lore raw

PATCH v2 5/6 mtd: initialize writebufsize in the MTD object of a partition

In reply to a message not in the archive

Propagate the writebufsize to the partition's MTD object so that UBI can set correct value for it's minimal I/O size using the writebufsize field of MTD object of the partition.

By previous patches we added proper writebufsize field initialization. Next patch can now change UBI to use this field for setting the minimal I/O size.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 drivers/mtd/mtdconcat.c |    1 +
 drivers/mtd/mtdpart.c   |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

drivers/mtd/mtdconcat.c +1 -0

index bf8de09..5f5777b 100644
@@ -776,6 +776,7 @@ struct mtd_info *mtd_concat_create(struct mtd_info *subdev[],	/* subdevices to c
 	concat->mtd.size = subdev[0]->size;
 	concat->mtd.erasesize = subdev[0]->erasesize;
 	concat->mtd.writesize = subdev[0]->writesize;
+	concat->mtd.writebufsize = subdev[0]->writebufsize;
 	concat->mtd.subpage_sft = subdev[0]->subpage_sft;
 	concat->mtd.oobsize = subdev[0]->oobsize;
 	concat->mtd.oobavail = subdev[0]->oobavail;

drivers/mtd/mtdpart.c +1 -0

index 1047ff0..b910a37 100644
@@ -384,6 +384,7 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	slave->mtd.flags = master->flags & ~part->mask_flags;
 	slave->mtd.size = part->size;
 	slave->mtd.writesize = master->writesize;
+	slave->mtd.writebufsize = master->writebufsize;
 	slave->mtd.oobsize = master->oobsize;
 	slave->mtd.oobavail = master->oobavail;
 	slave->mtd.subpage_sft = master->subpage_sft;

Signature · 1 line

1.7.1

Anatolij Gustschin

author

2010-12-16 22:42 UTC #20 lore raw

PATCH v2 6/6 UBI: use mtd->writebufsize to set minimal I/O unit size

In reply to a message not in the archive

Previously we used mtd->writesize field to set UBI's minimal I/O unit size. This sometimes caused UBIFS recovery issues when mounting an uncleanly unmounted UBIFS partition on NOR flash since mtd->writesize is 1 byte for NOR flash. The MTD CFI driver however often performs writing multiple bytes in one programming operation using the chip's write buffer. We have to use the size of this write buffer as a minimal I/O unit size for UBI on NOR flash to fix the observed UBIFS recovery issues.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 drivers/mtd/ubi/build.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

drivers/mtd/ubi/build.c +18 -1

index 5ebe280..c2d5310 100644
@@ -672,7 +672,24 @@ static int io_init(struct ubi_device *ubi)
 		ubi->nor_flash = 1;
 	}
 
-	ubi->min_io_size = ubi->mtd->writesize;
+	/*
+	 * Sets minimal I/O unit size (min_io_size) for UBI. On NAND flash
+	 * min_io_size should be equivalent to ubi->mtd->writesize.
+	 * In case of NOR flash minimal I/O size must be equal to the size
+	 * of the write buffer used by internal flash programming algorithm.
+	 * This requirement results from the fact that the flash programming
+	 * operation could be interrupted by a power cut or a system reset
+	 * causing corrupted (partially written) areas in a NOR flash sector.
+	 * Knowing the size of potentially corrupted areas UBIFS scanning
+	 * and recovery algorithms are able to perform successful recovery.
+	 */
+	if (ubi->mtd->type == MTD_NANDFLASH)
+		ubi_assert(ubi->mtd->writebufsize == ubi->mtd->writesize);
+	else if (ubi->mtd->type == MTD_NORFLASH)
+		ubi_assert(ubi->mtd->writebufsize > 0);
+
+	ubi->min_io_size = ubi->mtd->writebufsize;
+
 	ubi->hdrs_min_io_size = ubi->mtd->writesize >> ubi->mtd->subpage_sft;
 
 	/*

Signature · 1 line

1.7.1

Artem Bityutskiy

2010-12-19 17:46 UTC #21 lore raw

Re: [PATCH v2 6/6] UBI: use mtd->writebufsize to set minimal I/O unit size

Quoted 11 lines · Anatolij Gustschin

Previously we used mtd->writesize field to set UBI's minimal I/O unit size. This sometimes caused UBIFS recovery issues when mounting an uncleanly unmounted UBIFS partition on NOR flash since mtd->writesize is 1 byte for NOR flash. The MTD CFI driver however often performs writing multiple bytes in one programming operation using the chip's write buffer. We have to use the size of this write buffer as a minimal I/O unit size for UBI on NOR flash to fix the observed UBIFS recovery issues.
Signed-off-by Anatolij Gustschin <[email protected]>

I've re-wrote the commentary and pushed, please, check.

Signature · 2 lines

Best Regards,
Artem Bityutskiy (Битюцкий Артём)

Anatolij Gustschin

author

2010-12-20 09:19 UTC #22 lore raw

Re: [PATCH v2 6/6] UBI: use mtd->writebufsize to set minimal I/O unit size

Quoted 11 lines · On Sun, 19 Dec 2010 19:46:19 +0200 Artem Bityutskiy <[email protected]> wrote:

Quoted 10 lines · Anatolij Gustschin
Previously we used mtd->writesize field to set UBI's minimal I/O unit size. This sometimes caused UBIFS recovery issues when mounting an uncleanly unmounted UBIFS partition on NOR flash since mtd->writesize is 1 byte for NOR flash. The MTD CFI driver however often performs writing multiple bytes in one programming operation using the chip's write buffer. We have to use the size of this write buffer as a minimal I/O unit size for UBI on NOR flash to fix the observed UBIFS recovery issues.

Quoted 3 lines

Quoted 1 line
Signed-off-by Anatolij Gustschin <[email protected]>
I've re-wrote the commentary and pushed, please, check.

Checked, it is ok. Thanks!

Anatolij