linux-kernel - Re: [PATCH v2 4/9] 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 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]

Date:   Mon, 18 Sep 2017 11:50:52 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Romain Izard <romain.izard.pro@...il.com>
Cc:     Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Wenyou Yang <wenyou.yang@...el.com>,
        Josh Wu <rainyfeeling@...look.com>,
        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>,
        Alan Stern <stern@...land.harvard.edu>,
        linux-pwm@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
        linux-serial@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 4/9] mtd: nand: atmel: Avoid ECC errors when leaving
 backup mode

Hi Romain,

On Fri, 15 Sep 2017 16:04:06 +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.
> 
> In normal devices, it is up to the driver's suspend/resume code to
> restore the registers in a valid state. But the PMECC is not a regular
> device in the driver model when used with the legacy device tree binding
> for the Atmel NAND controller, and suspend/resume code is not called.
> 
> As in my case the bootloader leaves the PMECC controller in a programmed
> state, and the controller is only reset at boot or after a NAND access,
> the first NAND Flash access with the Atmel controller will report
> uncorrectable ECC errors.
> 
> To avoid this, systematically reset the PMECC controller before using
> it.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@...il.com>
> ---
>  drivers/mtd/nand/atmel/pmecc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c
> index 8c210a5776bc..8d1208f38025 100644
> --- a/drivers/mtd/nand/atmel/pmecc.c
> +++ b/drivers/mtd/nand/atmel/pmecc.c
> @@ -777,6 +777,9 @@ int atmel_pmecc_enable(struct atmel_pmecc_user *user, int op)
>  
>  	mutex_lock(&user->pmecc->lock);
>  
> +	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
> +	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
> +
>  	cfg = user->cache.cfg;
>  	if (op == NAND_ECC_WRITE)
>  		cfg |= PMECC_CFG_WRITE_OP;
> @@ -797,10 +800,6 @@ EXPORT_SYMBOL_GPL(atmel_pmecc_enable);
>  
>  void atmel_pmecc_disable(struct atmel_pmecc_user *user)
>  {
> -	struct atmel_pmecc *pmecc = user->pmecc;
> -
> -	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
> -	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);

So know you leave the ECC engine enabled even when it's not in use? Not
sure what kind of implication this has on power-consumption, but I
think I'd prefer to keep the write RST+DISABLE sequence in the disable
path.

How about creating a atmel_pmecc_reset() function that you'd call from
the nand-controller resume hook. Something like:

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);
}

This way you can re-use the same function and call it from the probe
and disable path as well.

Regards,

Boris

>  	mutex_unlock(&user->pmecc->lock);
>  }
>  EXPORT_SYMBOL_GPL(atmel_pmecc_disable);
> @@ -856,10 +855,6 @@ static struct atmel_pmecc *atmel_pmecc_create(struct platform_device *pdev,
>  	/* Disable all interrupts before registering the PMECC handler. */
>  	writel(0xffffffff, pmecc->regs.base + ATMEL_PMECC_IDR);
>  
> -	/* Reset the ECC engine */
> -	writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL);
> -	writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
> -
>  	return pmecc;
>  }
>