[PATCH] UBI: optimize erase-header IO read checks

[Artem Bityutskiy]

On Mon, 2013-04-29 at 10:37 +0530, Gupta, Pekon wrote:
...
> Signed-off-by: Gupta, Pekon <pekon at ti.com>

I do generally understand what you are trying to do in this patch, but I
cannot carefully review it, because it is too big, just like the second
patch.

It really should be split. I think you should start with a preparation
patch which does not change the current logic, but just transforms the
code to using a switch statement.

Then you should start modifying the big switch statement, preferably one
step per patch, explaining each step. I guess the first step would be
removing the magic numbers check. Another step would be removing the
0xFF comparison. Etc.

Then your work could be reviewed, and we could be somewhat confident
that it does not break anything.

> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index bf79def..624365b 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -537,7 +537,7 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>  
>  		err1 = ubi_io_read_ec_hdr(ubi, pnum, &ec_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)

Let's check this change, for example.

The comments in this function say that it is important to have both EC
and VID headers invalid. But here it looks like you allow the EC header
to be valid, but just have a bitflip. But in this function we do want to
make sure both headers are invalid...

This looks incorrect. But may be I just did not understand you changes
in the 'ubi_io_read_ec_hdr()' function.

Anyway, we really need to do everything in a more controllable and
reviewable manner.

-- 
Best Regards,
Artem Bityutskiy

-- 
Best Regards,
Artem Bityutskiy