linux-kernel - Re: [PATCH] mtd: nand: stm_nand

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <53F4E2BD.5080205@pek-sem.com>
Date:	Wed, 20 Aug 2014 23:32:37 +0530
From:	pekon <pekon@...-sem.com>
To:	Brian Norris <computersforpeace@...il.com>
CC:	devicetree@...r.kernel.org,
	Boris BREZILLON <b.brezillon.dev@...il.com>,
	kernel@...inux.com, linux-kernel@...r.kernel.org,
	linux-mtd@...ts.infradead.org,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	Lee Jones <lee.jones@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] mtd: nand: stm_nand_bch: add new driver

On Tuesday 19 August 2014 07:42 AM, Brian Norris wrote:
> On Wed, Aug 06, 2014 at 02:32:18AM +0530, pekon@...-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@...ux.intel.com>
>      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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/