From 5dd464eb1862d01fadc9c111aa98ff360d71605a Mon Sep 17 00:00:00 2001 From: Scott Duensing Date: Sat, 28 Feb 2026 20:06:04 -0600 Subject: [PATCH] Fix hang on second COM port open: CLI deadlock, MOVEABLE buffers, stale UART setque (called by USER.EXE after every inicom) was running GlobalAlloc and GlobalFree with interrupts disabled. On the second open the global heap could need compaction or DPMI selector work, deadlocking under CLI. Restructured to allocate new buffers first, swap pointers under CLI, then free old buffers after STI. Ring buffers changed from GMEM_MOVEABLE to GMEM_FIXED so addresses are inherently stable at interrupt time. Added rxBufH/txBufH handle fields to PortStateT, eliminating the fragile GlobalHandle(SELECTOROF()) recovery in freeBuffers. trmcom now resets FIFOs and drains all pending UART conditions (LSR, MSR, RBR, IIR) before unhooking the ISR, leaving hardware clean for reopen. Co-Authored-By: Claude Opus 4.6 --- drv/commdrv.c | 144 ++++++++++++++++++++++++++++++++++---------------- drv/commdrv.h | 4 +- 2 files changed, 101 insertions(+), 47 deletions(-) diff --git a/drv/commdrv.c b/drv/commdrv.c index 0608407..bdf66d8 100644 --- a/drv/commdrv.c +++ b/drv/commdrv.c @@ -185,7 +185,7 @@ void applyLineParams(PortStateT *port, uint8_t byteSize, uint8_t pari static uint32_t cbrToBaud(uint16_t cbr); int16_t detect16550(uint16_t baseAddr); void enableFifo(PortStateT *port); -static int16_t freeBuffers(PortStateT *port); +static void freeBuffers(PortStateT *port); static int16_t initBuffers(PortStateT *port, uint16_t rxSz, uint16_t txSz); static void initPortState(PortStateT *port, int16_t commId); void primeTx(PortStateT *port); @@ -763,29 +763,21 @@ int16_t FAR PASCAL _export enableNotification(int16_t commId, HWND hwnd, int16_t // ----------------------------------------------------------------------- // freeBuffers - Free ring buffers for a port // ----------------------------------------------------------------------- -static int16_t freeBuffers(PortStateT *port) +static void freeBuffers(PortStateT *port) { - HGLOBAL hMem; - - if (port->rxBuf) { - hMem = (HGLOBAL)LOWORD(GlobalHandle(SELECTOROF(port->rxBuf))); - if (hMem) { - GlobalUnlock(hMem); - GlobalFree(hMem); - } - port->rxBuf = NULL; + if (port->rxBufH) { + GlobalUnlock(port->rxBufH); + GlobalFree(port->rxBufH); + port->rxBufH = 0; + port->rxBuf = NULL; } - if (port->txBuf) { - hMem = (HGLOBAL)LOWORD(GlobalHandle(SELECTOROF(port->txBuf))); - if (hMem) { - GlobalUnlock(hMem); - GlobalFree(hMem); - } - port->txBuf = NULL; + if (port->txBufH) { + GlobalUnlock(port->txBufH); + GlobalFree(port->txBufH); + port->txBufH = 0; + port->txBuf = NULL; } - - return 0; } @@ -977,33 +969,39 @@ int16_t FAR PASCAL _export inicom(DCB FAR *dcb) // ----------------------------------------------------------------------- // initBuffers - Allocate ring buffers via GlobalAlloc +// +// Uses GMEM_FIXED so buffer addresses are stable at interrupt time +// without requiring GlobalLock. Saves handles in PortStateT for +// clean freeing (no GlobalHandle recovery hack). // ----------------------------------------------------------------------- static int16_t initBuffers(PortStateT *port, uint16_t rxSz, uint16_t txSz) { HGLOBAL hRx; HGLOBAL hTx; - hRx = GlobalAlloc(GMEM_MOVEABLE | GMEM_ZEROINIT, (DWORD)rxSz); + hRx = GlobalAlloc(GMEM_FIXED | GMEM_ZEROINIT, (DWORD)rxSz); if (!hRx) { return -1; } - hTx = GlobalAlloc(GMEM_MOVEABLE | GMEM_ZEROINIT, (DWORD)txSz); + hTx = GlobalAlloc(GMEM_FIXED | GMEM_ZEROINIT, (DWORD)txSz); if (!hTx) { GlobalFree(hRx); return -1; } - port->rxBuf = (uint8_t FAR *)GlobalLock(hRx); - port->rxSize = rxSz; - port->rxHead = 0; - port->rxTail = 0; + port->rxBufH = hRx; + port->rxBuf = (uint8_t FAR *)GlobalLock(hRx); + port->rxSize = rxSz; + port->rxHead = 0; + port->rxTail = 0; port->rxCount = 0; - port->txBuf = (uint8_t FAR *)GlobalLock(hTx); - port->txSize = txSz; - port->txHead = 0; - port->txTail = 0; + port->txBufH = hTx; + port->txBuf = (uint8_t FAR *)GlobalLock(hTx); + port->txSize = txSz; + port->txHead = 0; + port->txTail = 0; port->txCount = 0; return 0; @@ -1411,23 +1409,68 @@ int16_t FAR PASCAL _export setque(int16_t commId, int16_t rxSz, int16_t txSz) return -1; } - _disable(); - freeBuffers(port); - if (initBuffers(port, (uint16_t)rxSz, (uint16_t)txSz) != 0) { + // Allocate new buffers BEFORE disabling interrupts. + // GlobalAlloc/GlobalFree can deadlock under CLI if the heap + // needs compaction or DPMI services. + { + HGLOBAL newRxH; + HGLOBAL newTxH; + uint8_t FAR *newRxBuf; + uint8_t FAR *newTxBuf; + HGLOBAL oldRxH; + HGLOBAL oldTxH; + + newRxH = GlobalAlloc(GMEM_FIXED | GMEM_ZEROINIT, (DWORD)(uint16_t)rxSz); + if (!newRxH) { + dbgStr("KPCOMM: setque IE_MEMORY\r\n"); + return IE_MEMORY; + } + newTxH = GlobalAlloc(GMEM_FIXED | GMEM_ZEROINIT, (DWORD)(uint16_t)txSz); + if (!newTxH) { + GlobalFree(newRxH); + dbgStr("KPCOMM: setque IE_MEMORY\r\n"); + return IE_MEMORY; + } + newRxBuf = (uint8_t FAR *)GlobalLock(newRxH); + newTxBuf = (uint8_t FAR *)GlobalLock(newTxH); + + // Swap buffers with interrupts disabled -- ISR sees + // consistent pointers at all times + _disable(); + oldRxH = port->rxBufH; + oldTxH = port->txBufH; + port->rxBufH = newRxH; + port->rxBuf = newRxBuf; + port->rxSize = (uint16_t)rxSz; + port->rxHead = 0; + port->rxTail = 0; + port->rxCount = 0; + port->txBufH = newTxH; + port->txBuf = newTxBuf; + port->txSize = (uint16_t)txSz; + port->txHead = 0; + port->txTail = 0; + port->txCount = 0; + port->comDeb.qInSize = (uint16_t)rxSz; + port->comDeb.qInCount = 0; + port->comDeb.qInHead = 0; + port->comDeb.qInTail = 0; + port->comDeb.qOutSize = (uint16_t)txSz; + port->comDeb.qOutCount = 0; + port->comDeb.qOutHead = 0; + port->comDeb.qOutTail = 0; _enable(); - dbgStr("KPCOMM: setque IE_MEMORY\r\n"); - return IE_MEMORY; + + // Free old buffers AFTER re-enabling interrupts + if (oldRxH) { + GlobalUnlock(oldRxH); + GlobalFree(oldRxH); + } + if (oldTxH) { + GlobalUnlock(oldTxH); + GlobalFree(oldTxH); + } } - // Sync ComDEB queue sizes - port->comDeb.qInSize = port->rxSize; - port->comDeb.qInCount = 0; - port->comDeb.qInHead = 0; - port->comDeb.qInTail = 0; - port->comDeb.qOutSize = port->txSize; - port->comDeb.qOutCount = 0; - port->comDeb.qOutHead = 0; - port->comDeb.qOutTail = 0; - _enable(); dbgStr("KPCOMM: setque OK\r\n"); return 0; @@ -1590,14 +1633,23 @@ int16_t FAR PASCAL _export trmcom(int16_t commId) // Disable UART interrupts _outp(port->baseAddr + UART_IER, 0); - // Disable FIFOs + // Reset FIFOs to drain pending data, then disable if (port->is16550) { + _outp(port->baseAddr + UART_FCR, FCR_ENABLE | FCR_RX_RESET | FCR_TX_RESET); _outp(port->baseAddr + UART_FCR, 0); } // Drop DTR, RTS, OUT2 _outp(port->baseAddr + UART_MCR, 0); + // Drain all pending UART conditions so the hardware is clean + // for the next open. Each read clears the corresponding + // interrupt source inside the UART. + _inp(port->baseAddr + UART_LSR); + _inp(port->baseAddr + UART_MSR); + _inp(port->baseAddr + UART_RBR); + _inp(port->baseAddr + UART_IIR); + // Unhook ISR unhookIsr(port); diff --git a/drv/commdrv.h b/drv/commdrv.h index 34b60bd..7cd3975 100644 --- a/drv/commdrv.h +++ b/drv/commdrv.h @@ -336,13 +336,15 @@ typedef struct { uint8_t fifoEnabled; // FIFO enabled (COMnFIFO setting) uint8_t fifoTrigger; // RX FIFO trigger FCR bits (FCR_TRIG_*) - // Ring buffers (GlobalAlloc'd) + // Ring buffers (GlobalAlloc'd GMEM_FIXED) + HGLOBAL rxBufH; // Receive buffer handle uint8_t FAR *rxBuf; // Receive ring buffer uint16_t rxSize; // Buffer size uint16_t rxHead; // Write position (ISR writes) uint16_t rxTail; // Read position (app reads) uint16_t rxCount; // Bytes in buffer + HGLOBAL txBufH; // Transmit buffer handle uint8_t FAR *txBuf; // Transmit ring buffer uint16_t txSize; // Buffer size uint16_t txHead; // Write position (app writes)