From 4b5b24339c4f1d48d7cb6ed4e3ffb27e3d047f96 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:25:48 +0200 Subject: [PATCH 1/8] uart: create all devices via new_with_backend() The constructors initialize the device fields in several places. Route them through new_with_backend(), so new fields only need to be set in one place. --- src/lib.rs | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2aa0f34..558f6b2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -365,12 +365,8 @@ impl Uart16550 { } let backend = PioBackend { base: base_port }; - - Ok(Self { - backend, - // Will be replaced by the actual config in init() afterwards. - config: Config::DEFAULT, - }) + // SAFETY: The caller ensured that the port is valid. + Ok(unsafe { Self::new_with_backend(backend) }) } } @@ -395,15 +391,13 @@ impl Uart16550 { stride: u8, ) -> Result { Self::check_mmio(base_address.addr().get(), stride)?; - Ok(Self { - backend: MmioBackend { - base: base_address, - stride: NonZeroU8::new(stride) - .expect("check_mmio() should have rejected a zero stride"), - }, - // Will be replaced by the actual config in init() afterwards. - config: Config::DEFAULT, - }) + let backend = MmioBackend { + base: base_address, + stride: NonZeroU8::new(stride) + .expect("check_mmio() should have rejected a zero stride"), + }; + // SAFETY: The caller ensured that the address is valid. + Ok(unsafe { Self::new_with_backend(backend) }) } /// Creates a new [`Uart16550`] backed by MMIO from an integer address. @@ -446,15 +440,13 @@ impl Uart16550 { let ptr = ptr::with_exposed_provenance_mut::(base_address.get()); // SAFETY: The address is non-zero, so the pointer is non-null. let ptr = unsafe { NonNull::new_unchecked(ptr) }; - Ok(Self { - backend: MmioBackend { - base: ptr, - stride: NonZeroU8::new(stride) - .expect("check_mmio() should have rejected a zero stride"), - }, - // Will be replaced by the actual config in init() afterwards. - config: Config::DEFAULT, - }) + let backend = MmioBackend { + base: ptr, + stride: NonZeroU8::new(stride) + .expect("check_mmio() should have rejected a zero stride"), + }; + // SAFETY: The caller ensured that the address is valid. + Ok(unsafe { Self::new_with_backend(backend) }) } /// Checks that the stride is valid and that all registers fit into the From ff9d08ee4b87bedfeeb3ddcbd9e72077c1db23d8 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:26:10 +0200 Subject: [PATCH 2/8] uart: validate the baud rate before touching the device An invalid baud rate made init() return with DLAB set. Offsets 0 and 1 then address the divisor latch instead of the data registers and IER, so the device is unusable until the next successful init(). --- CHANGELOG.md | 1 + src/lib.rs | 15 ++++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 117ac68..80834ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ new `const fn` `Uart16550::new_with_backend()`, for example to perform port I/O via `tdcall` or GHCB in confidential VMs. - New `Uart16550Tty::new_with_backend()`. +- Fixed: `init()` no longer leaves DLAB set if the baud rate is invalid. ## 0.8.1 - 2026-09-14 diff --git a/src/lib.rs b/src/lib.rs index 558f6b2..6925b63 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -538,6 +538,14 @@ impl Uart16550 { /// Further, the serial config must match the expectations of the receiver /// on the other side. Otherwise, garbage will be received. pub fn init(&mut self, config: Config) -> Result<(), InitError> { + // Validate the config before touching the device. + let divisor = calc_divisor( + config.frequency, + config.baud_rate.to_integer(), + config.prescaler_division_factor, + ) + .map_err(InitError::InvalidBaudRate)?; + // It is important to set this early as some helpers rely on that. self.config = config; @@ -563,13 +571,6 @@ impl Uart16550 { // Set Divisor Latch Access Bit (DLAB) to access DLL and DLM next self.backend.write(offsets::LCR as u8, LCR::DLAB.bits()); - let divisor = calc_divisor( - self.config.frequency, - self.config.baud_rate.to_integer(), - self.config.prescaler_division_factor, - ) - .map_err(InitError::InvalidBaudRate)?; - let low = (divisor & 0xff) as u8; let high = ((divisor >> 8) & 0xff) as u8; self.backend.write(offsets::DLL as u8, low); From 4fea510b288b099808ad4f671d0c2fb4b01215ac Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:27:15 +0200 Subject: [PATCH 3/8] spec: avoid overflows in calc_baud_rate() The denominator 16 * (psd + 1) * divisor overflows u32 for large inputs, e.g., when calc_divisor() gets a very high baud rate. Calculate in u64. --- CHANGELOG.md | 1 + src/spec.rs | 14 ++++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80834ff..96a2196 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ I/O via `tdcall` or GHCB in confidential VMs. - New `Uart16550Tty::new_with_backend()`. - Fixed: `init()` no longer leaves DLAB set if the baud rate is invalid. +- Fixed: `calc_baud_rate()` no longer overflows for large inputs. ## 0.8.1 - 2026-09-14 diff --git a/src/spec.rs b/src/spec.rs index 95dc4be..5725759 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -101,11 +101,13 @@ pub fn calc_baud_rate( ); let psd = prescaler_division_factor.unwrap_or(0); - let a = frequency; - let b = 16 * (psd + 1) * divisor; + // Calculate in u64, as this can overflow u32 for large inputs. + let a = u64::from(frequency); + let b = 16 * (u64::from(psd) + 1) * u64::from(divisor); - if a.is_multiple_of(b) { - Ok(a / b) + if b != 0 && a.is_multiple_of(b) { + // The result is at most `frequency`. + Ok((a / b) as u32) } else { Err(NonIntegerBaudRateError { frequency, @@ -1212,6 +1214,10 @@ mod tests { prescaler_division_factor: None, }) ); + assert!(matches!( + calc_divisor(CLK_FREQUENCY_HZ, u32::MAX, Some(0xf)), + Err(NonIntegerDivisorError { .. }) + )); } // ── FCR ────────────────────────────────────────────────────────────────── From c35ff082f299a1edab095ee51a114d006ddfcaff Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:40:23 +0200 Subject: [PATCH 4/8] uart: discard stale data in init() like Linux Without FIFO, the FCR reset bits have no effect, so a byte received before init() stayed in RBR and was returned as new data. Read it out right after configuring the FIFOs, before signaling readiness via DTR and RTS, and also read IIR at the end, as Linux does. --- CHANGELOG.md | 1 + src/lib.rs | 26 +++++++++++++++++++++----- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96a2196..94a9617 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ - New `Uart16550Tty::new_with_backend()`. - Fixed: `init()` no longer leaves DLAB set if the baud rate is invalid. - Fixed: `calc_baud_rate()` no longer overflows for large inputs. +- Fixed: `init()` discards stale received data also without FIFO. ## 0.8.1 - 2026-09-14 diff --git a/src/lib.rs b/src/lib.rs index 6925b63..0218ba8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -599,6 +599,19 @@ impl Uart16550 { self.configure_fcr(); } + // Discard stale data, like Linux does. Without FIFO, FCR can't clear + // it. Do this before signaling readiness via DTR and RTS. + // SAFETY: We operate on valid register addresses. + unsafe { + for _ in 0..=FIFO_SIZE { + let lsr = LSR::from_bits_retain(self.backend.read(offsets::LSR as u8)); + if !lsr.contains(LSR::DATA_READY) { + break; + } + let _ = self.backend.read(offsets::DATA as u8); + } + } + // Set modem control register. // SAFETY: We operate on valid register addresses. unsafe { @@ -637,13 +650,16 @@ impl Uart16550 { } } - // Bring status bits into a clean state. - { + // Bring status bits into a clean state, like Linux does in + // `serial8250_clear_interrupts()`. + // SAFETY: We operate on valid register addresses. + unsafe { // Clear receiver line-status deltas / interrupt indicators. - let _ = self.lsr(); - + let _ = self.backend.read(offsets::LSR as u8); + // Clear pending interrupt indications. + let _ = self.backend.read(offsets::ISR as u8); // Clear modem-status deltas. - let _ = self.msr(); + let _ = self.backend.read(offsets::MSR as u8); } // Set interrupts as the last step. From e5cf27d1e8b092d829eff49025c7a51be862301d Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:28:02 +0200 Subject: [PATCH 5/8] uart: restore MCR when the loopback test fails On errors, test_loopback() returned early and left the device in loopback mode, which disconnects it from the outside. Restore MCR on all paths. --- CHANGELOG.md | 1 + src/lib.rs | 109 ++++++++++++++++++++++++++++----------------------- 2 files changed, 62 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94a9617..12a33cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ - Fixed: `init()` no longer leaves DLAB set if the baud rate is invalid. - Fixed: `calc_baud_rate()` no longer overflows for large inputs. - Fixed: `init()` discards stale received data also without FIFO. +- Fixed: `test_loopback()` leaves loopback mode also when it fails. ## 0.8.1 - 2026-09-14 diff --git a/src/lib.rs b/src/lib.rs index 0218ba8..04a7e90 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -682,66 +682,79 @@ impl Uart16550 { /// - The FIFO will be re-configured and activated by this test. // NEVER CHANGE this function without testing on real hardware! pub fn test_loopback(&mut self) -> Result<(), LoopbackError> { - /// Single test byte. Chosen arbitrarily. - const TEST_BYTE: u8 = 0x42; - /// Test message. Must be smaller than [`FIFO_SIZE`]. - const TEST_MESSAGE: [u8; FIFO_SIZE] = *b"hello world!1337"; + let old_mcr = self.mcr(); + // We also disable interrupts here. // SAFETY: We operate on valid register addresses. unsafe { - let old_mcr = self.mcr(); - - // We also disable interrupts here. self.backend .write(offsets::MCR as u8, MCR::LOOP_BACK.bits()); + } + + let result = self.test_loopback_in_loopback_mode(); + + // Restore MCR on all paths, so the device doesn't stay in loopback + // mode. + // SAFETY: We operate on valid register addresses. + unsafe { + self.backend.write(offsets::MCR as u8, old_mcr.bits()); + } + + result + } + + /// The part of [`Self::test_loopback`] that runs in loopback mode. + fn test_loopback_in_loopback_mode(&mut self) -> Result<(), LoopbackError> { + /// Single test byte. Chosen arbitrarily. + const TEST_BYTE: u8 = 0x42; + /// Test message. Must be smaller than [`FIFO_SIZE`]. + const TEST_MESSAGE: [u8; FIFO_SIZE] = *b"hello world!1337"; + // SAFETY: We have exclusive access to the device. + unsafe { // Reset send and receive FIFOs. self.configure_fcr(); + } - // Drain any data that might be still there - while self.receive_bytes(&mut [0]) > 0 {} - - // First: check a single byte - { - self.try_send_byte(TEST_BYTE) - .map_err(LoopbackError::SendError)?; - - let mut read_buf = [0]; - // Tests on real hardware showed that there can be a short delay - // until we can read the data. Therefore, we use the blocking - // API rather than `try_receive_byte()`. - self.receive_bytes_exact(&mut read_buf); - let read = read_buf[0]; - if read != TEST_BYTE { - return Err(LoopbackError::UnexpectedLoopbackByte { - expected: TEST_BYTE, - actual: read, - }); - } - } + // Drain any data that might be still there + while self.receive_bytes(&mut [0]) > 0 {} - // Now check sending and reading a whole message. - // This requires the FIFO to be activated. - { - self.send_bytes_exact(&TEST_MESSAGE); - - let mut read_buffer = [0_u8; TEST_MESSAGE.len()]; - // Tests on real hardware showed that there can be a short delay - // until we can read the data. Therefore, we use the blocking - // API rather than `receive_bytes()`. - self.receive_bytes_exact(&mut read_buffer); - let read = read_buffer; - - if read != TEST_MESSAGE { - return Err(LoopbackError::UnexpectedLoopbackMsg { - expected: TEST_MESSAGE, - actual: read_buffer, - }); - } + // First: check a single byte + { + self.try_send_byte(TEST_BYTE) + .map_err(LoopbackError::SendError)?; + + let mut read_buf = [0]; + // Tests on real hardware showed that there can be a short delay + // until we can read the data. Therefore, we use the blocking + // API rather than `try_receive_byte()`. + self.receive_bytes_exact(&mut read_buf); + let read = read_buf[0]; + if read != TEST_BYTE { + return Err(LoopbackError::UnexpectedLoopbackByte { + expected: TEST_BYTE, + actual: read, + }); } + } - // restore MCR - self.backend.write(offsets::MCR as u8, old_mcr.bits()); + // Now check sending and reading a whole message. + // This requires the FIFO to be activated. + { + self.send_bytes_exact(&TEST_MESSAGE); + + let mut read_buffer = [0_u8; TEST_MESSAGE.len()]; + // Tests on real hardware showed that there can be a short delay + // until we can read the data. Therefore, we use the blocking + // API rather than `receive_bytes()`. + self.receive_bytes_exact(&mut read_buffer); + + if read_buffer != TEST_MESSAGE { + return Err(LoopbackError::UnexpectedLoopbackMsg { + expected: TEST_MESSAGE, + actual: read_buffer, + }); + } } Ok(()) From 262dac46ee8d3fb4abb55a0daeb69b592e8c9b4c Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:30:27 +0200 Subject: [PATCH 6/8] embedded-io: wait for the transmitter in flush() flush() returned immediately, although up to a FIFO of data was still being sent. The embedded-io contract requires that all buffered data reaches its destination, e.g., before a reset or a baud rate change. --- CHANGELOG.md | 1 + src/embedded_io.rs | 9 +++++++++ 2 files changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12a33cf..67ee7b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ - Fixed: `calc_baud_rate()` no longer overflows for large inputs. - Fixed: `init()` discards stale received data also without FIFO. - Fixed: `test_loopback()` leaves loopback mode also when it fails. +- Fixed: `embedded_io::Write::flush()` waits until the transmitter is empty. ## 0.8.1 - 2026-09-14 diff --git a/src/embedded_io.rs b/src/embedded_io.rs index a7c88b4..31be8f3 100644 --- a/src/embedded_io.rs +++ b/src/embedded_io.rs @@ -7,6 +7,7 @@ use embedded_io::{ErrorType, Read, ReadReady, Write, WriteReady}; use crate::Uart16550; use crate::backend::Backend; +use crate::spec::registers::LSR; impl ErrorType for Uart16550 { type Error = Infallible; @@ -27,7 +28,15 @@ impl Write for Uart16550 { } } + /// Waits until the transmitter is empty. + /// + /// This blocks as long as the transmitter is stalled, e.g., by hardware + /// flow control. fn flush(&mut self) -> Result<(), Self::Error> { + // Wait until the transmitter sent everything, like Linux' tcdrain(). + while !self.lsr().contains(LSR::TRANSMITTER_EMPTY) { + hint::spin_loop(); + } Ok(()) } } From 42a414f9b3e8c33047e3c0dedaedeae3da99e881 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 19:31:13 +0200 Subject: [PATCH 7/8] uart: restore the scratch register in check_present() The presence check overwrote the scratch register, which firmware or other software may use. Linux restores it in its scratch test, too. --- CHANGELOG.md | 1 + src/lib.rs | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ee7b8..b291f11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ - Fixed: `init()` discards stale received data also without FIFO. - Fixed: `test_loopback()` leaves loopback mode also when it fails. - Fixed: `embedded_io::Write::flush()` waits until the transmitter is empty. +- Fixed: `check_present()` restores the scratch register. ## 0.8.1 - 2026-09-14 diff --git a/src/lib.rs b/src/lib.rs index 04a7e90..ecb8289 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -492,13 +492,20 @@ impl Uart16550 { /// Checks if a device answers at the base address by checking the [`SPR`] /// register. pub fn check_present(&mut self) -> bool { - [0x42, 0x73].into_iter().all(|pattern| { + // Others may use the scratch register, so restore it. + let old_spr = self.spr(); + let present = [0x42, 0x73].into_iter().all(|pattern| { // SAFETY: We operate on valid register addresses. unsafe { self.backend.write(offsets::SPR as u8, pattern); self.backend.read(offsets::SPR as u8) == pattern } - }) + }); + // SAFETY: We operate on valid register addresses. + unsafe { + self.backend.write(offsets::SPR as u8, old_spr); + } + present } /// Initializes the devices according to the provided [`Config`] including a From 7908cf95da803d4faac48a000389a07400508531 Mon Sep 17 00:00:00 2001 From: Philipp Schuster Date: Sat, 10 Oct 2026 20:00:14 +0200 Subject: [PATCH 8/8] doc: fix the description of InterruptType::from_bits() It described the meaning of ISR bit 0 inverted. --- src/spec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/spec.rs b/src/spec.rs index 5725759..0946abe 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -424,8 +424,8 @@ pub mod registers { /// Returns the [`InterruptType`] that corresponds to the bits in /// [`ISR`]. /// - /// If the lowest bit is `0` (interrupt enable), this function - /// returns `None` + /// If the lowest bit is `1` (no interrupt pending), this function + /// returns `None`. #[must_use] pub fn from_bits(isr_bits: u8) -> Option { let bits = isr_bits & 0xf;