Page MenuHomeFreeBSD

ns8250: don't drop IER_TXRDY on bus_grab/ungrab

Authored by mhorne on Mar 8 2021, 5:59 PM.



It has been observed that some systems are often unable to resume from
ddb after entering with debug.kdb.enter=1. Checking the status further
shows the terminal is blocked waiting in tty_drain(), but it never makes
progress in clearing the output queue, because sc->sc_txbusy is high.

I noticed that when entering polling mode for the debugger, IER_TXRDY is
set in the failure case. Since this bit is never tracked by the softc,
it will not be restored by ns8250_bus_ungrab(). This creates a race in
which a TX interrupt can be lost, creating the hang described above.
Ensuring that this bit is restored is enough to prevent this, and resume
from ddb as expected.

After some study, the best solution seems to be to track this bit in the
sc->ier field, for the same lifetime that TX interrupts are enabled.

PR: 223917, 240122
Partial diagnosis: markj, in 240122

Test Plan

Without this patch, continuing from ddb will almost always hang on my rockpro64.

With, it appears to never happen.

Diff Detail

rG FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mhorne requested review of this revision.Mar 8 2021, 5:59 PM
mhorne created this revision.

With the bit or'd in on the prior line, is this still needed? What does broken_txfifo control?


broken_txfifo seems to be for devices where the TX-ready interrupt won't fire. When set, it drains the tx fifo and schedules the SER_INT_TXIDLE softih immediately.

So it seems that IER_ETXRDY should not be needed at all for this case, but I think I left it above because I was unsure of this.

Looks like this tunable was needed for old versions of QEMU and possibly the Allwinner A10 uart (1c60b24baa50 and ac4adddf040f).

Remove superfluous IER_ETXRDY.

This revision is now accepted and ready to land.Mar 9 2021, 9:02 PM
manu added a subscriber: manu.

Awesome, thanks for looking into this !

bz added a subscriber: bz.

Accepting on the basis that it fixes PR 240122 (just tested)