[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

This is an update to a previous RFC v3 [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].

RFC v3 did not solve the problem by itself, due to the problem being
triggered again in pl011_enable_interrupts() after working around it
in pl011_hwinit().  See the updated commit message in the patch for
details.

This patch is intended to supersede the previous RFCs, so please test
_without_ RFC v2 (or 3) applied.

If you can, please:

 a) Check that you can still reproduce the bug on mainline without this
    patch.

 b) Check whether this patch fixes the problem.

Cheers
---Dave

[1] [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html

[2] [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

Dave Martin (1):
  tty: pl011: Avoid spuriously stuck-off interrupts

 drivers/tty/serial/amba-pl011.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.1.4

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

This is an update to a previous RFC v3 [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].

RFC v3 did not solve the problem by itself, due to the problem being
triggered again in pl011_enable_interrupts() after working around it
in pl011_hwinit().  See the updated commit message in the patch for
details.

This patch is intended to supersede the previous RFCs, so please test
_without_ RFC v2 (or 3) applied.

If you can, please:

 a) Check that you can still reproduce the bug on mainline without this
    patch.

 b) Check whether this patch fixes the problem.

Cheers
---Dave

[1] [RFC PATCH v3] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/573120.html

[2] [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

Dave Martin (1):
  tty: pl011: Avoid spuriously stuck-off interrupts

 drivers/tty/serial/amba-pl011.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.1.4

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

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 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 instead clears the interrupt condition by draining the
RX FIFO during UART startup, after clearing any potentially
spurious interrupt.  This should ensure that an interrupt will
definitely be asserted if the RX FIFO subsequently becomes
sufficiently full.

The drain is done at the point of enabling interrupts only.  This
means that it will occur any time the UART is newly opened through
the tty layer.  It will not apply to polled-mode use of the UART by
kgdboc: since that scenario cannot use interrupts by design, this
should not matter.  kgdboc will interact badly with "normal" use of
the UART in any case: this patch makes no attempt to paper over
such issues.

This patch does not attempt to address the case where the RX FIFO
fills faster than it can be drained: that is a pathological
hardware design problem that is beyond the scope of the driver to
work around.

[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 <xuwei5@hisilicon.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since RFC v3:

 * Move the RX FIFO drain to pl011_enable_interrupts().  The
   rationale for this is explained in the updated commit message
   above.
---
 drivers/tty/serial/amba-pl011.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4b40a5b..a6ccb85 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1731,6 +1731,16 @@ 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);
+
+	/*
+	 * 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 RX FIFO explicitly to fix this:
+	 */
+	while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
+		pl011_read(uap, REG_DR);
+
 	uap->im = UART011_RTIM;
 	if (!pl011_dma_rx_running(uap))
 		uap->im |= UART011_RXIM;
-- 
2.1.4

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

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 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 instead clears the interrupt condition by draining the
RX FIFO during UART startup, after clearing any potentially
spurious interrupt.  This should ensure that an interrupt will
definitely be asserted if the RX FIFO subsequently becomes
sufficiently full.

The drain is done at the point of enabling interrupts only.  This
means that it will occur any time the UART is newly opened through
the tty layer.  It will not apply to polled-mode use of the UART by
kgdboc: since that scenario cannot use interrupts by design, this
should not matter.  kgdboc will interact badly with "normal" use of
the UART in any case: this patch makes no attempt to paper over
such issues.

This patch does not attempt to address the case where the RX FIFO
fills faster than it can be drained: that is a pathological
hardware design problem that is beyond the scope of the driver to
work around.

[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 <xuwei5@hisilicon.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since RFC v3:

 * Move the RX FIFO drain to pl011_enable_interrupts().  The
   rationale for this is explained in the updated commit message
   above.
---
 drivers/tty/serial/amba-pl011.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4b40a5b..a6ccb85 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1731,6 +1731,16 @@ 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);
+
+	/*
+	 * 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 RX FIFO explicitly to fix this:
+	 */
+	while (!(pl011_read(uap, REG_FR) & UART01x_FR_RXFE))
+		pl011_read(uap, REG_DR);
+
 	uap->im = UART011_RTIM;
 	if (!pl011_dma_rx_running(uap))
 		uap->im |= UART011_RXIM;
-- 
2.1.4

Re: [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Peter Maydell]

On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> This is an update to a previous RFC v3 [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].
>
> RFC v3 did not solve the problem by itself, due to the problem being
> triggered again in pl011_enable_interrupts() after working around it
> in pl011_hwinit().  See the updated commit message in the patch for
> details.
>
> This patch is intended to supersede the previous RFCs, so please test
> _without_ RFC v2 (or 3) applied.
>
> If you can, please:
>
>  a) Check that you can still reproduce the bug on mainline without this
>     patch.
>
>  b) Check whether this patch fixes the problem.

