UBI: optimize vid-header IO read checks

From: "Gupta, Pekon" <pekon@ti.com>

This patch is extension of earlier patch
	'UBI: optimize erase-header IO read checksUBI'

During mounting of UBI device, ec_hdr and vid_hdr of all PEB are scanned.
This patch tries to reduce the PEB scan time, by optimizing ubi_io_read_vid_hdr
based on following analysis.
--------------------------------------------------------------------------
EC_HDR	VID_HDR	CONCLUSION		NEXT STEP
--------------------------------------------------------------------------
valid	valid	PEB contains data	map PEB to corresponding LEB
valid	blank	PEB is free		attach to free list
valid	invalid	PEB may contain data	try recovering data

invalid valid	PEB may contain data	try recovering data
invalid blank	PEB is free[1]		schedule for erasure
invalid invalid	underministic		schedule for erasure
--------------------------------------------------------------------------

EC_HDR or VID_HDR may read 'invalid' due to following reasons:
(a) contains non-UBI data.
(b) block is completely erased.
(b) header corrupted due to power-failure during program or erase operation.
(c) header corrupted due to bitflips after program or erase operation.

Signed-off-by: Gupta, Pekon <pekon@ti.com>
---
 drivers/mtd/ubi/io.c | 110 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 42 deletions(-)

Hi, 

Requesting feedbacks/comments on this one too ?
This should be independent of previous one..

with regards, pekon

