tty: pl011: Avoid spuriously stuck-off interrupts
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
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.
Cheers
---Dave
[1] [RFC PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/556897.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 | 9 +++++++++ 1 file changed, 9 insertions(+)
Signature · 1 line
2.1.4
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
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.
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
---
Changes since RFC v2:
* Drain the RX FIFO explicitly on startup.
This reduces the complexity of the changes, but should still fix the underlying problem.
Characters present in the RX FIFO before the kernel boots will now be lost. However it was always poor practice to rely on such characters being processed by the kernel, since UART initialisation and RX FIFO overruns can always lead to data loss or corruption in any case.
--- drivers/tty/serial/amba-pl011.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/tty/serial/amba-pl011.c +9 -0
@@ -1676,6 +1676,15 @@ static int pl011_hwinit(struct uart_port *port) 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 ); + +/* * Save interrupts enable mask, and enable RX interrupts in case if * the interrupt is used for NMI entry. */
Signature · 1 line
2.1.4
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
In reply to Dave MartinQuoted 11 lines · Dave Martin
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.
Quoted 7 lines
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
-- PMM
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 18 lines · Peter Maydell
Quoted 17 lines · Dave Martin
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.
Quoted 7 lines
Quoted 1 line
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.
If you choose to go with v2, then you can add my tested-by.
Thanks,
drew
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 27 lines · Andrew Jones
Quoted 18 lines · Peter Maydell
Quoted 17 lines · Dave Martin
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.
Quoted 7 lines
Quoted 1 line
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
Quoted 5 lines
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
Quoted 4 lines
If you choose to go with v2, then you can add my tested-by.
Thanks,
drew
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 30 lines · Ciro Santilli
Quoted 29 lines · Andrew Jones
Quoted 18 lines · Peter Maydell
Quoted 17 lines · Dave Martin
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.
Quoted 7 lines
Quoted 1 line
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.htmlHi Dave,
Quoted 8 lines
Quoted 3 lines
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
drivers/tty/serial/amba-pl011.c +16 -9
@@ -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 ;
RFC PATCH v3 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 100 lines · Dave Martin
Quoted 30 lines · Ciro Santilli
Quoted 29 lines · Andrew Jones
Quoted 18 lines · Peter Maydell
Quoted 17 lines · Dave Martin
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.
Quoted 7 lines
Quoted 1 line
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.htmlHi Dave,
Quoted 8 lines
Quoted 3 lines
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/14965a40d27c8d9d1ff5b023ace827b288a024efHmmm, interesting.
Looking at the code, it looks like RXIS is explicitly cleared twice, once in
pl011_hwinit()and once inpl011_startup(). TheCONFIG_POLLcode 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
---Davedrivers/tty/serial/amba-pl011.c +14
-9@@ -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
drivers/tty/serial/amba-pl011.c static void pl011_enable_interrupts(struct uart_amba_port *uap) · line 1752
Quoted 8 lines
drivers/tty/serial/amba-pl011.c +2
-0@@ -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
drivers/tty/serial/amba-pl011.c +17 -0
@@ -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 ;
Thanks,
drew