Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Artem Bityutskiy <dedekind1@gmail.com>]

On Mon, 2011-08-22 at 16:42 -0700, Brian Norris wrote:
> On Mon, Aug 22, 2011 at 3:02 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> >> Does MTD_MODE_RAW imply MTD_OOB_RAW when OOB operations are involved?
> >
> > I would make sure these 2 set of macros have different prefixes.
> > Probably re-naming the internal ones is easier, but I'd anyway re-named
> > the external ones - indeed they are about access mode for a specific
> > file descriptor, so I'd use the "MTD_FILE_MODE_" prefix.
> 
> To be sure, are you categorizing the internal/external modes as follows?
> 
> External = MTD_MODE_NORMAL, MTD_MODE_RAW, etc.
> Internal = MTD_OOB_PLACE, MTD_OOB_AUTO, MTD_OOB_RAW

Yes :-)

> Because my proposal here is to move those Internal ones to becoming a
> new set of External modes for the purpose of the new MEMWRITEDATAOOB.

Right, I was talking about what is in mainline at the moment.

> Finding good names for them makes more sense now than after they
> become External. But as you say, renaming the *current* External modes
> could help clarify things as well.

I think you can re-name any of them nicely - we can always amend
mtd-utils.

> 
> >> Does MTD_MODE_RAW imply that no ECC is applied to data?
> >
> > Not sure, but judging from how it is used - no.
> 
> I see at least one instance of code that seems to assume the answer is
> "yes"; see the "noecc" code in nandwrite.c. Where do you find evidence
> that says "no"?

You are right, this looks like "no ECC" actually.

Well, my cscopes find only 2 places where this symbol is used in the
kernel:

   2    220  drivers/mtd/mtdchar.c <<mtd_read>>
             case MTD_MODE_RAW:
   3    316  drivers/mtd/mtdchar.c <<mtd_write>>
             case MTD_MODE_RAW:

So this is only about reading/writing and nothing else.

Then, let's look at one of them, say 3:

drivers/mtd/mtdchar.c: ret = mtd->write_oob(mtd, *ppos, &ops);

Then find out in nand_base.c that this maps to 'nand_write_oob()',
which, in-turn calls then 'nand_do_write_ops()', and which then calls,
_I guess_:

               ret = chip->write_page(mtd, chip, wbuf, page, cached,
                                       (ops->mode == MTD_OOB_RAW));

which then calls:

chip->ecc.write_page_raw(mtd, chip, buf);

which seems to be the "no ECC" write function.

But I guess MEMWRITEDATAOOB should anyway "override" the "global" mode.

> > I think MEMWRITEDATAOOB should ignore the mode.
> 
> Sure. May be easier said than done though. It looks like there are too
> many entry points into the MTD layer, with different ioctls plus the
> standard read/write controls. I'll see what I can do about keeping
> this under control.

Yeah, it is a mess, I agree :-(

> It looks like going forward, we should agree on something like the
> following, although the names still may change:
> 
> 1) the generic term "raw" mode means that we are writing or reading
> data exactly as-is to/from the flash, without error correction applied
> to it.

Right.

> 2) MTD_OOB_RAW means means that we read or write in raw mode. This can
> be directly user-controlled from the new MEMWRITEDATAOOB ioctl.

OK

> 3) MTD_MODE_RAW is used only with the MTDFILEMODE ioctl, and it *only*
> affects error correction for read() and write() calls to the
> corresponding file descriptor. Its sole purpose is to enable
> MTD_OOB_RAW during standard read/write operations (no OOB
> interaction).

OK

> Unfortunately, item 3 showcases our naming inconsistency, since
> MTD_OOB_RAW is actually utilized for a non-OOB operation. Perhaps this
> should be considered for renaming?

Probably the "OOB" part should go away? Give the "per-file" modes some
other name which suggests that these are old-fashioned and then use
MTD_RW_MODE_ or even just MTD_MODE_ for the "internal" constants?

> Speaking of renaming, I'm not so sure about the name I gave this
> ioctl; which of the following makes the most sense for naming the new
> ioctl? Speak soon or forever hold your peace.
> 1) MEMWRITEDATAOOB
> 2) MEMWRITE
> 3) MEMWRITEOPS
> 4) ...something else?

1 or 2 seems OK to me, but I do not have a strong opinion here.

-- 
Best Regards,
Artem Bityutskiy