On Wed, Aug 06, 2014 at 02:32:18AM +0530, pekon-+F4LgK8oP1VBDgjK7y7TUQ@public.gmane.org wrote:

On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:

quoted

On Thu, 03 Jul 2014, Gupta, Pekon wrote:

quoted

quoted

quoted

+	/* 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 [off-list ref]
    Date:   Mon Jan 30 14:58:32 2012 +0200

        mtd: add leading underscore to all mtd functions

quoted

quoted

- 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.

quoted

quoted

quoted

quoted

+	/* 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html