tty: pl011: Avoid spuriously stuck-off interrupts
PATCH 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
This is an update to a previous RFC [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].
This patch could cause problems for people that are actually relying on chars queued in the PL011 RX FIFO during boot or while the UART is closed. There are no guarantees about such things working in general. In either case, there is no protection against RX FIFO overflow or reprogramming of the UART parameters while Linux is not actively receiving chars.
Cheers
---Dave
[1] [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/574033.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(+)
Signature · 1 line
2.1.4
PATCH 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.
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
--- drivers/tty/serial/amba-pl011.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/tty/serial/amba-pl011.c +10 -0
@@ -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 ;
Signature · 1 line
2.1.4
Russell King - ARM Linux
maintainerPATCH 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 67 lines · 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.htmlReported-by Wei Xu <[email protected]>Cc Russell King <[email protected]>Cc Linus Walleij <[email protected]>Cc Peter Maydell <[email protected]>Fixes 9b96fbacda34 ("serial: PL011: clear pending interrupts")Signed-off-by Dave Martin <[email protected]>Tested-by Andrew Jones <[email protected]>Reviewed-by Andrew Jones <[email protected]>--- drivers/tty/serial/amba-pl011.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/tty/serial/amba-pl011.c +0
-0@@ -1731,6 +1731,16 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)/* Clear out any spuriously appearing RX interrupts */
drivers/tty/serial/amba-pl011.c static void pl011_enable_interrupts(struct uart_amba_port *uap) · line 1742
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +9
-0pl011_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 );
You probably ought to limit this in case of a stuck receive not empty case, maybe to (eg) twice the depth of the FIFO?
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
PATCH 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 67 lines · Russell King - ARM Linux
Quoted 66 lines · 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.htmlReported-by Wei Xu <[email protected]>Cc Russell King <[email protected]>Cc Linus Walleij <[email protected]>Cc Peter Maydell <[email protected]>Fixes 9b96fbacda34 ("serial: PL011: clear pending interrupts")Signed-off-by Dave Martin <[email protected]>Tested-by Andrew Jones <[email protected]>Reviewed-by Andrew Jones <[email protected]>--- drivers/tty/serial/amba-pl011.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/tty/serial/amba-pl011.c +0
-0@@ -1731,6 +1731,16 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
Quoted 14 lines
drivers/tty/serial/amba-pl011.c static void pl011_enable_interrupts(struct uart_amba_port *uap) · line 1742
Quoted 11 lines
drivers/tty/serial/amba-pl011.c +9
-0/* 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 );You probably ought to limit this in case of a stuck receive not empty case, maybe to (eg) twice the depth of the FIFO?
My argument would have been that if the FIFO fills too fast to be drained, then you have more serious problems... but it would be better for this to show as character loss than a lockup.
So, I guess draining twice the FIFO depth before giving up is a reasonable compromise.
I'll hack that in and repost.
Cheers
---Dave
PATCH 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
In reply to Dave MartinQuoted 21 lines · Dave Martin
This is an update to a previous RFC [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].
This patch could cause problems for people that are actually relying on chars queued in the PL011 RX FIFO during boot or while the UART is closed. There are no guarantees about such things working in general. In either case, there is no protection against RX FIFO overflow or reprogramming of the UART parameters while Linux is not actively receiving chars.
Cheers
---Dave[1] [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/574033.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
Quoted 8 lines
Dave Martin (1):
tty: pl011: Avoid spuriously stuck-off interruptsdrivers/tty/serial/amba-pl011.c | 10 ++++++++++ 1 file changed, 10 insertions(+)Signature · 1 line
2.1.4
There is no patch here, did something go wrong?
You do not need a "cover letter" for a single patch...
thanks,
greg k-h
PATCH 1/1 tty: pl011: Avoid spuriously stuck-off interrupts
Quoted 28 lines · Greg KH
Quoted 27 lines · Dave Martin
This is an update to a previous RFC [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].
This patch could cause problems for people that are actually relying on chars queued in the PL011 RX FIFO during boot or while the UART is closed. There are no guarantees about such things working in general. In either case, there is no protection against RX FIFO overflow or reprogramming of the UART parameters while Linux is not actively receiving chars.
Cheers
---Dave[1] [RFC PATCH v4] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/574033.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.htmlDave Martin (1):
tty: pl011: Avoid spuriously stuck-off interruptsdrivers/tty/serial/amba-pl011.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
Quoted 6 lines
Quoted 2 lines
Signature · 1 line
2.1.4There is no patch here, did something go wrong?
You do not need a "cover letter" for a single patch...
I wanted to provide some additional context for the benefit of people who'd been reviewing earler RFCs for this change, though I could have put it under the tearoff instead.
I was expecting --cover-letter to generate [PATCH 0/1] and [PATCH 1/1] Subject lines, but it seems that it doesn't do that.
To clarify, there is only intended to be one patch here.
Do you want me to repost?
Cheers
---Dave