RE: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flashafter erasure interrupted

[Qi Wang çè (qiwang)]

Hi Artem:
Sorry to interrupt your busy life. 
As you said in previous mail, I send my patch separately without quoting this e-mail. And I have send to you, but I never get  your reply. I am very confuse, no sure if is there anything wrong at the patch I send to you.
Can you help explain to me?
Thanks

-----Original Message-----
From: Artem Bityutskiy [mailto:dedekind1@xxxxxxxxx] 
Sent: Friday, November 01, 2013 4:58 PM
To: Qi Wang çè (qiwang)
Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; Adrian Hunter; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

Hi,

could you please re-send your patch separately, without quoting any
parts of this conversation, so that I could use 'git am'.

Your patch also contains trailing white-spaces, please, get rid of them
in the next submission.

Also, could you please clearly state whether you have tested this patch
on a real NOR flash or not. If yes, then could you share the chip
vendor/type information?

On Thu, 2013-10-31 at 04:07 +0000, Qi Wang çè (qiwang) wrote:

> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -499,59 +499,44 @@ 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 header
> -	 * comment in this file). But we know this is a 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.
> -	 */

Please, do not remove this comment.

>  	struct ubi_vid_hdr vid_hdr;
> +	struct ubi_ec_hdr ec_hdr;

To make it obvious what the above big comment talks about, could you
please define 'struct ubi_ec_hdr ec_hdr' above that big comment.

Otherwise looks good to me, thank you!


> My Comments for above changing:
> 1. 
> 	-	/*
> 	-	 * Note, we cannot generally define VID header buffers on stack,
> 	-	 * because of the way we deal with these buffers (see the header
> 	-	 * comment in this file). But we know this is a 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.
> 	-	 */
> 	I remove above comment, because I pre-allocate VID header and EC header together. 
> 	So I think no need to emphasize VID header buffers cannot be on stack.
> 	(Maybe my understanding about this comment is error, if so, please correct me)

The problem is that some functions in io.c can read or write _beyond_
sizeof(struct ubi_vid_hdr), but this is only relevant to NAND, not for
NOR, and the code you change is NOR-only. This is why that comment is
there, and I'd like to keep it.

> 2.
> 	why use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	but not 
> 	"if (!err)" 
> 	to judge if need to program '0' to invalid this block.
> 
> 	In case err == UBI_IO_FF_BITFLIPS, err == UBI_IO_BITFLIPS or unexpected value return
> 	from read function, I think UBI still need to invalid this block for above mentioned 
> 	condition. So I use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	to judge. 

In case of UBI_IO_FF (all FFs) UBI will erase the eraseblock before
using it anyway, so invalidation is not necessary.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—