Adding back to the CC list the people who might be able to do
this testing...
Link to v4 patch:
https://patchwork.kernel.org/patch/10365551/

thanks
-- PMM

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Peter Maydell]

On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> This is an update to a previous RFC v3 [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].
>
> RFC v3 did not solve the problem by itself, due to the problem being
> triggered again in pl011_enable_interrupts() after working around it
> in pl011_hwinit().  See the updated commit message in the patch for
> details.
>
> This patch is intended to supersede the previous RFCs, so please test
> _without_ RFC v2 (or 3) applied.
>
> If you can, please:
>
>  a) Check that you can still reproduce the bug on mainline without this
>     patch.
>
>  b) Check whether this patch fixes the problem.

Adding back to the CC list the people who might be able to do
this testing...
Link to v4 patch:
https://patchwork.kernel.org/patch/10365551/

thanks
-- PMM

Re: [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

On Thu, Apr 26, 2018 at 01:11:41PM +0100, Peter Maydell wrote:
> On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> > This is an update to a previous RFC v3 [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].
> >
> > RFC v3 did not solve the problem by itself, due to the problem being
> > triggered again in pl011_enable_interrupts() after working around it
> > in pl011_hwinit().  See the updated commit message in the patch for
> > details.
> >
> > This patch is intended to supersede the previous RFCs, so please test
> > _without_ RFC v2 (or 3) applied.
> >
> > If you can, please:
> >
> >  a) Check that you can still reproduce the bug on mainline without this
> >     patch.
> >
> >  b) Check whether this patch fixes the problem.
> 
> Adding back to the CC list the people who might be able to do
> this testing...
> Link to v4 patch:
> https://patchwork.kernel.org/patch/10365551/

Dang.  Thanks!

(Sorry guys)

---Dave

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

On Thu, Apr 26, 2018 at 01:11:41PM +0100, Peter Maydell wrote:
> On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> > This is an update to a previous RFC v3 [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].
> >
> > RFC v3 did not solve the problem by itself, due to the problem being
> > triggered again in pl011_enable_interrupts() after working around it
> > in pl011_hwinit().  See the updated commit message in the patch for
> > details.
> >
> > This patch is intended to supersede the previous RFCs, so please test
> > _without_ RFC v2 (or 3) applied.
> >
> > If you can, please:
> >
> >  a) Check that you can still reproduce the bug on mainline without this
> >     patch.
> >
> >  b) Check whether this patch fixes the problem.
> 
> Adding back to the CC list the people who might be able to do
> this testing...
> Link to v4 patch:
> https://patchwork.kernel.org/patch/10365551/

Dang.  Thanks!

(Sorry guys)

---Dave

Re: [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Andrew Jones]

On Thu, Apr 26, 2018 at 01:11:41PM +0100, Peter Maydell wrote:
> On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> > This is an update to a previous RFC v3 [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].
> >
> > RFC v3 did not solve the problem by itself, due to the problem being
> > triggered again in pl011_enable_interrupts() after working around it
> > in pl011_hwinit().  See the updated commit message in the patch for
> > details.
> >
> > This patch is intended to supersede the previous RFCs, so please test
> > _without_ RFC v2 (or 3) applied.
> >
> > If you can, please:
> >
> >  a) Check that you can still reproduce the bug on mainline without this
> >     patch.
> >
> >  b) Check whether this patch fixes the problem.
> 
> Adding back to the CC list the people who might be able to do
> this testing...
> Link to v4 patch:
> https://patchwork.kernel.org/patch/10365551/
>

Tested-by: Andrew Jones <drjones@redhat.com>

And
 
Reviewed-by: Andrew Jones <drjones@redhat.com>

as this patch is consistent with what I expected was needed back
when I reported the issue[*]. I was just too dense to realize
that we didn't need to "reset" the FIFO, but rather just drain it.

Thanks,
drew

[*] https://lkml.org/lkml/2016/8/1/247

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Andrew Jones]