> -----Original Message-----
> From: Gupta, Pekon
> Sent: Sunday, April 28, 2013 7:21 PM
> To: linux-mtd@lists.infradead.org
> Cc: Gupta, Pekon
> Subject: [PATCH] UBI: optimize vid-header IO read checks
> 
> From: "Gupta, Pekon" <pekon@ti.com>
> 
> This patch is extension of earlier patch
> 	'UBI: optimize erase-header IO read checksUBI'
> 
> During mounting of UBI device, ec_hdr and vid_hdr of all PEB are
> scanned.
> This patch tries to reduce the PEB scan time, by optimizing
> ubi_io_read_vid_hdr
> based on following analysis.
> --------------------------------------------------------------------------
> EC_HDR	VID_HDR	CONCLUSION		NEXT STEP
> --------------------------------------------------------------------------
> valid	valid	PEB contains data	map PEB to corresponding LEB
> valid	blank	PEB is free		attach to free list
> valid	invalid	PEB may contain data	try recovering data
> 
> invalid valid	PEB may contain data	try recovering data
> invalid blank	PEB is free[1]		schedule for erasure
> invalid invalid	underministic		schedule for erasure
> --------------------------------------------------------------------------
> 
> EC_HDR or VID_HDR may read 'invalid' due to following reasons:
> (a) contains non-UBI data.
> (b) block is completely erased.
> (b) header corrupted due to power-failure during program or erase
> operation.
> (c) header corrupted due to bitflips after program or erase operation.
> 
> Signed-off-by: Gupta, Pekon <pekon@ti.com>
> ---
>  drivers/mtd/ubi/io.c | 110 +++++++++++++++++++++++++++++++------
> --------------
>  1 file changed, 68 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 0b9b0af..1446137 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -532,7 +532,7 @@ static int nor_erase_prepare(struct ubi_device
> *ubi, int pnum)
>  	 */
>  	err1 = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0);
>  	if (err1 == UBI_IO_BAD_HDR_EBADMSG || err1 ==
> UBI_IO_BAD_HDR ||
> -	    err1 == UBI_IO_FF) {
> +	    err1 == UBI_IO_FF || err1 == UBI_IO_FF_BITFLIPS) {
>  		struct ubi_ec_hdr ec_hdr;
> 
>  		err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0);
> @@ -829,7 +829,6 @@ int ubi_io_write_ec_hdr(struct ubi_device *ubi,
> int pnum,
>  {
>  	int err;
>  	uint32_t crc;
> -
>  	dbg_io("write EC header to PEB %d", pnum);
>  	ubi_assert(pnum >= 0 &&  pnum < ubi->peb_count);
> 
> @@ -998,60 +997,87 @@ int ubi_io_read_vid_hdr(struct ubi_device
> *ubi, int pnum,
>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
>  			  ubi->vid_hdr_alsize);
> -	if (read_err && read_err != UBI_IO_BITFLIPS &&
> !mtd_is_eccerr(read_err))
> -		return read_err;
> -
> -	magic = be32_to_cpu(vid_hdr->magic);
> -	if (magic != UBI_VID_HDR_MAGIC) {
> -		if (mtd_is_eccerr(read_err))
> +	switch (read_err) {
> +	case -EBADMSG:
> +		/* un-correctable bit-flips detected
> +		 * Case-1: hdr_crc != crc(vid_hdr)
> +		 *	uncorrectable bit-flips detected within header
> region.
> +		 *	header data cannot be trusted.
> +		 * Case-2: hdr_crc == crc(vid_hdr)
> +		 *	uncorrectable bit-flip do not effect header
> region.
> +		 *	header data can be parsed.
> +		 */
> +		crc = crc32(UBI_CRC32_INIT, vid_hdr,
> UBI_VID_HDR_SIZE_CRC);
> +		hdr_crc = be32_to_cpu(vid_hdr->hdr_crc);
> +		if (hdr_crc == crc)
> +			goto good_vid_hdr;
> +		else
>  			return UBI_IO_BAD_HDR_EBADMSG;
> +		break;
> 
> -		if (ubi_check_pattern(vid_hdr, 0xFF,
> UBI_VID_HDR_SIZE)) {
> +	case 0:
> +	case UBI_IO_BITFLIPS:
> +		/* check whether this is UBI vid-hdr */
> +		magic = be32_to_cpu(vid_hdr->magic);
> +		if (magic == UBI_VID_HDR_MAGIC) {
> +			/* check hdr-crc (data integrity check) */
> +			crc = crc32(UBI_CRC32_INIT, vid_hdr,
> +
> 	UBI_VID_HDR_SIZE_CRC);
> +			hdr_crc = be32_to_cpu(vid_hdr->hdr_crc);
> +			if (hdr_crc == crc) {
> +				goto good_vid_hdr;
> +			} else {
> +				if (verbose) {
> +					ubi_warn("bad vid_hdr CRC at
> PEB %d",
> +
> 	pnum);
> +					ubi_dump_vid_hdr(vid_hdr);
> +				}
> +				ubi_warn("bad vid_hdr CRC at PEB %d",
> pnum);
> +				if (read_err)
> +					return
> UBI_IO_BAD_HDR_EBADMSG;
> +				else
> +					return UBI_IO_BAD_HDR;
> +			}
> +
> +		/* BUT UNLIKE EC_HDR, we need to explicit check for
> +		* vid-hdr(data) == all(0xFF) because vid_hdr is written
> only
> +		* when PEB is mapped to LEB, Thus valid free PEB will still
> +		* have blank vid_hdr.
> +		*/
> +		} else if (ubi_check_pattern(vid_hdr, 0xFF,
> +
> 	UBI_VID_HDR_SIZE)) {
>  			if (verbose)
> -				ubi_warn("no VID header found at PEB
> %d, only 0xFF bytes",
> -					 pnum);
> -			dbg_bld("no VID header found at PEB %d, only
> 0xFF bytes",
> -				pnum);
> -			if (!read_err)
> -				return UBI_IO_FF;
> -			else
> -				return UBI_IO_FF_BITFLIPS;
> -		}
> +				ubi_warn("No vid_hdr found at PEB %d",
> pnum);
> +			dbg_bld("No vid_hdr found at PEB %d", pnum);
> +			return (read_err) ? UBI_IO_FF_BITFLIPS :
> UBI_IO_FF;
> 
> -		if (verbose) {
> -			ubi_warn("bad magic number at PEB %d: %08x
> instead of %08x",
> -				 pnum, magic, UBI_VID_HDR_MAGIC);
> -			ubi_dump_vid_hdr(vid_hdr);
> +		/* its a programmed page, but does not contain UBI
> vid_hdr */
> +		} else {
> +			if (verbose) {
> +				ubi_warn("invalid vid_hdr at PEB %d",
> pnum);
> +				ubi_dump_vid_hdr(vid_hdr);
> +			}
> +			dbg_bld("invalid vid_hdr at PEB %d", pnum);
> +			return UBI_IO_BAD_HDR_EBADMSG;
>  		}
> -		dbg_bld("bad magic number at PEB %d: %08x instead of
> %08x",
> -			pnum, magic, UBI_VID_HDR_MAGIC);
> -		return UBI_IO_BAD_HDR;
> -	}
> -
> -	crc = crc32(UBI_CRC32_INIT, vid_hdr, UBI_VID_HDR_SIZE_CRC);
> -	hdr_crc = be32_to_cpu(vid_hdr->hdr_crc);
> +		break;
> 
> -	if (hdr_crc != crc) {
> -		if (verbose) {
> -			ubi_warn("bad CRC at PEB %d, calculated %#08x,
> read %#08x",
> -				 pnum, crc, hdr_crc);
> -			ubi_dump_vid_hdr(vid_hdr);
> -		}
> -		dbg_bld("bad CRC at PEB %d, calculated %#08x, read
> %#08x",
> -			pnum, crc, hdr_crc);
> -		if (!read_err)
> -			return UBI_IO_BAD_HDR;
> -		else
> -			return UBI_IO_BAD_HDR_EBADMSG;
> +	default:
> +		/* -EIO (may be incomplete page reads) */
> +		return read_err;
>  	}
> 
> +good_vid_hdr:
>  	err = validate_vid_hdr(ubi, vid_hdr);
>  	if (err) {
>  		ubi_err("validation failed for PEB %d", pnum);
>  		return -EINVAL;
>  	}
> 
> -	return read_err ? UBI_IO_BITFLIPS : 0;
> +	if (read_err)
> +		return UBI_IO_BITFLIPS;
> +	else
> +		return 0;
>  }
> 
>  /**
> --
> 1.8.1

On Thu, 2013-05-16 at 06:38 +0000, Gupta, Pekon wrote:
> Hi, 
> 
> Requesting feedbacks/comments on this one too ?
> This should be independent of previous one..

How did you test this change? Did you test that we do not die in
scanning in case of bit-flips? How about ECC errors? Do we scrub
eraseblocks then?

> On Thu, 2013-05-16 at 06:38 +0000, Gupta, Pekon wrote:

> > Hi,

> >

> > Requesting feedbacks/comments on this one too ?

> > This should be independent of previous one..

> 

> How did you test this change? Did you test that we do not die in

> scanning in case of bit-flips? How about ECC errors? Do we scrub

> eraseblocks then?

> 

Hi,

Attaching the logs for tests I ran to for ec_hdr patch. Added 
following printk to cross-check the result.
- printk in scan_peb() to check return values of ubi_io_read_ec_hdr()  
- printk in scan_peb() to check return values of ubi_io_read_vid_hdr()
- printk in do_sync_erase() to check if bit-flipped PEB is scrubbed

s/*UBI in attached logs..


Test 1: introduced correctable bit-flips in ec_header of PEB=1
- Observations:
- UBI: fixable bit-flip detected at PEB 1
	*UBI:ubi_io_read_ec_hdr: PEB=1, OK
	*UBI:scan_peb: PEB=1: ec_hdr=UBI_IO_BITFLIPS
	*UBI:scan_peb: PEB=1: ec_hdr acceptable
	*UBI:scan_peb: PEB=1: vid_hdr=UBI_IO_FF
- PEB 1 scubbed
	*UBI:do_sync_erase: PEB=1 erasing ..
	*UBI:ubi_io_sync_erase: PEB=1: erase successful> --
- UBIFS: mounted UBI device 0, volume 0, name "test"



Test 2: introduced un-correctable bit-flips in ec_header of PEB=1
-Observations:
- warning: ubi_io_read: error -74 (ECC error) while reading 64 bytes from PEB 1:0
	<stack_dump>
	*UBI:ubi_io_read_ec_hdr: PEB=1, EBADMSG
	*UBI:scan_peb: PEB=1: ec_hdr=UBI_IO_BAD_HDR_EBADMSG
- vid_hdr not parsed due to reasons below (un-correctable bit-flips)
	+	/* un=recoverable erase-header
	+	* unknown erase-count: can be set to mean erase-count.
	+	* unknown image_seq: cannot determine if PEB was programmed
	+	* unknown version: cannot determine UBI protocol to use
	+	* So no point in checking vid_hdr, schedule this PEB for erase. */
- PEB 1 scrubbed
	*UBI:do_sync_erase: PEB=1 erasing ..
	*UBI:ubi_io_sync_erase: PEB=1: erase successful
- UBIFS: mounted UBI device 0, volume 0, name "test"



Test 3: Normal case without any bit-flip
- Observations
- Both ec_hdr and vid_hdr scanned ok
	*UBI:scan_peb: PEB=1: ec_hdr=OK
	*UBI:scan_peb: PEB=1: ec_hdr acceptable
	*UBI:scan_peb: PEB=1: vid_hdr=UBI_IO_FF
- no scrubbing required, volume mounted.
- UBIFS: mounted UBI device 0, volume 0, name "test"



Hope that helps in understanding the attached logs..
(1) Request you to review the tables in patch description, in case I missed 
any corner case. 
(2) Also if you would like me to check any scenario explicitly.
(3) I have similar results for vid_hdr patch, I'll share it separately.


with regards, pekon