From patchwork Tue Jan 30 09:25:03 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Martin X-Patchwork-Id: 10191457 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5D2066056E for ; Tue, 30 Jan 2018 09:25:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6052D28975 for ; Tue, 30 Jan 2018 09:25:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5503828987; Tue, 30 Jan 2018 09:25:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A0F4228985 for ; Tue, 30 Jan 2018 09:25:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=WBCKbnEUy//nSxrVV7A2jLU6sRmWSckkKaRL0yG7fRU=; b=LCh+wO09Yu6MrY AC2re0t2dBSfod6Xp07yf5w9sCCSzpK/JBfCYC3g1hgvQRjyCNzViBJE+6S5IBf0JSZT54+AjFRXk A/D3LZKqpyU19kXM7BdSEkxFrOfMeIsEhIOFr0YbtPgxloDjvKDLB0P4lR4zgyCY2VjG+5B7/lHc8 htoiekYMh2DIsveZlXlP9sOYRmJsLjB2eHoh4+kYfndIZDs/XxuADPiYEA82sd+SKBZw6rJ/ncXmE kd7ktA577BOjeHJtD7qcfoPwsrNWASWFUZEUpzVTvvnqt4GQzZhUcGUPMDmbX7QirE9lDwtihQWBb zoDgLv0nVXXgowiXn4vw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1egSAO-0007mn-OW; Tue, 30 Jan 2018 09:25:24 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1egSAH-0007CU-IU for linux-arm-kernel@lists.infradead.org; Tue, 30 Jan 2018 09:25:20 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E7B2D80D; Tue, 30 Jan 2018 01:25:06 -0800 (PST) Received: from e103592.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D7F1B3F25C; Tue, 30 Jan 2018 01:25:05 -0800 (PST) Date: Tue, 30 Jan 2018 09:25:03 +0000 From: Dave Martin To: Wei Xu Subject: Re: [RFC PATCH] tty: pl011: Avoid stuck-off spurious interrupts Message-ID: <20180130092502.GD5862@e103592.cambridge.arm.com> References: <1517242181-20652-1-git-send-email-Dave.Martin@arm.com> <5A6F5EDB.20405@hisilicon.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5A6F5EDB.20405@hisilicon.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Linus Walleij , Russell King , linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Jan 29, 2018 at 05:50:19PM +0000, Wei Xu wrote: > Hi Dave, > > On 2018/1/29 16:09, Dave Martin wrote: > > Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts") > > clears the RX and receive timeout interrupts on pl011 startup, to > > avoid a screaming-interrupt scenario that can occur when the > > firmware or bootloader leaves these interrupts spuriously > > asserted. > > > > This has been noted as an issue when running Linux on qemu [1]. > > > > Unfortunately, the above fix seems to lead to potential > > misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously > > on driver startup, if the RX FIFO is also already full to the > > trigger level. > > > > Clearing the RX FIFO interrupt does not change the FIFO fill level. > > In this scenario, because the interrupt is now clear and because > > the FIFO is already full to the trigger level, no new assertion of > > the RX FIFO interrupt can occur unless the FIFO is drained back > > below the trigger level. This never occurs because the pl011 > > driver is waiting for an RX FIFO interrupt to tell it that there is > > something to read, and does not read the FIFO at all until that > > interrupt occurs. > > > > Thus, simply clearing "spurious" interrupts on startup may be > > misguided, since there is no way to be sure that the interrupts are > > truly spurious, and things can go wrong if they are not. > > > > This patch attempts to handle (suspected) spurious interrupts more > > robustly, by allowing the interrupt(s) to fire but quenching the > > scream. > > > > pl011_int() runs and attempts to drain the FIFO anyway just as if > > the interrupts were real. If the FIFO is already empty, great. To > > avoid a screaming spurious interrupt, the RX FIFO and timeout > > interrupts are now explicitly cleared in between committing to > > drain the RX FIFO and actually draining it. We do not have to > > worry about lost interrupts here, because we are effectively in > > polled mode inside pl011_int() until the RX FIFO becomes empty: > > > > * A new char received before the RX FIFO is fully drained will be > > drained out synchronously by pl011_int() along with the other > > chars already pending. A new char received after the RX FIFO > > is drained will result in correct RX FIFO interrupt assertion, > > because emptying the RX FIFO guarantees that the RX FIFO / > > timeout interrupt state machines are back in a sane state. > > > > * A new RX timeout before the RX FIFO is fully drained is no > > problem, because pl011_int() has already committed to emptying > > the FIFO at this point, guaranteeing that no stray chars will > > be left behind. A new RX timeout after the RX FIFO is fully > > drained will result in correct interrupt assertion. > > > > This patch does not attempt to address the case where the RX FIFO > > fills faster than it can be drained: that is a pathological > > condition that is beyond the scope of the driver to work around. > > Users cannot expect this to work unless they enable hardware flow > > control. > > > > [1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo > > before enabled the interruption > > https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html > > > > Reported-by: Wei Xu > > Cc: Russell King > > Cc: Linus Walleij > > Cc: Peter Maydell > > Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts") > > Signed-off-by: Dave Martin > > --- > > drivers/tty/serial/amba-pl011.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > After commented the 1549 line in the amba-pl011.c[1] and applied this patch, > the console is not hanged any more. > > The UART011_ICR is cleared in the pl011_hwinit that will clear the > RX interruption as well. > Is it OK to remove 1549 as well? > Thanks! Hmm, that probably does make sense: I was looking only at the pl011_startup() path, but the clearing of interrupts in pl011_hwinit() appears like it can lead to the same problem, as you say. So I think that it makes sense to remove RTIS and RXIS from the status bit clearing on line 1550. To avoid accidentally breaking any existing behaviour, it may be safer to leave the other interrupts alone. So does the following work? (Note, have you tried to reproduce the bug and test the fix on mainline? The amba-pl011 driver in v4.4 looks a little different here.) Cheers ---Dave [...] diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index e7b5fc6..dd6c285 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1672,9 +1672,8 @@ static int pl011_hwinit(struct uart_port *port) uap->port.uartclk = clk_get_rate(uap->clk); - /* Clear pending error and receive interrupts */ - pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS | - UART011_FEIS | UART011_RTIS | UART011_RXIS, + /* Clear pending error interrupts */ + pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS, uap, REG_ICR); /*