linux-kernel - Re: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190121195354.788199f5@bbrezillon>
Date:   Mon, 21 Jan 2019 19:53:54 +0100
From:   Boris Brezillon <bbrezillon@...nel.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Boris Brezillon <boris.brezillon@...tlin.com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH] mtd: rawnand: check return code of nand_reset() and
 nand_readid_op()

On Tue, 22 Jan 2019 00:57:43 +0900
Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:

> 
> Maybe, is the following better?

Sounds good, even if the original commit message was fine too. I was
just pointing out that nand_scan() should, when possible, be passed
the real number of CS lines connected to the chip instead of the max
number of CS lines supported by the controller.

> 
> 
> ------------------>8-----------------------  
> nand_scan_ident() iterates over maxchips to find as many homogeneous
> chips as possible.
> 
> Since commit 2d472aba15ff ("mtd: nand: document the NAND controller/NAND
> chip DT representation"), new drivers should pass in the exact number of
> CS lines instead of possible max, but old platforms may still rely on
> nand_scan_ident() to detect the actual number of connected CS lines.
> 
> In that case, this loop bails out when manufacturer or device ID
> unmatches. The reason of unmatch is most likely no chip is connected
> to that CS line. If so, nand_reset() should already have failed,
> and the following nand_readid_op() is pointless.
> 
> Before ->exec_op hook was introduced, drivers had no way to tell
> the failure of NAND_CMD_RESET to the framework because the legacy
> ->cmdfunc() has void return type. Now drivers implementing ->exec_op  
> hook can return the error code. You can save nand_readid_op() by
> checking the return value of nand_reset(). The return value of
> nand_readid_op() should be checked as well. If it fails, probably
> id[0] and id[1] are undefined values.
> 
> Just for consistency, it should be sensible to check the return
> code in nand_do_write_oob() as well.