From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-out.m-online.net ([212.18.0.10]) by canuck.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1POTc4-0000an-Vj for linux-mtd@lists.infradead.org; Fri, 03 Dec 2010 11:15:42 +0000 Date: Fri, 3 Dec 2010 12:15:37 +0100 From: Anatolij Gustschin To: dedekind1@gmail.com Subject: Re: UBIFS partition on NOR flash not mountable after power cut test Message-ID: <20101203121537.50b5ff63@mw.dip.t-dialin.net> In-Reply-To: <1291264926.14534.32.camel@koala> References: <20101129195014.19224240@wker> <20101201130534.5b95ce83@wker> <20101201164447.2215bc58@wker> <1291264926.14534.32.camel@koala> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: linux-mtd@lists.infradead.org, Detlev Zundel List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Thu, 02 Dec 2010 06:42:06 +0200 Artem Bityutskiy wrote: ... > From 7ddb9cb80ab54d118e2a055ce7a35649070578b1 Mon Sep 17 00:00:00 2001 > From: Artem Bityutskiy > Date: Thu, 2 Dec 2010 06:34:01 +0200 > Subject: [PATCH] UBI: fix corrupted PEB detection for NOR flash > > My new shiny code for corrupted PEB detection has NOR specific bug. > We tread PEB as corrupted and preserve it, if > > 1. EC header is OK. > 2. VID header is corrupted. > 3. data area is not "all 0xFFs" > > In case of NOR we have 'nor_erase_prepare()' quirk, which invalidates > the headers before erasing the PEB. And we invalidate first the VID > header, and then the EC header. So if a power cut happens after we have > invalidated the VID header, but before we have invalidated the EC > header, we end up with a PEB which satisfies the above 3 conditions, > and the scanning code will treat it as corrupted, and will print > scary warnings, wrongly. > > This patch fixes the issue by firt invalidating the EC header, then > invalidating the VID header. In case of power cut inbetween, we still > just lose the EC header, and UBI can deal with this situation gracefully. > > This patch also adds one irrelevant comment about defining VID header > buffer on stack. > > Thanks to Anatolij Gustschin for tracking this down. > > Signed-off-by: Artem Bityutskiy > Reported-by: Anatolij Gustschin Tested-by: Anatolij Gustschin > --- > drivers/mtd/ubi/io.c | 37 ++++++++++++++++++++++++++++--------- > drivers/mtd/ubi/scan.c | 4 ++++ > 2 files changed, 32 insertions(+), 9 deletions(-) > > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c > index c2960ac..3839253 100644 > --- a/drivers/mtd/ubi/io.c > +++ b/drivers/mtd/ubi/io.c > @@ -480,12 +480,26 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum) > size_t written; > loff_t addr; > uint32_t data = 0; > + /* > + * Note, we cannot generally define VID header buffers on stack, > + * because of the way we deal with these buffers (see the heder > + * comment). But we know this is NOR-specific piece of code, so we can > + * do this. But yes, this is error-prone and we should (pre-)allocate > + * VID header buffer instead. > + */ > struct ubi_vid_hdr vid_hdr; > > - addr = (loff_t)pnum * ubi->peb_size + ubi->vid_hdr_aloffset; > + /* > + * Note, it is important to first invalidate the EC header, and then > + * VID header. Otherwise a power cut may result in valid EC header and > + * invalid VID header, then UBI will treat this PEB as corrupted and > + * will try to preserve it, print scary warnings. See scan.c header > + * comment for more information. > + */ > + addr = (loff_t)pnum * ubi->peb_size; > err = ubi->mtd->write(ubi->mtd, addr, 4, &written, (void *)&data); > if (!err) { > - addr -= ubi->vid_hdr_aloffset; > + addr += ubi->vid_hdr_aloffset; > err = ubi->mtd->write(ubi->mtd, addr, 4, &written, > (void *)&data); > if (!err) > @@ -499,13 +513,18 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum) > * In this case we probably anyway have garbage in this PEB. > */ > err1 = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0); > - if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR) > - /* > - * The VID header is corrupted, so we can safely erase this > - * PEB and not afraid that it will be treated as a valid PEB in > - * case of an unclean reboot. > - */ > - return 0; > + if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR) { > + struct ubi_ec_hdr ec_hdr; > + > + err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0); > + if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 == UBI_IO_BAD_HDR) > + /* > + * The VID and EC headers are corrupted, so we can > + * safely erase this PEB and not afraid that it will be > + * treated as a valid PEB in case of an unclean reboot. > + */ > + return 0; > + } > > /* > * The PEB contains a valid VID header, but we cannot invalidate it. > diff --git a/drivers/mtd/ubi/scan.c b/drivers/mtd/ubi/scan.c > index 3c63186..d4b1115 100644 > --- a/drivers/mtd/ubi/scan.c > +++ b/drivers/mtd/ubi/scan.c > @@ -951,6 +951,10 @@ static int process_eb(struct ubi_device *ubi, struct ubi_scan_info *si, > * impossible to distinguish it from a PEB which just > * contains garbage because of a power cut during erase > * operation. So we just schedule this PEB for erasure. > + * > + * Besides, in case of NOR flash, we deliberatly > + * corrupt both headers because NOR flash erasure is > + * slow and can start from the end. > */ > err = 0; > else