Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752516AbbHSVZt (ORCPT ); Wed, 19 Aug 2015 17:25:49 -0400 Received: from mail-lb0-f173.google.com ([209.85.217.173]:35894 "EHLO mail-lb0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751457AbbHSVZs convert rfc822-to-8bit (ORCPT ); Wed, 19 Aug 2015 17:25:48 -0400 MIME-Version: 1.0 In-Reply-To: <1440009015-3765-3-git-send-email-robert.jarzmik@free.fr> References: <1440009015-3765-1-git-send-email-robert.jarzmik@free.fr> <1440009015-3765-3-git-send-email-robert.jarzmik@free.fr> Date: Wed, 19 Aug 2015 18:25:46 -0300 Message-ID: Subject: Re: [PATCH RESEND 2/2] mtd: nand: pxa3xx-nand: fix random command timeouts From: Ezequiel Garcia To: Robert Jarzmik Cc: Ezequiel Garcia , David Woodhouse , Brian Norris , "linux-mtd@lists.infradead.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2994 Lines: 82 On 19 August 2015 at 15:30, 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. > > Signed-off-by: Robert Jarzmik > --- > drivers/mtd/nand/pxa3xx_nand.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index d1a4c336de1d..d6c696798811 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -675,8 +675,14 @@ static irqreturn_t pxa3xx_nand_irq(int irq, void *devid) > is_ready = 1; > } > > + /* > + * Clear all status bit before issuing the next command, which > + * can and will alter the status bits and will deserve a new > + * interrupt on its own. This lets the controller exit the IRQ > + */ > + nand_writel(info, NDSR, status); > + Actually, the comment I had in mind was more about why we *cannot* clear the NDSR register before waking the IRQ thread. But this is a nitpick: I don't care much :-) > if (status & NDSR_WRCMDREQ) { > - nand_writel(info, NDSR, NDSR_WRCMDREQ); > status &= ~NDSR_WRCMDREQ; > info->state = STATE_CMD_HANDLE; > > @@ -697,8 +703,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 García, VanguardiaSur www.vanguardiasur.com.ar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/