Re: [LINUX PATCH v18 1/2] mtd: rawnand: nand_micron: Do not over write driver's read_page()/write_page()

[Boris Brezillon]

On Wed, 17 Jul 2019 10:21:56 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

> On Wed, 17 Jul 2019 09:55:25 +0200
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> 
> > On Wed, 17 Jul 2019 05:33:35 +0000
> > Naga Sureshkumar Relli <nagasure@xxxxxxxxxx> wrote:
> >   
> > > Hi Boris,
> > >     
> > > > -----Original Message-----
> > > > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > > Sent: Tuesday, July 16, 2019 1:15 PM
> > > > To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> > > > Cc: miquel.raynal@xxxxxxxxxxx; bbrezillon@xxxxxxxxxx; richard@xxxxxx;
> > > > dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx;
> > > > vigneshr@xxxxxx; yamada.masahiro@xxxxxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-
> > > > kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Srikanth Vemula
> > > > <svemula@xxxxxxxxxx>; nagasuresh12@xxxxxxxxx
> > > > 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@xxxxxxxxxxxxx> wrote:
> > > >       
> > > > > On Mon, 15 Jul 2019 23:30:51 -0600
> > > > > Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx> wrote:
> > > > >      
> > > > > > Add check before assigning chip->ecc.read_page() and
> > > > > > chip->ecc.write_page()
> > > > > >
> > > > > > Signed-off-by: Naga Sureshkumar Relli
> > > > > > <naga.sureshkumar.relli@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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@xxxxxxxxxxxxx>
> > > > >
> > > > > 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.