On Thu, Apr 26, 2018 at 01:11:41PM +0100, Peter Maydell wrote:
> On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> > This is an update to a previous RFC v3 [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].
> >
> > RFC v3 did not solve the problem by itself, due to the problem being
> > triggered again in pl011_enable_interrupts() after working around it
> > in pl011_hwinit().  See the updated commit message in the patch for
> > details.
> >
> > This patch is intended to supersede the previous RFCs, so please test
> > _without_ RFC v2 (or 3) applied.
> >
> > If you can, please:
> >
> >  a) Check that you can still reproduce the bug on mainline without this
> >     patch.
> >
> >  b) Check whether this patch fixes the problem.
> 
> Adding back to the CC list the people who might be able to do
> this testing...
> Link to v4 patch:
> https://patchwork.kernel.org/patch/10365551/
>

Tested-by: Andrew Jones <drjones@redhat.com>

And
 
Reviewed-by: Andrew Jones <drjones@redhat.com>

as this patch is consistent with what I expected was needed back
when I reported the issue[*]. I was just too dense to realize
that we didn't need to "reset" the FIFO, but rather just drain it.

Thanks,
drew

[*] https://lkml.org/lkml/2016/8/1/247

Re: [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

On Thu, Apr 26, 2018 at 03:42:01PM +0200, Andrew Jones wrote:
> On Thu, Apr 26, 2018 at 01:11:41PM +0100, Peter Maydell wrote:
> > On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> > > This is an update to a previous RFC v3 [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].
> > >
> > > RFC v3 did not solve the problem by itself, due to the problem being
> > > triggered again in pl011_enable_interrupts() after working around it
> > > in pl011_hwinit().  See the updated commit message in the patch for
> > > details.
> > >
> > > This patch is intended to supersede the previous RFCs, so please test
> > > _without_ RFC v2 (or 3) applied.
> > >
> > > If you can, please:
> > >
> > >  a) Check that you can still reproduce the bug on mainline without this
> > >     patch.
> > >
> > >  b) Check whether this patch fixes the problem.
> > 
> > Adding back to the CC list the people who might be able to do
> > this testing...
> > Link to v4 patch:
> > https://patchwork.kernel.org/patch/10365551/
> >
> 
> Tested-by: Andrew Jones <drjones@redhat.com>
> 
> And
>  
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> as this patch is consistent with what I expected was needed back
> when I reported the issue[*]. I was just too dense to realize
> that we didn't need to "reset" the FIFO, but rather just drain it.

Thanks for the testing and review!

There seem to be a few ways to solve this, and it's hard to know which 
ne is best.  This at least looks simple now.

Possibly there is a way to reset the FIFO on a real PL011, but I ignored
the possibility since there is definitely no way to do that on e.g.,
SBSA UART.  Draining the FIFO by hand ought to work on all variants IIUC.

Cheers
---Dave

> 
> Thanks,
> drew
> 
> [*] https://lkml.org/lkml/2016/8/1/247
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts

[Dave Martin]

On Thu, Apr 26, 2018 at 03:42:01PM +0200, Andrew Jones wrote:
> On Thu, Apr 26, 2018 at 01:11:41PM +0100, Peter Maydell wrote:
> > On 26 April 2018 at 11:54, Dave Martin <Dave.Martin@arm.com> wrote:
> > > This is an update to a previous RFC v3 [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].
> > >
> > > RFC v3 did not solve the problem by itself, due to the problem being
> > > triggered again in pl011_enable_interrupts() after working around it
> > > in pl011_hwinit().  See the updated commit message in the patch for
> > > details.
> > >
> > > This patch is intended to supersede the previous RFCs, so please test
> > > _without_ RFC v2 (or 3) applied.
> > >
> > > If you can, please:
> > >
> > >  a) Check that you can still reproduce the bug on mainline without this
> > >     patch.
> > >
> > >  b) Check whether this patch fixes the problem.
> > 
> > Adding back to the CC list the people who might be able to do
> > this testing...
> > Link to v4 patch:
> > https://patchwork.kernel.org/patch/10365551/
> >
> 
> Tested-by: Andrew Jones <drjones@redhat.com>
> 
> And
>  
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> as this patch is consistent with what I expected was needed back
> when I reported the issue[*]. I was just too dense to realize
> that we didn't need to "reset" the FIFO, but rather just drain it.

Thanks for the testing and review!

There seem to be a few ways to solve this, and it's hard to know which 
ne is best.  This at least looks simple now.

Possibly there is a way to reset the FIFO on a real PL011, but I ignored
the possibility since there is definitely no way to do that on e.g.,
SBSA UART.  Draining the FIFO by hand ought to work on all variants IIUC.

Cheers
---Dave

> 
> Thanks,
> drew
> 
> [*] https://lkml.org/lkml/2016/8/1/247
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel