linux-kernel - Re: [PATCH v2 02/10] mtd: rawnand: denali: refactor syndrome layout handling for raw access

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: <CAK7LNASeBDxsUj2fu0bh9G6dXV2jxeHp6iBG+8FytNbHGtVHKQ@mail.gmail.com>
Date:   Tue, 5 Mar 2019 18:20:22 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     Boris Brezillon <bbrezillon@...nel.org>,
        Richard Weinberger <richard@....at>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Marek Vasut <marek.vasut@...il.com>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH v2 02/10] mtd: rawnand: denali: refactor syndrome layout
 handling for raw access

Hi Miquel,

On Mon, Mar 4, 2019 at 6:01 PM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Masahiro,
>
> Masahiro Yamada <yamada.masahiro@...ionext.com> wrote on Tue, 12 Feb
> 2019 16:12:54 +0900:
>
> > The Denali IP adopts the syndrome page layout (payload and ECC are
> > interleaved). The *_page_raw() and *_oob() callbacks are complicated
> > because they must hide the underlying layout used by the hardware,
> > and always return contiguous in-band and out-of-band data.
> >
> > Currently, similar code is duplicated to reorganize the data layout.
> > For example, denali_read_page_raw() and denali_write_page_raw() look
> > almost the same.
> >
> > The idea for refactoring is to split the code into two parts:
> >   [1] conversion of page layout
> >   [2] what to do at every ECC chunk boundary
> >
> > For [1], I wrote denali_raw_payload_op() and denali_raw_oob_op().
> > They manipulate data for the Denali controller's specific page layout
> > of in-band, out-of-band, respectively.
>
> Could you please comment the purpose of these two functions in the code
> as well?


OK, I will.



> >
> > The difference between write and read is just the operation at
> > ECC chunk boundaries. For example, denali_read_oob() calls
> > nand_change_read_column_op(), whereas denali_write_oob() calls
> > nand_change_write_column_op(). So, I implemented [2] as a callback
> > passed into [1].
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> > ---
> >
> > Changes in v2: None
> >
> > drivers/mtd/nand/raw/denali.c | 354 +++++++++++++++++++-----------------------
> > 1 file changed, 163 insertions(+), 191 deletions(-)
>
> Too bad that the size of the driver did not shrink more than that :)

Indeed, less than expected.

But, please do not miss this commit is adding
error check to every operation.

Prior to this commit, the code just ignored the return code
because 97d90da8a886949f simply replaced old hooks
despite the new ones return the error code.


Generally, every error check costs two lines
in the following form:


  ret = (do something)
  if (ret)
           return ret;



> > +
> > +static int denali_memcpy_in(void *buf, unsigned int offset, unsigned int len,
> > +                         void *priv)
> > +{
> > +     memcpy(buf, priv + offset, len);
> > +     return 0;
> >  }
>
> Maybe this "callback" and the (_out cousin) could be part of you
> controller's structure,
> and you could use a read/write flag instead of
> passing the functions' pointer?

This is what the old code does.

There are 4 callbacks for the combination
of raw/oob, and write/read.

I do not know how your suggestion looks like,
and whether it will make the code cleaner.



--
Best Regards
Masahiro Yamada