Page MenuHomeFreeBSD

ns8250: don't drop IER_TXRDY on bus_grab/ungrab
ClosedPublic

Authored by mhorne on Mar 8 2021, 5:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 9:11 AM
Unknown Object (File)
Oct 2 2024, 1:33 PM
Unknown Object (File)
Sep 30 2024, 9:44 PM
Unknown Object (File)
Sep 26 2024, 7:21 AM
Unknown Object (File)
Sep 16 2024, 9:41 PM
Unknown Object (File)
Sep 16 2024, 5:39 AM
Unknown Object (File)
Sep 11 2024, 2:18 AM
Unknown Object (File)
Sep 4 2024, 10:23 PM
Subscribers

Details

Summary

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

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mhorne requested review of this revision.Mar 8 2021, 5:59 PM
mhorne created this revision.
sys/dev/uart/uart_dev_ns8250.c
1039โ€“1041

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

sys/dev/uart/uart_dev_ns8250.c
1039โ€“1041

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)

This revision was automatically updated to reflect the committed changes.