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

[pekon]

On Tuesday 19 August 2014 07:42 AM, Brian Norris wrote:
> On Wed, Aug 06, 2014 at 02:32:18AM +0530, pekon@xxxxxxxxxxx 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@xxxxxxxxxxxxxxx>
>      Date:   Mon Jan 30 14:58:32 2012 +0200
>
>          mtd: add leading underscore to all mtd functions
Makes sense. Thanks for pointing this.

[...]

> > 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
Agree.. re-using mtd_*() API for non-critical paths is justified.


with regards, pekon

------------------------
Powered by BigRock.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/