From d07a33ec0b4ce14e23d9aed2c5ecb015ac4b993b Mon Sep 17 00:00:00 2001 From: Jeremy Soller Date: Wed, 26 Nov 2025 09:30:45 -0700 Subject: [PATCH] Add timeouts to more driver spin loops --- common/src/lib.rs | 2 ++ common/src/timeout.rs | 45 ++++++++++++++++++++++++++++++++ input/ps2d/src/controller.rs | 32 +++-------------------- net/rtl8139d/src/device.rs | 17 +++++++----- net/rtl8168d/src/device.rs | 22 +++++++++------- storage/ahcid/src/ahci/hba.rs | 40 +++++++++++++--------------- storage/nvmed/src/main.rs | 2 +- storage/nvmed/src/nvme/mod.rs | 49 +++++++++++++++++++++++------------ 8 files changed, 125 insertions(+), 84 deletions(-) create mode 100644 common/src/timeout.rs diff --git a/common/src/lib.rs b/common/src/lib.rs index 1966d5b05a..275caec413 100644 --- a/common/src/lib.rs +++ b/common/src/lib.rs @@ -16,6 +16,8 @@ pub mod io; mod logger; /// The Scatter Gather List (SGL) API for drivers. pub mod sgl; +/// Low latency timeout for driver loops +pub mod timeout; pub use logger::{output_level, file_level, setup_logging}; diff --git a/common/src/timeout.rs b/common/src/timeout.rs new file mode 100644 index 0000000000..ec226d1629 --- /dev/null +++ b/common/src/timeout.rs @@ -0,0 +1,45 @@ +use std::{thread, time::{Duration, Instant}}; + +pub struct Timeout { + instant: Instant, + duration: Duration, +} + +impl Timeout { + #[inline] + pub fn new(duration: Duration) -> Self { + Self { + instant: Instant::now(), + duration, + } + } + + #[inline] + pub fn from_micros(micros: u64) -> Self { + Self::new(Duration::from_micros(micros)) + } + + #[inline] + pub fn from_millis(millis: u64) -> Self { + Self::new(Duration::from_millis(millis)) + } + + #[inline] + pub fn from_secs(secs: u64) -> Self { + Self::new(Duration::from_secs(secs)) + } + + #[inline] + pub fn run(&self) -> Result<(), ()> { + if self.instant.elapsed() < self.duration { + // Sleeps in Redox are only evaluated on PIT ticks (a few ms), which is not + // short enough for a reasonably responsive timeout. However, the clock is + // highly accurate. So, we yield instead of sleep to reduce latency. + //TODO: allow timeout that spins instead of yields? + std::thread::yield_now(); + Ok(()) + } else { + Err(()) + } + } +} \ No newline at end of file diff --git a/input/ps2d/src/controller.rs b/input/ps2d/src/controller.rs index fbbbf75cb0..7a58f3de3a 100644 --- a/input/ps2d/src/controller.rs +++ b/input/ps2d/src/controller.rs @@ -1,4 +1,4 @@ -use common::io::{Io, Pio, ReadOnly, WriteOnly}; +use common::{io::{Io, Pio, ReadOnly, WriteOnly}, timeout::Timeout}; use log::{debug, error, info, trace}; use std::{ @@ -103,32 +103,6 @@ const DEFAULT_TIMEOUT: u64 = 50_000; // Reset timeout in microseconds const RESET_TIMEOUT: u64 = 500_000; -struct Timeout { - instant: Instant, - duration: Duration, -} - -impl Timeout { - fn new(micros: u64) -> Self { - Self { - instant: Instant::now(), - duration: Duration::from_micros(micros), - } - } - - fn run(&self) -> Result<(), ()> { - if self.instant.elapsed() < self.duration { - // Sleeps in Redox are only evaluated on PIT ticks (a few ms), which is not - // short enough for a reasonably responsive timeout. However, the clock is - // highly accurate. So, we yield instead of sleep to reduce latency. - thread::yield_now(); - Ok(()) - } else { - Err(()) - } - } -} - pub struct Ps2 { data: Pio, status: ReadOnly>, @@ -149,7 +123,7 @@ impl Ps2 { } fn wait_read(&mut self, micros: u64) -> Result<(), Error> { - let timeout = Timeout::new(micros); + let timeout = Timeout::from_micros(micros); loop { if self.status().contains(StatusFlags::OUTPUT_FULL) { return Ok(()); @@ -159,7 +133,7 @@ impl Ps2 { } fn wait_write(&mut self, micros: u64) -> Result<(), Error> { - let timeout = Timeout::new(micros); + let timeout = Timeout::from_micros(micros); loop { if !self.status().contains(StatusFlags::INPUT_FULL) { return Ok(()); diff --git a/net/rtl8139d/src/device.rs b/net/rtl8139d/src/device.rs index 1245d864f6..37167ee2fb 100644 --- a/net/rtl8139d/src/device.rs +++ b/net/rtl8139d/src/device.rs @@ -6,6 +6,7 @@ use syscall::error::{Error, Result, EIO, EMSGSIZE}; use common::dma::Dma; use common::io::{Io, Mmio, ReadOnly}; +use common::timeout::Timeout; const RX_BUFFER_SIZE: usize = 64 * 1024; @@ -219,7 +220,7 @@ impl Rtl8139 { mac_address: [0; 6], }; - module.init(); + module.init()?; Ok(module) } @@ -253,7 +254,7 @@ impl Rtl8139 { } } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&mut self) -> Result<()> { let mac_low = self.regs.mac[0].read(); let mac_high = self.regs.mac[1].read(); let mac = [ @@ -276,10 +277,13 @@ impl Rtl8139 { self.mac_address = mac; // Reset - this will disable tx and rx, reinitialize FIFOs, and set the system buffer pointer to the initial value - log::debug!("Reset"); - self.regs.cr.writef(CR_RST, true); - while self.regs.cr.readf(CR_RST) { - core::hint::spin_loop(); + { + log::debug!("Reset"); + let timeout = Timeout::from_secs(1); + self.regs.cr.writef(CR_RST, true); + while self.regs.cr.readf(CR_RST) { + timeout.run().map_err(|()| Error::new(EIO))?; + } } // Set up rx buffer @@ -300,5 +304,6 @@ impl Rtl8139 { self.regs.cr.writef(CR_RE | CR_TE, true); log::debug!("Complete!"); + Ok(()) } } diff --git a/net/rtl8168d/src/device.rs b/net/rtl8168d/src/device.rs index fe6d7f5ebd..ce0675877a 100644 --- a/net/rtl8168d/src/device.rs +++ b/net/rtl8168d/src/device.rs @@ -1,11 +1,11 @@ use std::convert::TryInto; use std::mem; -use common::io::{Io, Mmio, ReadOnly}; -use driver_network::NetworkAdapter; -use syscall::error::{Error, Result, EMSGSIZE}; - use common::dma::Dma; +use common::io::{Io, Mmio, ReadOnly}; +use common::timeout::Timeout; +use driver_network::NetworkAdapter; +use syscall::error::{Error, Result, EIO, EMSGSIZE}; #[repr(C, packed)] struct Regs { @@ -220,7 +220,7 @@ impl Rtl8168 { } } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&mut self) -> Result<()> { let mac_low = self.regs.mac[0].read(); let mac_high = self.regs.mac[1].read(); let mac = [ @@ -243,10 +243,13 @@ impl Rtl8168 { self.mac_address = mac; // Reset - this will disable tx and rx, reinitialize FIFOs, and set the system buffer pointer to the initial value - log::debug!("Reset"); - self.regs.cmd.writef(1 << 4, true); - while self.regs.cmd.readf(1 << 4) { - core::hint::spin_loop(); + { + log::debug!("Reset"); + let timeout = Timeout::from_secs(1); + self.regs.cmd.writef(1 << 4, true); + while self.regs.cmd.readf(1 << 4) { + timeout.run().map_err(|()| Error::new(EIO))?; + } } // Set up rx buffers @@ -337,5 +340,6 @@ impl Rtl8168 { self.regs.cmd_9346.write(0); log::debug!("Complete!"); + Ok(()) } } diff --git a/storage/ahcid/src/ahci/hba.rs b/storage/ahcid/src/ahci/hba.rs index 1e788dc912..af7232762b 100644 --- a/storage/ahcid/src/ahci/hba.rs +++ b/storage/ahcid/src/ahci/hba.rs @@ -4,13 +4,13 @@ use std::ops::DerefMut; use std::time::{Duration, Instant}; use std::{ptr, u32}; +use common::dma::Dma; use common::io::{Io, Mmio}; +use common::timeout::Timeout; use syscall::error::{Error, Result, EIO}; use super::fis::{FisRegH2D, FisType}; -use common::dma::Dma; - const ATA_CMD_READ_DMA_EXT: u8 = 0x25; const ATA_CMD_WRITE_DMA_EXT: u8 = 0x35; const ATA_CMD_IDENTIFY: u8 = 0xEC; @@ -80,13 +80,12 @@ impl HbaPort { } pub fn start(&mut self) -> Result<()> { - let timer = Instant::now(); + let timeout = Timeout::new(TIMEOUT); while self.cmd.readf(HBA_PORT_CMD_CR) { - core::hint::spin_loop(); - if timer.elapsed() >= TIMEOUT { + timeout.run().map_err(|()| { log::error!("HBA start timed out"); - return Err(Error::new(EIO)); - } + Error::new(EIO) + })?; } self.cmd.writef(HBA_PORT_CMD_FRE | HBA_PORT_CMD_ST, true); @@ -96,13 +95,12 @@ impl HbaPort { pub fn stop(&mut self) -> Result<()> { self.cmd.writef(HBA_PORT_CMD_ST, false); - let timer = Instant::now(); + let timeout = Timeout::new(TIMEOUT); while self.cmd.readf(HBA_PORT_CMD_FR | HBA_PORT_CMD_CR) { - core::hint::spin_loop(); - if timer.elapsed() >= TIMEOUT { + timeout.run().map_err(|()| { log::error!("HBA stop timed out"); - return Err(Error::new(EIO)); - } + Error::new(EIO) + })?; } self.cmd.writef(HBA_PORT_CMD_FRE, false); @@ -404,13 +402,12 @@ impl HbaPort { callback(cmdheader, cmdfis, prdt_entry, acmd) } - let timer = Instant::now(); + let timeout = Timeout::new(TIMEOUT); while self.tfd.readf((ATA_DEV_BUSY | ATA_DEV_DRQ) as u32) { - core::hint::spin_loop(); - if timer.elapsed() >= TIMEOUT { + timeout.run().map_err(|()| { log::error!("HBA ata_start timeout"); - return Err(Error::new(EIO)); - } + Error::new(EIO) + })?; } self.ci.writef(1 << slot, true); @@ -426,13 +423,12 @@ impl HbaPort { } pub fn ata_stop(&mut self, slot: u32) -> Result<()> { - let timer = Instant::now(); + let timeout = Timeout::new(TIMEOUT); while self.ata_running(slot) { - core::hint::spin_loop(); - if timer.elapsed() >= TIMEOUT { + timeout.run().map_err(|()| { log::error!("HBA ata_stop timeout"); - return Err(Error::new(EIO)); - } + Error::new(EIO) + })?; } self.stop()?; diff --git a/storage/nvmed/src/main.rs b/storage/nvmed/src/main.rs index 6112756c76..ed83b7809c 100644 --- a/storage/nvmed/src/main.rs +++ b/storage/nvmed/src/main.rs @@ -169,7 +169,7 @@ fn daemon(daemon: redox_daemon::Daemon) -> ! { let mut nvme = Nvme::new(address.as_ptr() as usize, interrupt_method, pcid_handle) .expect("nvmed: failed to allocate driver data"); - unsafe { nvme.init() } + unsafe { nvme.init().expect("nvmed: failed to init") } log::debug!("Finished base initialization"); let nvme = Arc::new(nvme); diff --git a/storage/nvmed/src/nvme/mod.rs b/storage/nvmed/src/nvme/mod.rs index b10369e2cd..faa6ab4ee0 100644 --- a/storage/nvmed/src/nvme/mod.rs +++ b/storage/nvmed/src/nvme/mod.rs @@ -9,6 +9,7 @@ use std::sync::Arc; use parking_lot::{Mutex, ReentrantMutex, RwLock}; use common::io::{Io, Mmio}; +use common::timeout::Timeout; use syscall::error::{Error, Result, EIO}; use common::dma::Dma; @@ -190,7 +191,7 @@ impl Nvme { self.doorbell_write(2 * (qid as usize) + 1, u32::from(head)); } - pub unsafe fn init(&mut self) { + pub unsafe fn init(&mut self) -> Result<()> { let thread_ctxts = self.thread_ctxts.get_mut(); { let regs = self.regs.read(); @@ -204,14 +205,20 @@ impl Nvme { log::debug!("Disabling controller."); self.regs.get_mut().cc.writef(1, false); - log::trace!("Waiting for not ready."); - loop { - let csts = self.regs.get_mut().csts.read(); - log::trace!("CSTS: {:X}", csts); - if csts & 1 == 1 { - std::hint::spin_loop(); - } else { - break; + { + log::trace!("Waiting for not ready."); + let mut timeout = Timeout::from_secs(5); + loop { + let csts = self.regs.get_mut().csts.read(); + log::trace!("CSTS: {:X}", csts); + if csts & 1 == 1 { + timeout.run().map_err(|()| { + log::error!("failed to wait for not ready"); + Error::new(EIO) + })?; + } else { + break; + } } } @@ -269,16 +276,24 @@ impl Nvme { log::debug!("Enabling controller."); self.regs.get_mut().cc.writef(1, true); - log::debug!("Waiting for ready"); - loop { - let csts = self.regs.get_mut().csts.read(); - log::debug!("CSTS: {:X}", csts); - if csts & 1 == 0 { - std::hint::spin_loop(); - } else { - break; + { + log::debug!("Waiting for ready"); + let mut timeout = Timeout::from_secs(5); + loop { + let csts = self.regs.get_mut().csts.read(); + log::debug!("CSTS: {:X}", csts); + if csts & 1 == 0 { + timeout.run().map_err(|()| { + log::error!("failed to wait for ready"); + Error::new(EIO) + })?; + } else { + break; + } } } + + Ok(()) } /// Masks or unmasks multiple vectors.