| Message ID | 71CF8D7F32C5C24C9CD1D0E02D52498A770B220C@NTXXIAMBX02.xacn.micron.com |
|---|---|
| State | New, archived |
| Headers | show |
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!
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
On Mon, Dec 23, 2013 at 02:03:16AM +0000, Qi Wang 王起 (qiwang) wrote: > Sorry to interrupt your busy life. Yes, unfortunately Artem hasn't been too responsive lately. > 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? I'm not really a UBI expert, but I doubt that there is a real problem with your patch. Artem just hasn't had time to look at it again. Although you have been waiting a while, I suppose the best option is simply to wait some more. Artem tends to revisit patches eventually, sometimes after several weeks/months. Brian
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c index bf79def..0fdaca9 100644 --- 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. - */ struct ubi_vid_hdr vid_hdr; + struct ubi_ec_hdr ec_hdr; + + addr = (loff_t)pnum * ubi->peb_size; /* + * If VID or EC is valid, need to corrupt it before erase operation. * It is important to first invalidate the EC header, and then the VID * header. Otherwise a power cut may lead to valid EC header and * invalid VID header, in which case UBI will treat this PEB as * corrupted and will try to preserve it, and print scary warnings. */ - addr = (loff_t)pnum * ubi->peb_size; - err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data); - if (!err) { - addr += ubi->vid_hdr_aloffset; - err = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data); - if (!err) - return 0; + err = ubi_io_read_ec_hdr(ubi, pnum, &ec_hdr, 0); + if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && + err != UBI_IO_FF){ + err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data); + if(err1) + goto error; } - /* - * We failed to write to the media. This was observed with Spansion - * S29GL512N NOR flash. Most probably the previously eraseblock erasure - * was interrupted at a very inappropriate moment, so it became - * unwritable. 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 || - err1 == UBI_IO_FF) { - 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 || - err1 == UBI_IO_FF) - /* - * Both 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; + err = ubi_io_read_vid_hdr(ubi, pnum, &vid_hdr, 0); + if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && + err != UBI_IO_FF){ + addr += ubi->vid_hdr_aloffset; + err1 = mtd_write(ubi->mtd, addr, 4, &written, (void *)&data); + if (err1) + goto error; } + return 0; + +error: /* - * The PEB contains a valid VID header, but we cannot invalidate it. + * The PEB contains a valid VID or EC header, but we cannot invalidate it. * Supposedly the flash media or the driver is screwed up, so return an * error. */ - ubi_err("cannot invalidate PEB %d, write returned %d read returned %d", + ubi_err("cannot invalidate PEB %d, read returned %d write returned %d ", pnum, err, err1); ubi_dump_flash(ubi, pnum, 0, ubi->peb_size); return -EIO;