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

@@ -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 &reg)
 {
     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 &reg, 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<Iir> &reg)
     } 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<Ier> &reg, 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<Ier> &reg, 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

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;