linux-kernel - Re: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic

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: <20191007101337.647300e2@dhcp-172-31-174-146.wireless.concordia.ca>
Date:   Mon, 7 Oct 2019 10:13:37 +0200
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     "Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@...ron.com>
Cc:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Chuanhong Guo <gch981213@...il.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Boris Brezillon <bbrezillon@...nel.org>,
        Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        Richard Weinberger <richard@....at>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Frieder Schrempf <frieder.schrempf@...tron.de>,
        liaoweixiong <liaoweixiong@...winnertech.com>,
        Marek Vasut <marek.vasut@...il.com>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        Jeff Kletsky <git-commits@...ycomm.com>,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>
Subject: Re: [EXT] Re: [PATCH 6/8] mtd: spinand: micron: Turn driver
 implementation generic

On Mon, 19 Aug 2019 09:03:38 +0000
"Shivamurthy Shastri (sshivamurthy)" <sshivamurthy@...ron.com> wrote:

> >   
> > >  static int micron_spinand_detect(struct spinand_device *spinand)
> > >  {
> > > +	const struct spi_mem_op *op;
> > >  	u8 *id = spinand->id.data;
> > > -	int ret;
> > >
> > >  	/*
> > >  	 * Micron SPI NAND read ID need a dummy byte,
> > > @@ -114,16 +102,55 @@ static int micron_spinand_detect(struct  
> > spinand_device *spinand)  
> > >  	if (id[1] != SPINAND_MFR_MICRON)
> > >  		return 0;
> > >
> > > -	ret = spinand_match_and_init(spinand, micron_spinand_table,
> > > -				     ARRAY_SIZE(micron_spinand_table),  
> > id[2]);
> > 
> > I am not sure this is the right solution. I would keep this call and
> > overwrite what you need to overwrite with the fixup hook.
> >   

I'm definitely not comfortable with this whole "rely on ONFi
param-page" thing. Vendors have proven to get it wrong from time to
time, so before we do that, I'd like to make sure all currently
supported Micron NANDs (looks like we only support MT29F2G01ABAGD, so
that shouldn't be hard) expose the right thing there. For instance, are
we sure the ECC layout is always the same, and if not, do we have a
reliable way to extract that?

> 
> Then, I will have dummy structure like below.
> 
> static const struct spinand_info micron_spinand_table[] = {                      
>         SPINAND_INFO(NULL, 0,                                                                    
>                      NAND_MEMORG(0, 0, 0, 0, 0, 0, 0, 0, 0),           
>                      NAND_ECCREQ(0, 0),                                                                       
>                      SPINAND_INFO_OP_VARIANTS(&read_cache_variants,              
>                                               &write_cache_variants,             
>                                               &update_cache_variants),           
>                      0,                                                                                         
>                      SPINAND_ECCINFO(&micron_ooblayout_ops,                      
>                                      micron_ecc_get_status)),                    
> };   

> 
> Let me know if you are thinking for different approach.

Exposing dummy entries is useless. If you're entirely sure all Micron
SPI NANDs have a valid ONFi param page, then no need to use the
ID-based detection. But as I said above, I feel param-page-based
detection is going to be as messy as SFDP-based detection is for SPI
NORs. Vendors tend to make mistakes which we have to fix to make
things work. ID-based detection is much more reliable in this regard,
as long as we don't have ID collisions :P.
Plus, it looks like only a few manufacturers decided to use ONFi param
pages to expose SPI NAND info (AFAICT, only Micron and Macronix do
that), which is not surprising since the ONFi param page has been
created to describe parallel NANDs not SPI NANDs (if you look closely
enough, you'll notice that some fields are meaningless for SPI NANDs).