[PATCH] mtd: nand: stm_nand_bch: add new driver

[Brian Norris]

On Wed, Aug 06, 2014 at 02:32:18AM +0530, pekon at pek-sem.com wrote:
> On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
> >On Thu, 03 Jul 2014, Gupta, Pekon wrote:
> >>>>+	/* Load last page of block */
> >>>>+	offs = (loff_t)block << chip->phys_erase_shift;
> >>>>+	offs += mtd->erasesize - mtd->writesize;
> >>>>+	if (bch_read_page(nandi, offs, buf) < 0) {
> >>
> >>*Important*: You shouldn't call internal functions directly here..
> >>Instead use chip->_read() OR mtd-read() that will:

I'm not sure exactly what you're saying in the above line (chip->_read()
is not a function, and and mtd-read() is syntactically incorrect),
but...

You most certainly do *not* want to call mtd->_read() directly (or any
of the callbacks prefixed with underscores). That is one of the main
purposes of:

    commit 3c3c10bba1e4ccb75b41442e45c1a072f6cded19
    Author: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
    Date:   Mon Jan 30 14:58:32 2012 +0200

        mtd: add leading underscore to all mtd functions

> >>- keep this code independent of ECC modes added in your driver
> >>in future.
> >>- implicitly handle updating mtd->ecc_stat (like ecc_failed,
> >>bits_corrected).
> >>- implicitly take care of address alignments checks and offset
> >>calculations.
> >>
> >><Same applies to other places where you have directly used
> >>bch_read_page())
> >
> >Yourself and Brian seem to disagree on this point.  Which is correct?

I think you've worked out a decent solution in your latest series, but
a few of the guiding principles:

 * BBT code can rely on generic NAND implementation details (e.g.,
   chip->options), but it can also act as an MTD user (i.e., use the
   mtd_*() APIs)

 * It should not rely on details of the particular NAND driver (e.g.,
   calling your ST bch_read_page())

 * Look at similar code and try to follow its pattern where it makes
   sense. So while nand_bbt.c is not a shining example in all regards, I
   think it does OK in terms of some of the key points of layering.

> >>>>+	/* Is block already considered bad? (will also catch
> >>>>invalid offsets) */
> >>>>+	ret = mtd_block_isbad(mtd, offs);
> >>>
> >>>You're violating some of the layering here; the low-level driver
> >>>probably shouldn't be calling the high-level mtd_*() APIs. On
> >>>a similar
> >>>note, I'm not 100% confident that the nand_base/nand_bbt
> >>>separation is
> >>>written cleanly enough for easy maintenance of your nand_base
> >>>+ ST BBT
> >>>code in parallel. I might need a second look at that, and I think
> >>>modularizing your BBT code to a separate file could help ease
> >>>this a
> >>>little.
> >
> >... here is the converse argument.

I think I clarified this one; I misspoke about the mtd_*() APIs.

> I think somewhere in earlier comments, Brian also supported
> to use high-level function like mtd_read(). Also, nand_bbt.c
> itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
> at many places. So I'll let Brain decide here.
> But having low-level function will add redundant code for
> re-checking and aligning the addresses boundaries to block
> and page/sector sizes.

Not all checks are redundant. It's redundant to have the direct
descendant doing the same checks that the parent does, like:

  mtd_read() (checks boundaries)
  |_ mtd->_read() (e.g., nand_read())

So nand_read() and nand_do_read_ops() don't need the checking.

But for a BBT implementation, it can help to make sure that its
page/block/etc. arithmetic fits the right bounds when it ends up
deciding to scan a block.

Now, it still may be easy to prove that the checks are
redundant/unnecessary for correctly-written code, but if the layering
makes sense, then it still may be a better choice to simply use the
high-level, self-checking API than to try to dig deeper. For instance,
I'm pretty sure UBI does some checks to make sure it's not reading off
the end of an MTD, but it still calls mtd_read() because it's The Right
Thing (TM).

Brian