linux-kernel - Re: [PATCH v3 4/8] mtd: nand: atmel: Avoid ECC errors when leaving backup mode

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] [day] [month] [year] [list]

Message-ID: <CAGkQfmOYg6Usyd++KuGqMjDnaWaP77U5jfOL++3nN7PTcAhh9A@mail.gmail.com>
Date:   Wed, 27 Sep 2017 17:32:10 +0200
From:   Romain Izard <romain.izard.pro@...il.com>
To:     Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Lee Jones <lee.jones@...aro.org>,
        Wenyou Yang <wenyou.yang@...el.com>,
        Josh Wu <rainyfeeling@...look.com>,
        Richard Weinberger <richard@....at>,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        Thierry Reding <thierry.reding@...il.com>,
        Richard Genoud <richard.genoud@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        linux-pwm@...r.kernel.org, linux-usb@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        linux-serial@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 4/8] mtd: nand: atmel: Avoid ECC errors when leaving
 backup mode

2017-09-27 17:08 GMT+02:00 Boris Brezillon <boris.brezillon@...e-electrons.com>:
> On Wed, 27 Sep 2017 10:35:51 +0200
> Romain Izard <romain.izard.pro@...il.com> wrote:
>
>> During backup mode, the contents of all registers will be cleared as the
>> SoC will be completely powered down. For a product that boots on NAND
>> Flash memory, the bootloader will obviously use the related controller
>> to read the Flash and correct any detected error in the memory, before
>> handling back control to the kernel's resuming entry point.
>>
>> But it does not clean the NAND controller registers after use and on its
>> side the kernel driver expects the error locator to be powered down and
>> in a clean state. Add a resume hook for the PMECC error locator, and
>> reset its registers.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@...il.com>
>> ---
>> Change in v3:
>> * keep the PMECC disabled when not in use, and use atmel_pmecc_resume to
>>   reset the controller after the bootloader has left it enabled.
>>
>>  drivers/mtd/nand/atmel/nand-controller.c |  3 +++
>>  drivers/mtd/nand/atmel/pmecc.c           | 22 ++++++++++++++--------
>>  drivers/mtd/nand/atmel/pmecc.h           |  1 +
>>  3 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c
>> index f25eca79f4e5..86c2199380c2 100644
>> --- a/drivers/mtd/nand/atmel/nand-controller.c
>> +++ b/drivers/mtd/nand/atmel/nand-controller.c
>> @@ -2530,6 +2530,9 @@ static __maybe_unused int atmel_nand_controller_resume(struct device *dev)
>>       struct atmel_nand_controller *nc = dev_get_drvdata(dev);
>>       struct atmel_nand *nand;
>>
>> +     if (nand->pmecc)
>> +             atmel_pmecc_resume(nand->pmecc);
>> +
>
> nand is used uninitialized here, and atmel_pmecc_resume() should be
> passed a atmel_pmecc object not a atmel_pmecc_user.
>
>         if (nc->pmecc)
>                 atmel_pmecc_resume(nc->pmecc);
>
And yet I thought I correctly tested this code...

:(

>>       list_for_each_entry(nand, &nc->chips, node) {
>>               int i;
>>
>> diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c
>> index 146af8218314..ff09c0f25dd4 100644
>> --- a/drivers/mtd/nand/atmel/pmecc.c
>> +++ b/drivers/mtd/nand/atmel/pmecc.c
>> @@ -765,6 +765,12 @@ void atmel_pmecc_get_generated_eccbytes(struct atmel_pmecc_user *user,
>>  }
>>  EXPORT_SYMBOL_GPL(atmel_pmecc_get_generated_eccbytes);
>>
>> +void atmel_pmecc_reset(struct atmel_pmecc *pmecc)
>> +{
>> +     writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
>> +     writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
>> +}
>
> It's not used outside of this file, so it should have a static
> specifier. Anyway, I wonder why you don't expose atmel_pmecc_reset()
> directly instead of creating this atmel_pmecc_resume() wrapper.

I will fix this...

-- 
Romain Izard