Re: [PATCH v3 09/10] mtd: nand: utilize oob_required parameter

[Brian Norris <computersforpeace@gmail.com>]

On Sun, Apr 29, 2012 at 5:47 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Fri, 27 Apr 2012 18:29:53 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>> Don't read/write OOB if the caller doesn't requre it.
>
> Re-thinking this, I think this might be incorrect.
> Sorry I havn't noticed this earlier.
>
> Suppose the nand chip is working in "sequential" (aka "incremental")
> mode.
> Meaning, a NAND_CMD_READ0 command is issued, and then adjacent pages are
> read sequencially, without having to re-issue NAND_CMD_READ0.
>
> (see for example the loop within 'nand_do_read_ops', especially the
> 'sndcmd' variable).
>
> In that case, we MUST read the 'oob', because the OOB bytes will always
> arrive on the bus following the inband data.
...

I see. This is the kind of issue I was alluding to back in v2:

"For instance, is there any sort of hardware that expects the whole
page + OOB to be read via chip->read_buf() for all reads..."

This situation comes up if NAND_NO_AUTOINCR is not set. But really, it
looks like we *always* have NAND_NO_AUTOINCR enabled, and so we
*always* send a new READ cmd. I know that it's possible for some board
driver to override this, but I don't see that anywhere...

> I have no idea if this issue is also relevant for 'read_page' implementors
> other than of nand_base.c; should carefully review.
>
> There are 2 options for fixing the issue in nand_base.c:
>
> - Disregard the 'oob_required' parameter in all nand_base's
>  'read_page' implementations (not future proof, someone might miss the
>  nuance; also bug might still exist in the non-default implementations)
>
> - Fix your 02/10 patch, in away that the passed 'oob_required' argument
>  will be somehow set according to the 'snd_cmd'

I'm fine with adding an 'else' to the 'if (likely(sndcmd))', so that
we get something like this in patch 2:

               if (likely(sndcmd)) {
                       chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
                       sndcmd = 0;
+              } else {
+                      /* force driver to read OOB, for sequential read */
+                      oob_required = 1;
               }

I think that would take care of the corner case where we use autoincrement.

Brian