lkml.org 
[lkml]   [2013]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRE: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted
Date
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@gmail.com]
Sent: Friday, November 01, 2013 4:58 PM
To: Qi Wang 王起 (qiwang)
Cc: linux-mtd@lists.infradead.org; Adrian Hunter; linux-kernel@vger.kernel.org
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

\
 
 \ /
  Last update: 2013-12-23 03:41    [W:0.086 / U:0.400 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site