From patchwork Wed Apr 25 14:59:28 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Jones X-Patchwork-Id: 10363415 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 ED31D6032C for ; Wed, 25 Apr 2018 14:59:59 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D89CC28B89 for ; Wed, 25 Apr 2018 14:59:59 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CCCC828BE0; Wed, 25 Apr 2018 14:59:59 +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=-2.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,MAILING_LIST_MULTI autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.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 0707528B8A for ; Wed, 25 Apr 2018 14:59:59 +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=NptjXaC5oBqckuLUmdvXJrBIuo/DRhh98D1DYogqEls=; b=uCaGMUqYd97Xkl cP2CS+clmW7APrJfcF9R1jEm0fuEjBMqywlINVDSQg5tWr5dqztCM7I1UPDcJhUEQ/bFlO8XJ70nw 6n7BIkj4q+tLbLwAhuLULIB2FESTDcQuJ2wG4rFQAb+X/P0Oyemh/0ha6Y72yOwUniWv5nbm7EJw2 /9b/uQUcISqeVkedU6H6gvJploXuE8y9NTeOjCO0l+78jhe5EiB30oD/dQ2z8LqXwebdcfMtKjXCJ FgCKMneoJbEnWvbNqRhAEqITY1R7rK/GsfetAUZnSyOSJcYUsOBItu71OlSeDnPA7JD/vyn1kBwLs fh1O8KRg8iUBHusjkdUA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1fBLte-0002vE-HT; Wed, 25 Apr 2018 14:59:50 +0000 Received: from mx3-rdu2.redhat.com ([66.187.233.73] helo=mx1.redhat.com) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1fBLtY-0002uI-Cm for linux-arm-kernel@lists.infradead.org; Wed, 25 Apr 2018 14:59:48 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 75E384270948; Wed, 25 Apr 2018 14:59:32 +0000 (UTC) Received: from kamzik.brq.redhat.com (unknown [10.43.2.160]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 36B0710F1BF1; Wed, 25 Apr 2018 14:59:31 +0000 (UTC) Date: Wed, 25 Apr 2018 16:59:28 +0200 From: Andrew Jones To: Dave Martin Subject: Re: [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts Message-ID: <20180425145928.jnrlj3232xmxqix6@kamzik.brq.redhat.com> References: <1524490888-849-1-git-send-email-Dave.Martin@arm.com> <20180425122000.cts26gtvjuje4j7f@kamzik.brq.redhat.com> <20180425141010.GO16308@e103592.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180425141010.GO16308@e103592.cambridge.arm.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 25 Apr 2018 14:59:32 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Wed, 25 Apr 2018 14:59:32 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'drjones@redhat.com' RCPT:'' X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180425_075944_619342_51A5FE12 X-CRM114-Status: GOOD ( 38.22 ) 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 , Ciro Santilli , Linus Walleij , Russell King , Wei Xu , linux-serial@vger.kernel.org, arm-mail-list 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 Wed, Apr 25, 2018 at 03:10:14PM +0100, Dave Martin wrote: > On Wed, Apr 25, 2018 at 01:47:42PM +0100, Ciro Santilli wrote: > > On Wed, Apr 25, 2018 at 1:20 PM, Andrew Jones wrote: > > > On Mon, Apr 23, 2018 at 02:49:30PM +0100, Peter Maydell wrote: > > >> On 23 April 2018 at 14:41, Dave Martin wrote: > > >> > This is an update to a previous RFC v2 [1], to fix a problem observed by > > >> > the qemu community that causes serial input to hang when booting a > > >> > simulated system with data already queued in the UART FIFO [2]. > > >> > > > >> > After discussion, I decided that the approach in [1] was over- > > >> > engineered: it tries to preserve a guarantee that people shouldn't be > > >> > relying on anyway, namely that data sent to the UART prior to kernel > > >> > boot will be received by the kernel; or more generally that data > > >> > received by the UART while the pl011 driver is not opened will be > > >> > received (either intact or at all) by the driver. > > >> > > > >> > If anyone can please test the following and let me know the results, > > >> > that would be much appreciated! > > >> > > > >> > a) Check that you can still reproduce the bug on mainline without this > > >> > patch. > > >> > > > >> > b) Check whether this patch fixes the problem. > > >> > > >> Thanks. I'm cc'ing Ciro and Drew, who are the two people I > > >> recall reporting this issue to me. > > >> Link to the patch for their benefit: > > >> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html > > >> > > > > > > Hi Dave, > > > > > > v3 does not resolve the issue for me. v2 does, and, fwiw, v3 applied on > > > top of v2 (i.e. applying both), still works. > > > > > > > I also confirm that v2 + v3 on top of Linux kernel v4.16, QEMU v2.11.0 > > solves the problem on arm and aarch64, otherwise if I hit Ctrl + C > > during boot my terminal become irresponsive after boot. Test setup: > > https://github.com/cirosantilli/linux-kernel-module-cheat/tree/14965a40d27c8d9d1ff5b023ace827b288a024ef > > Hmmm, interesting. > > Looking at the code, it looks like RXIS is explicitly cleared twice, > once in pl011_hwinit() and once in pl011_startup(). The CONFIG_POLL > code uses hwinit alone, and I'm not sure exactly what properties it > relies on. > > To be most robust, perhaps we should drain the RX FIFO in both places. > > Can you try applying this on top of the branch and see what happens? > > Cheers > ---Dave > > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 73e6f44..841afbd 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -1652,6 +1652,19 @@ static void pl011_put_poll_char(struct uart_port *port, > > #endif /* CONFIG_CONSOLE_POLL */ > > +/* > + * RXIS is asserted only when the RX FIFO transitions from below to > + * above the trigger threshold. If the RX FIFO is already full to the > + * threshold this can't happen and RXIS will now be stuck off. > + * Unless polling the UART, use this function to drain the RX FIFO > + * explicitly after clearing RXIS. > + */ > +static void pl011_drain_rx_fifo(struct uart_amba_port *uap) > +{ > + while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE)) > + pl011_read(uap, REG_DR); > +} > + > static int pl011_hwinit(struct uart_port *port) > { > struct uart_amba_port *uap = > @@ -1674,15 +1687,7 @@ static int pl011_hwinit(struct uart_port *port) > pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS | > UART011_FEIS | UART011_RTIS | UART011_RXIS, > uap, REG_ICR); > - > - /* > - * RXIS is asserted only when the RX FIFO transitions from below > - * to above the trigger threshold. If the RX FIFO is already > - * full to the threshold this can't happen and RXIS will now be > - * stuck off. Drain the FIFO explicitly to fix this: > - */ > - while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE)) > - pl011_read(uap, REG_DR); > + pl011_drain_rx_fifo(uap); > > /* > * Save interrupts enable mask, and enable RX interrupts in case if > @@ -1740,6 +1745,8 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap) > > /* Clear out any spuriously appearing RX interrupts */ > pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR); > + pl011_drain_rx_fifo(uap); > + > uap->im = UART011_RTIM; > if (!pl011_dma_rx_running(uap)) > uap->im |= UART011_RXIM; > > > This worked for me. To be clear, I applied the following, and nothing else, to my base kernel for the test Thanks, drew diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 111e6a9..d64b64f 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1672,6 +1672,19 @@ static void pl011_put_poll_char(struct uart_port *port, #endif /* CONFIG_CONSOLE_POLL */ +/* + * RXIS is asserted only when the RX FIFO transitions from below to + * above the trigger threshold. If the RX FIFO is already full to the + * threshold this can't happen and RXIS will now be stuck off. + * Unless polling the UART, use this function to drain the RX FIFO + * explicitly after clearing RXIS. + */ +static void pl011_drain_rx_fifo(struct uart_amba_port *uap) +{ + while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE)) + pl011_read(uap, REG_DR); +} + static int pl011_hwinit(struct uart_port *port) { struct uart_amba_port *uap = @@ -1695,6 +1708,8 @@ static int pl011_hwinit(struct uart_port *port) UART011_FEIS | UART011_RTIS | UART011_RXIS, uap, REG_ICR); + pl011_drain_rx_fifo(uap); + /* * Save interrupts enable mask, and enable RX interrupts in case if * the interrupt is used for NMI entry. @@ -1751,6 +1766,8 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap) /* Clear out any spuriously appearing RX interrupts */ pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR); + pl011_drain_rx_fifo(uap); + uap->im = UART011_RTIM; if (!pl011_dma_rx_running(uap)) uap->im |= UART011_RXIM;