From 84cba2a8a8c45147c81b0426ccdead6472346b4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20G=C4=85siorzewski?= Date: Wed, 17 Apr 2024 19:27:39 +0100 Subject: [PATCH] dev: Fix interrupt logic in uart8250 (#1009) Hi, we've noticed some issues with the Uart8250 device when using it as the Linux console. Sometimes the Uart interrupt would remain constantly posted, so Linux would continue to try and handle it, effectively resulting in an infinite loop. With this patch, I'm no longer seeing any issues, but my testing has been limited to configurations and workloads we're interested in at Imagination, so please let me know if there's some other tests I should run or if you notice any other issues. This patch fixes several issues with interrupt posting and clearing in the uart8250 device. The "status" member variable and the console interrupt should be kept in sync. However, in one code path in readIir, the interrupt bit was being cleared in the status variable but not in the platform controller. Additionally, in some code paths, the interrupts would be cleared in the status variable and in the interrupt controller, but a future interrupt would remain scheduled, causing a spurious interrupt and setting a bit in status to 1. These issues can confuse the kernel and result in an ininite interrupt handling loop. Another issue is related to the fact that there are two interrupt causes (TX and RX) and both of them can be valid at the same time. When one of them becomes no longer valid, we should check the status of the other one before clearing the interrupt. This patch addresses the issues listed above and refactors the interrupt clearing logic to reduce repetition. --- src/dev/serial/uart8250.cc | 82 ++++++++++++++++++++++++-------------- src/dev/serial/uart8250.hh | 1 + 2 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/dev/serial/uart8250.cc b/src/dev/serial/uart8250.cc index a2f13be77d..5bcf2cac3e 100644 --- a/src/dev/serial/uart8250.cc +++ b/src/dev/serial/uart8250.cc @@ -57,7 +57,6 @@ Uart8250::processIntrEvent(int intrBit) } else { DPRINTF(Uart, "UART InterEvent, not interrupting\n"); } - } /* The linux serial driver (8250.c about line 1182) loops reading from @@ -78,12 +77,26 @@ Uart8250::scheduleIntr(Event *event) static const Tick interval = 225 * sim_clock::as_int::ns; DPRINTF(Uart, "Scheduling IER interrupt for %s, at cycle %lld\n", event->name(), curTick() + interval); - if (!event->scheduled()) + if (!event->scheduled()) { schedule(event, curTick() + interval); - else + } else { reschedule(event, curTick() + interval); + } } +void +Uart8250::clearIntr(int intrBit) +{ + if ((status & intrBit) == 0) { + return; + } + status &= ~intrBit; + + // Clear interrupt at the controller if neither TX nor RX is pending now + if (!status) { + platform->clearConsoleInt(); + } +} Uart8250::Uart8250(const Params &p) : Uart(p, p.pio_size), registers(this, name() + ".registers"), @@ -132,16 +145,21 @@ uint8_t Uart8250::readRbr(Register8 ®) { uint8_t data = 0; - if (device->dataAvailable()) + if (device->dataAvailable()) { data = device->readData(); - else + } else { DPRINTF(Uart, "empty read of RX register\n"); + } - status &= ~RX_INT; - platform->clearConsoleInt(); + clearIntr(RX_INT); - if (device->dataAvailable() && registers.ier.get().rdi) + if (device->dataAvailable() && registers.ier.get().rdi) { scheduleIntr(&rxIntrEvent); + } else if (rxIntrEvent.scheduled()) { + // Must deschedule a future interrupt if there is no more data to read, + // otherwise this would confuse the kernel (iir.id=rx but lsr.rdr=0) + deschedule(rxIntrEvent); + } return data; } @@ -150,10 +168,12 @@ void Uart8250::writeThr(Register8 ®, const uint8_t &data) { device->writeData(data); - platform->clearConsoleInt(); - status &= ~TX_INT; - if (registers.ier.get().thri) + clearIntr(TX_INT); + if (registers.ier.get().thri) { scheduleIntr(&txIntrEvent); + } else if (txIntrEvent.scheduled()) { + deschedule(txIntrEvent); + } } Uart8250::Iir @@ -168,7 +188,10 @@ Uart8250::readIir(Register ®) } else if (status & TX_INT) { iir.id = (uint8_t)InterruptIds::Tx; // Tx interrupts are cleared on IIR reads. - status &= ~TX_INT; + if (txIntrEvent.scheduled()) { + deschedule(txIntrEvent); + } + clearIntr(TX_INT); } else { iir.pending = 1; } @@ -193,11 +216,10 @@ Uart8250::writeIer(Register ®, const Ier &ier) } } else { DPRINTF(Uart, "IER: IER_THRI cleared, descheduling TX intrrupt\n"); - if (txIntrEvent.scheduled()) + if (txIntrEvent.scheduled()) { deschedule(txIntrEvent); - if (status & TX_INT) - platform->clearConsoleInt(); - status &= ~TX_INT; + } + clearIntr(TX_INT); } if (ier.rdi && device->dataAvailable()) { @@ -205,11 +227,10 @@ Uart8250::writeIer(Register ®, const Ier &ier) scheduleIntr(&rxIntrEvent); } else { DPRINTF(Uart, "IER: IER_RDI cleared, descheduling RX intrrupt\n"); - if (rxIntrEvent.scheduled()) + if (rxIntrEvent.scheduled()) { deschedule(rxIntrEvent); - if (status & RX_INT) - platform->clearConsoleInt(); - status &= ~RX_INT; + } + clearIntr(RX_INT); } } @@ -248,7 +269,6 @@ Uart8250::dataAvailable() platform->postConsoleInt(); status |= RX_INT; } - } AddrRangeList @@ -267,17 +287,19 @@ Uart8250::serialize(CheckpointOut &cp) const paramOut(cp, "LCR", registers.lcr); paramOut(cp, "MCR", registers.mcr); Tick rxintrwhen; - if (rxIntrEvent.scheduled()) + if (rxIntrEvent.scheduled()) { rxintrwhen = rxIntrEvent.when(); - else + } else { rxintrwhen = 0; + } Tick txintrwhen; - if (txIntrEvent.scheduled()) + if (txIntrEvent.scheduled()) { txintrwhen = txIntrEvent.when(); - else + } else { txintrwhen = 0; - SERIALIZE_SCALAR(rxintrwhen); - SERIALIZE_SCALAR(txintrwhen); + } + SERIALIZE_SCALAR(rxintrwhen); + SERIALIZE_SCALAR(txintrwhen); } void @@ -291,10 +313,12 @@ Uart8250::unserialize(CheckpointIn &cp) Tick txintrwhen; UNSERIALIZE_SCALAR(rxintrwhen); UNSERIALIZE_SCALAR(txintrwhen); - if (rxintrwhen != 0) + if (rxintrwhen != 0) { schedule(rxIntrEvent, rxintrwhen); - if (txintrwhen != 0) + } + if (txintrwhen != 0) { schedule(txIntrEvent, txintrwhen); + } } } // namespace gem5 diff --git a/src/dev/serial/uart8250.hh b/src/dev/serial/uart8250.hh index 5774f78aab..670ee646c1 100644 --- a/src/dev/serial/uart8250.hh +++ b/src/dev/serial/uart8250.hh @@ -215,6 +215,7 @@ class Uart8250 : public Uart void processIntrEvent(int intrBit); void scheduleIntr(Event *event); + void clearIntr(int intrBit); EventFunctionWrapper txIntrEvent; EventFunctionWrapper rxIntrEvent;