| 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
| ||
|
Message-ID: <DM6PR02MB4779D2DBBD94ACE5E4C2FD76AFC80@DM6PR02MB4779.namprd02.prod.outlook.com>
Date: Thu, 18 Jul 2019 12:32:23 +0000
From: Naga Sureshkumar Relli <nagasure@...inx.com>
To: Boris Brezillon <boris.brezillon@...labora.com>
CC: "miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>,
"bbrezillon@...nel.org" <bbrezillon@...nel.org>,
"richard@....at" <richard@....at>,
"dwmw2@...radead.org" <dwmw2@...radead.org>,
"computersforpeace@...il.com" <computersforpeace@...il.com>,
"marek.vasut@...il.com" <marek.vasut@...il.com>,
"vigneshr@...com" <vigneshr@...com>,
"yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Michal Simek <michals@...inx.com>,
Srikanth Vemula <svemula@...inx.com>,
"nagasuresh12@...il.com" <nagasuresh12@...il.com>
Subject: RE: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over
write driver's read_page()/write_page()
Hi Boris,
> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@...labora.com>
> Sent: Wednesday, July 17, 2019 2:34 PM
> To: Naga Sureshkumar Relli <nagasure@...inx.com>
> Cc: miquel.raynal@...tlin.com; bbrezillon@...nel.org; richard@....at;
> dwmw2@...radead.org; computersforpeace@...il.com; marek.vasut@...il.com;
> vigneshr@...com; yamada.masahiro@...ionext.com; linux-mtd@...ts.infradead.org; linux-
> kernel@...r.kernel.org; Michal Simek <michals@...inx.com>; Srikanth Vemula
> <svemula@...inx.com>; nagasuresh12@...il.com
> Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write
> driver's read_page()/write_page()
>
> On Wed, 17 Jul 2019 10:21:56 +0200
> Boris Brezillon <boris.brezillon@...labora.com> wrote:
>
> > On Wed, 17 Jul 2019 09:55:25 +0200
> > Boris Brezillon <boris.brezillon@...labora.com> wrote:
> >
> > > On Wed, 17 Jul 2019 05:33:35 +0000
> > > Naga Sureshkumar Relli <nagasure@...inx.com> wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > > -----Original Message-----
> > > > > From: Boris Brezillon <boris.brezillon@...labora.com>
> > > > > Sent: Tuesday, July 16, 2019 1:15 PM
> > > > > To: Naga Sureshkumar Relli <nagasure@...inx.com>
> > > > > Cc: miquel.raynal@...tlin.com; bbrezillon@...nel.org;
> > > > > richard@....at; dwmw2@...radead.org;
> > > > > computersforpeace@...il.com; marek.vasut@...il.com;
> > > > > vigneshr@...com; yamada.masahiro@...ionext.com;
> > > > > linux-mtd@...ts.infradead.org; linux- kernel@...r.kernel.org;
> > > > > Michal Simek <michals@...inx.com>; Srikanth Vemula
> > > > > <svemula@...inx.com>; nagasuresh12@...il.com
> > > > > Subject: Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do
> > > > > not over write driver's read_page()/write_page()
> > > > >
> > > > > On Tue, 16 Jul 2019 09:31:37 +0200 Boris Brezillon
> > > > > <boris.brezillon@...labora.com> wrote:
> > > > >
> > > > > > On Mon, 15 Jul 2019 23:30:51 -0600 Naga Sureshkumar Relli
> > > > > > <naga.sureshkumar.relli@...inx.com> wrote:
> > > > > >
> > > > > > > Add check before assigning chip->ecc.read_page() and
> > > > > > > chip->ecc.write_page()
> > > > > > >
> > > > > > > Signed-off-by: Naga Sureshkumar Relli
> > > > > > > <naga.sureshkumar.relli@...inx.com>
> > > > > > > ---
> > > > > > > Changes in v18
> > > > > > > - None
> > > > > > > ---
> > > > > > > drivers/mtd/nand/raw/nand_micron.c | 7 +++++--
> > > > > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > b/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > index cbd4f09ac178..565f2696c747 100644
> > > > > > > --- a/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > +++ b/drivers/mtd/nand/raw/nand_micron.c
> > > > > > > @@ -500,8 +500,11 @@ static int micron_nand_init(struct nand_chip *chip)
> > > > > > > chip->ecc.size = 512;
> > > > > > > chip->ecc.strength = chip->base.eccreq.strength;
> > > > > > > chip->ecc.algo = NAND_ECC_BCH;
> > > > > > > - chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > > > - chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
> > > > > > > + if (!chip->ecc.read_page)
> > > > > > > + chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
> > > > > > > +
> > > > > > > + if (!chip->ecc.write_page)
> > > > > > > + chip->ecc.write_page =
> > > > > > > +micron_nand_write_page_on_die_ecc;
> > > > > > >
> > > > > >
> > > > > > Seriously?! I told you this was inappropriate and you keep
> > > > > > sending this patch. So let's make it clear:
> > > > > >
> > > > > > Nacked-by: Boris Brezillon <boris.brezillon@...labora.com>
> > > > > >
> > > > > > Fix your controller driver instead of adding hacks to the Micron logic!
> > > > >
> > > > > Not even going to review the other patch: if you have to do
> > > > > that, that means the driver is broken. On a side note, this
> > > > > patch series is still not threaded as it should be and it's a v18 for a damn NAND
> controller driver! Sorry but you reached the limit of my patience. Please find
> > > > > someone to help you with that task.
> > > > My intention is not to resend this 1/2 again. Sorry for that.
> > > > We already had some discussion on [v17 1/2], https://lkml.org/lkml/2019/6/26/430
> > > > And there we didn't conclude that raw_read()/writes().
> > >
> > > Yes, looks like I never replied to that one, but I think my previous
> > > explanation were clear enough to not argue on that aspect any
> > > longer/
> > >
> > > > So I thought that, will send updated driver along with this patch, then will get more
> information about
> > > > The issue on the latest driver review.
> > >
> > > More on that topic. I don't think you ever tested on-die ECC on a
> > > Micron NAND, otherwise you would have noticed that your solution
> > > completely bypasses the on-die ECC logic (and this will clearly
> > > break existing on-die ECC users). See, that's what I'm complaining
> > > about, Looks like you don't really understand what you're doing.
> > >
> > > > There is nothing like keep on sending this patch, As you people
> > > > are experts in the driver review, if this patch is a hack, then we will definitely fix that in
> controller driver. I will find a way to do that.
> > > >
> > > > But in this flow of patch sending, if the work I did hurts you, then I am really sorry for
> that.
> > >
> > > I'm not offended, just tired going through the same driver over and
> > > over again, reporting things that are wrong/inappropriate to then
> > > realize you only addressed of a tiny portion of it in the following
> > > version. My last reviews were rather incomplete because of that, and
> > > now I'm giving up.
> > >
> > > > Will fix this issue in the controller driver and will send the updated one.
> > >
> > > How? You say you'll fix the issue but I'm not even sure you
> > > understand what the issue is? Clearly, the patch you've posted
> > > doesn't fix anything, it's just papering over the fact that your
> > > controller driver is not supporting raw accesses (or at least, not
> > > supporting it properly).
> > >
> > > Have you even looked at the datasheet you pointed to in patch 2 [1]?
> > > Just went through it, and found a field that's supposed to control
> > > the ECC engine activation: ecc_memcfg.ecc_mode. I don't see anything
> > > changing that field in your code, so I guess raw accesses are actually
> > > not really happening with the ECC engine disabled...
> >
> > Looks like I was wrong about that part, you seem to call
> > pl353_smc_set_ecc_mode(), just not in the right place (this should be
> > done in the read/write_page_raw()).
>
> I'm wrong again. ECC should be in BYPASS mode by default and you should enable it when
> entering pl353_nand_{read,write}_page() and disable it (put it back to BYPASS mode) when
> leaving those functions. This way, you don't even have to implement your own raw accessors
> and can use
> nand_{read,write}_page_raw() instead.
Yes, ECC should be in BYPASS mode, and we are already doing that in pl353_ecc_init(),
I will move that to page read/writes.
The PL353 driver is not configuring properly the third row address cycle, i.e. when chip->options is set
With NAND_ROW_ADDR_3, which says third row address cycle is needed.
In the drivers ->exec_op() method, it is just setting three row address cycles but not writing
The row address properly during NAND_OP_ADDR_INSTR.
I fixed that and now as you said, without pl353_nand_read_page_raw() and pl353_nand_write_page_raw()
The on-die page read/writes are working.
I have tested ubifs and I see no issues with that.
Thanks,
Naga Sureshkumar Relli
Powered by blists - more mailing lists