linux-kernel - Re: [v7, 1/3] nand: pl353: Add basic driver for arm pl353 smc nand interface

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: <20161025044608.GA4929@obsidianresearch.com>
Date:   Mon, 24 Oct 2016 22:46:08 -0600
From:   Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:     Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@...inx.com>
Cc:     Boris Brezillon <boris.brezillon@...e-electrons.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "ezequiel.garcia@...e-electrons.com" 
        <ezequiel.garcia@...e-electrons.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rob@...dley.net" <rob@...dley.net>,
        "galak@...eaurora.org" <galak@...eaurora.org>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>
Subject: Re: [v7, 1/3] nand: pl353: Add basic driver for arm pl353 smc nand
 interface

On Tue, Oct 25, 2016 at 03:58:49AM +0000, Punnaiah Choudary Kalluri wrote:

> > I have hacked the v7 patchset enough to work on 4.8 and hacked it some
> > more to work on my hardware. The driver appears to be in no better
> > shape than the 3.12 out-of-tree Xilinx release I was using previously..
>
> The driver in Xilinx tree works with 4.6 kernel and adopted the
> required

I mean, the driver from the 3.12 Xilinx tree that I last looked
at years ago. This is at v7 now and is still needs lots of work, I did
some fixing, including making parts of it work on 4.8.
You can copy it out of the patch I sent you.

> Changes (may release in 1-2 weeks). Still it need some rework, now a days
> I am seeing many requests from different people for this driver and some of
> Them are using different version of IP as you know this IP has four variants and
> Xilinx is using the pl353 variant.

Well, I am using Xilinx Zynq hardware and care about making that
configuration actually work. This is the last driver I require to use
Zynq with mainline.

It clearly needs more work than just forward porting to 4.8, so please
let me know if you are able to tackle this.

> Let's wait for the next series of patches and Get the patches tested
> with others as well along with the review comments.

You now have 10 review comments from me, please respond to all of them in
your v8 patchset - no sense in continuing to drag this out.

Please cc me on future series.

Jason

> Regards,
> Punnaiah
> > I've attached the ugly, ugly patch I made, it may save you some time
> > when preparing v8.
> > 
> > Commentary:
> >  1) nand_chip already includes mtd_info, no reason for a second one in
> >     the pl353_nand_info struct. The standard accessors mtd_to_nand/etc
> >     should be used instead of priv
> >  2) I hacked out all the ECC stuff from the driver since I don't use
> >     it and it was broken.. Obviously some has to come back, but fixing
> >     other things on this list first will make that much easier and better..
> >  3) pl353_nand_write_page_swecc/pl353_nand_read_page_swecc are pure
> >     outdated copies of the core routines and should not exist in a
> >     driver. This needs to be fixed so nand_scan_tail sets them.
> >     This seems to be a side effect of #9 ?
> >  4) The hacky stuff to detect 2 vs 3 address cycle NAND doesn't work,
> >     and doesn't work without ONFI (see patch for possible fix). The
> >     driver should probably use the same scheme the core code uses..
> >     But this is all a problem because of #10
> >  5) The driver assumes alignment of the iomap, needs to use + not |
> >     when constructing the address. (yes, this blows up on my system)
> >  6) Driver unconditionally sets timing to ONFI mode 0 (slow!)
> >     Maybe timing should be common code driven by DT..
> >  7) Driver unconditionally selects a BBT format, and an ECC layout
> >     (neither match what my system uses, but I guess that is a core mtd
> >     issue these days :/)
> >  8) Driver unconditionally forces a chip-delay (wrong for my
> >     system) maybe this should be common code driven by DT..
> >  9) This buisness with pl353_nand_ecc_init and the
> >     special functions to do PL353_NAND_LAST_TRANSFER_LENGTH stuff
> >     is just horrid. I'd expect that is a big NAK.
> > 
> >     The driver needs to implement read_buf *properly* so the core
> >     routines can be used instead of trying to 'fix' the call sites
> >     to do the PL353_NAND_LAST_TRANSFER_LENGTH stuff.
> >  10) pl353_nand_cmd_function is a wonky copy of nand_command_lp,
> > maybe
> >      this driver should use cmd_ctrl, or the core code should be
> >      refactored some more..