ns8250: Check if flush via FCR succeeded

[cperciva]

Summary

The emulated UART in the Firecracker VMM (aka the implementation in the
rust-vmm/vm-superio project) includes FIFOs but does not implement the
FCR register, which is used by ns8250_flush to flush the FIFOs.

Check the LSR to see if there is still data in the FIFOs and call
ns8250_drain if necessary.

[emaste]

IMO it's worth putting a link to https://github.com/rust-vmm/vm-superio/issues/83 in here, with the intent that this workaround could eventually be removed again.

[emaste]

sys/dev/uart/uart_dev_ns8250.c

232

if (drain != 0) may be more style(9)ish

[imp]

sys/dev/uart/uart_dev_ns8250.c

214

drain could be a bool since we don't use the bits set for anything.

[emaste]

sys/dev/uart/uart_dev_ns8250.c

214

I was about to make that comment before noticing we pass it to ns8250_drain() which does use the bits.

[cperciva]

Add comment and change if (drain) to if (drain != 0).

[emaste]

Presumably it is possible for a character to arrive between successfully flushing the RX fifo and reading LSR_RXRDY, resulting in a bogus report that UART FCR is broken? IMO that's fine, but an argument for keeping this as a temporary workaround until rust-vmm is fixed.

[cperciva]

In D36979#839866, @emaste wrote:

Presumably it is possible for a character to arrive between successfully flushing the RX fifo and reading LSR_RXRDY, resulting in a bogus report that UART FCR is broken? IMO that's fine, but an argument for keeping this as a temporary workaround until rust-vmm is fixed.

Yes, a race condition is possible. Since we're flushing the FIFO anyway I don't think losing a character matters (it would have been lost in the flushing if it arrived a few microseconds earlier) but you're right that theoretically we could get a bogus warning.

I've made a note to remove this once rust-vmm has been fixed (after a reasonable window of time for deployment).

[imp]

If we see these warnings on !FIRECRACKER systems, we could limit the workaround to FIRECRACKER kernels too.

[jrtc27]

sys/dev/uart/uart_dev_ns8250.c

240

Printing whilst trying to drain seems like an odd thing to do that might cause issues?..

[cperciva]

In D36979#839868, @imp wrote:

If we see these warnings on !FIRECRACKER systems, we could limit the workaround to FIRECRACKER kernels too.

Yeah, I considered hiding it behind a machdep.broken_uart_fcr loader setting or similar, but figured that the cost (a single LSR read) was low enough and the rust-vmm bug might surface in other environments. Certainly if the warning shows up outside of Firecracker it's worth investigating.

[cperciva]

sys/dev/uart/uart_dev_ns8250.c

240

I wondered about that, but the printf shows up on Firecracker so I concluded that it seemed to be ok.