| Message ID | 1313625029-19546-2-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
I forgot to CC a contributor on this (and the complementary patch for "read OOB") Cc: Jim Quinlan <jim2101024@gmail.com> On Mon, Aug 22, 2011 at 1:35 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote: >> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls. > > I guess this patch deserves to be non-RFC? Should it be pushed to > l2-mtd-2.6.git? Should it even have "Cc: stable@kernel.org [kernel > version +] ? I'm not actually sure where this stands (RFC vs. patch) since I really wanted some outside opinion on the methods used here. I have a feeling that some of this is only working on my hardware. For instance, somehow (I'm really not sure how!) `nandwrite -n -o' is working in nandsim without my fix. Perhaps this is due to a different set of nand_ecc_ctrl functions (soft ECC vs. HW ECC). With this patch applied, however, I get some strange kernel oopses with nandsim. I've identified at least one issue, I think, but I haven't completely resolved this discrepancy between nandsim and my driver. See sample commands: # insmod nandsim.ko # nandwrite /dev/mtdX <data.bin> -n -o So for this "fix" (and its coming updates), I would appreciate some outside testing on other systems, especially before we send this to stable or even before including it upstream at all. And any comments on the current status of noecc and MTD_OOB_RAW from others would be highly valuable to me; a bit of system information and a "working" or "not working since commit [XXX]" would be a start. In the meantime, I'm trying to get a hold of a wider variety of test systems for myself for these kind of issues... >> - ops.mode = MTD_OOB_PLACE; >> + ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE; > > Would be really great to document things while you are in it - e.g., > putting a little note in include/linux/mtd/mtd.h that MTD_OOB_PLACE is > the default mode. And of course documenting the modes in this file a bit > more would be also great. I encourage you to store the wisdom you gain > in the comments :-) I'll see what I can do. Probably a task for after the dust settles on some of the changes I'm working on. Brian
On Mon, 2011-08-22 at 13:08 -0700, Brian Norris wrote: > I forgot to CC a contributor on this (and the complementary patch for > "read OOB") > > Cc: Jim Quinlan <jim2101024@gmail.com> > > On Mon, Aug 22, 2011 at 1:35 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote: > >> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls. > > > > I guess this patch deserves to be non-RFC? Should it be pushed to > > l2-mtd-2.6.git? Should it even have "Cc: stable@kernel.org [kernel > > version +] ? > > I'm not actually sure where this stands (RFC vs. patch) since I really > wanted some outside opinion on the methods used here. I have a feeling > that some of this is only working on my hardware. For instance, > somehow (I'm really not sure how!) `nandwrite -n -o' is working in > nandsim without my fix. Perhaps this is due to a different set of > nand_ecc_ctrl functions (soft ECC vs. HW ECC). May be one of the reasons is that nandsim just _copies_ data to the internal RAM buffer, instead of doing binary "&", so you can re-write in some cases, but not sure. > With this patch applied, however, I get some strange kernel oopses > with nandsim. I've identified at least one issue, I think, but I > haven't completely resolved this discrepancy between nandsim and my > driver. See sample commands: > > # insmod nandsim.ko > # nandwrite /dev/mtdX <data.bin> -n -o > > So for this "fix" (and its coming updates), I would appreciate some > outside testing on other systems, especially before we send this to > stable or even before including it upstream at all. > > And any comments on the current status of noecc and MTD_OOB_RAW from > others would be highly valuable to me; a bit of system information and > a "working" or "not working since commit [XXX]" would be a start. In > the meantime, I'm trying to get a hold of a wider variety of test > systems for myself for these kind of issues... Yes, would be great to have more people to test. I always encourage people to look at patches _before_ they get in and break their systems, not afterwards. But even if nobody cares, we can merge your stuff after you gave it some more testing - absence of caring people should not stop the progress in MTD :-)
Hi, Brian, 2011/8/18 Brian Norris <computersforpeace@gmail.com>: > mtd_do_write_oob does not pass information about whether to write in RAW > mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or > MTD_OOB_PLACE based on the MTD file mode. > > This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls. It's worthy here stating clear what the issue you meet and how it fix. Jason
On Mon, Aug 22, 2011 at 10:25 PM, Jason Liu <liu.h.jason@gmail.com> wrote: > 2011/8/18 Brian Norris <computersforpeace@gmail.com>: >> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls. > > It's worthy here stating clear what the issue you meet and how it fix. The issue is simply that `nandwrite -n -o' does not work on my system; `nandwrite -n' can write page data to my flash without applying ECC, but when I use the `-o' option, ECC is applied (incorrectly), contrary to the `--noecc' option. I found that this is the case because my hardware computes and writes ECC data to flash upon either OOB write or page write. Thus, to support a proper "no ECC" write, my driver must know when we're performing a raw OOB write vs. a normal ECC OOB write. However, MTD does not pass any raw mode information to the write_oob functions. This patch addresses the problems by: 1) Passing MTD_OOB_RAW down to lower layers, instead of just defaulting to MTD_OOB_PLACE 2) Handling MTD_OOB_RAW within the NAND layer's `nand_do_write_oob' Is this a better description? I'll include this description for v2. Note that I have yet to update the appropriate `nand_write_page_raw' function to properly handle an OOB-only write. I've fixed my own driver, but there's an issue in the nand_base version. Will include in v2. Thanks, Brian
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index a75d555..7ca4361 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -391,6 +391,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd, uint64_t start, uint32_t length, void __user *ptr, uint32_t __user *retp) { + struct mtd_file_info *mfi = file->private_data; struct mtd_oob_ops ops; uint32_t retlen; int ret = 0; @@ -412,7 +413,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd, ops.ooblen = length; ops.ooboffs = start & (mtd->oobsize - 1); ops.datbuf = NULL; - ops.mode = MTD_OOB_PLACE; + ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE; if (ops.ooboffs && ops.ooblen > (mtd->oobsize - ops.ooboffs)) return -EINVAL; diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index d2ee68a..e2b1c90 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2401,7 +2401,14 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to, chip->pagebuf = -1; nand_fill_oob(mtd, ops->oobbuf, ops->ooblen, ops); - status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask); + + if (ops->mode == MTD_OOB_RAW) { + status = 0; + chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); + chip->ecc.write_page_raw(mtd, chip, NULL); + } else { + status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask); + } if (status) return status;
mtd_do_write_oob does not pass information about whether to write in RAW mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or MTD_OOB_PLACE based on the MTD file mode. This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/mtdchar.c | 3 ++- drivers/mtd/nand/nand_base.c | 9 ++++++++- 2 files changed, 10 insertions(+), 2 deletions(-)