linux-kernel - Re: [PATCH] mtd: nand: pxa3xx-nand: fix random command timeouts

Open Source and information security mailing list archives

Date:	Sun, 16 Aug 2015 12:29:27 -0300
From:	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
To:	Robert Jarzmik <robert.jarzmik@...e.fr>
Cc:	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Brian Norris <computersforpeace@...il.com>,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Antoine Tenart <antoine.tenart@...e-electrons.com>
Subject: Re: [PATCH] mtd: nand: pxa3xx-nand: fix random command timeouts

On 12 Aug 06:22 PM, Robert Jarzmik wrote:
> When 2 commands are submitted in a row, and the second is very quick,
> the completion of the second command might never come. This happens
> especially if the second command is quick, such as a status read after
> an erase.
> 
> The issue is that in the interrupt handler, the status bits are cleared
> after the new command is issued. There is a small temporal window where
> this happens :
>  - the previous command has set the command done bit
>  - the ready for a command bit is set
>  - the handler submits the next command
>    - just then, the command completes, and the command done bit is still
>      set
>  - the handler clears the "previous" command done bit
>  - the handler exits
> 
> In this flow, the "command done" of the next command will never trigger
> a new interrupt to finish the status command, as it was cleared for both
> commands.
> 
> Fix this by clearing the status bit before submitting a new command.
> 

This fix looks correct. Thanks!

Couple questions:

1. In which platform are you seeing this bug?
2. Is this a regression? (i.e. should we queue it for -stable?)

Also, one might question why we can't just write NDSR right after it's read,
before we wake the IRQ thread or start DMA. It appears this is
a requirement of BCH, as per the comment in drain_fifo.

It would be nice to put a comment explaining why we clear NDSR only
before the check to WRCMDREQ. Maybe even copy-pasting something
from the commit log?

I'd like to say "Yay, let's pick it" but I'd like to make sure this is
tested on all platforms first (unless you've tested it already).

> Signed-off-by: Robert Jarzmik <robert.jarzmik@...e.fr>
> ---
>  drivers/mtd/nand/pxa3xx_nand.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index b0737aec7caf..718097414b9c 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -687,8 +687,10 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  		is_ready = 1;
>  	}
>  
> +	/* clear NDSR to let the controller exit the IRQ */
> +	nand_writel(info, NDSR, status);
> +
>  	if (status & NDSR_WRCMDREQ) {
> -		nand_writel(info, NDSR, NDSR_WRCMDREQ);
>  		status &= ~NDSR_WRCMDREQ;
>  		info->state = STATE_CMD_HANDLE;
>  
> @@ -709,8 +711,6 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid)
>  			nand_writel(info, NDCB0, info->ndcb3);
>  	}
>  
> -	/* clear NDSR to let the controller exit the IRQ */
> -	nand_writel(info, NDSR, status);
>  	if (is_completed)
>  		complete(&info->cmd_complete);
>  	if (is_ready)
> -- 
> 2.1.4
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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