From 656b8a8dcfbb10a330edc2ea6d9417d0380ee192 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 29 Mar 2024 15:16:59 -0700 Subject: [PATCH 01/26] stm32h7-spi: don't reject xfers > u16::max --- drv/stm32h7-spi-server-core/src/lib.rs | 411 +++++++++++++------------ 1 file changed, 214 insertions(+), 197 deletions(-) diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index 58f6fc1592..90e331b4ae 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -64,7 +64,13 @@ pub struct SpiServerCore { #[derive(Copy, Clone, PartialEq, counters::Count)] enum Trace { - Start(#[count(children)] SpiOperation, (u16, u16)), + Start { + #[count(children)] + op: SpiOperation, + src_len: u16, + dst_len: u16, + iter: usize, + }, Tx(u8), Rx(u8), WaitISR(u32), @@ -84,7 +90,7 @@ pub struct LockState { /// [`SpiServerCore::exchange`]. #[derive(Copy, Clone, Eq, PartialEq)] pub enum TransferError { - /// Transfer size is 0 or exceeds maximum + /// Transfer size is 0. BadTransferSize = 1, /// Attempt to operate device N when there is no device N, or an attempt to @@ -331,229 +337,240 @@ impl SpiServerCore { panic!(); } - // Get the required transfer lengths in the src and dest directions. - // - // Sizes that overflow a u16 are invalid and we reject them - let src_len: u16 = tx - .as_ref() - .map(|tx| tx.remaining_size()) - .unwrap_or(0) - .try_into() - .map_err(|_| TransferError::BadTransferSize)?; - let dest_len: u16 = rx - .as_ref() - .map(|rx| rx.remaining_size()) - .unwrap_or(0) - .try_into() - .map_err(|_| TransferError::BadTransferSize)?; - let overall_len = src_len.max(dest_len); - // Zero-byte SPI transactions don't make sense and we'll // decline them. - if overall_len == 0 { + let tx_len = tx.as_ref().map(BufWrite::remaining_size).unwrap_or(0); + let rx_len = rx.as_ref().map(BufReader::remaining_size).unwrap_or(0); + if tx_len + rx_len == 0 { return Err(TransferError::BadTransferSize); } - // We have a reasonable-looking request containing reasonable-looking - // lease(s). This is our commit point. - ringbuf_entry!(Trace::Start(op, (src_len, dest_len))); - - // Switch the mux to the requested port. - let current_mux_index = self.current_mux_index.get(); - if device.mux_index != current_mux_index { - deactivate_mux_option( - &CONFIG.mux_options[current_mux_index], - &self.sys, - ); - activate_mux_option( - &CONFIG.mux_options[device.mux_index], - &self.sys, - &self.spi, - ); - // Remember this for later to avoid unnecessary - // switching. - self.current_mux_index.set(device.mux_index); - } - - // Make sure SPI is on. - // - // Due to driver limitations we will only move up to 64kiB - // per transaction. It would be worth lifting this - // limitation, maybe. Doing so would require managing data - // in 64kiB chunks (because the peripheral is 16-bit) and - // using the "reload" facility on the peripheral. - self.spi.enable(overall_len, device.clock_divider); - - // Load transfer count and start the state machine. At this - // point we _have_ to move the specified number of bytes - // through (or explicitly cancel, but we don't). - self.spi.start(); - - // As you might expect, we will work from byte 0 to the end - // of each buffer. There are two complications: - // - // 1. Transmit and receive can be at different positions -- - // transmit will tend to lead receive, because the SPI - // unit contains FIFOs. - // - // 2. We're only keeping track of position in the buffers - // we're using: both tx and rx are `Option`. - // - // The BufReader/Writer types manage position tracking for us. - - // Enable interrupt on the conditions we're interested in. - self.spi.enable_transfer_interrupts(); + let mut iter = 0; + loop { + // Get the required transfer lengths in the src and dest directions. + // + // If the size is greater than `u16::MAX`, transfer `u16::MAX` bytes and + // then keep looping. + let src_len = + tx.as_ref().map(|tx| tx.remaining_size()).unwrap_or(0) as u16; + let dst_len = + rx.as_ref().map(|rx| rx.remaining_size()).unwrap_or(0) as u16; + let overall_len = src_len.max(dst_len); + + // If we've consumed all the remaining buffer in both directions, + // we're done! + if overall_len == 0 { + break; + } - self.spi.clear_eot(); + // We have a reasonable-looking request containing reasonable-looking + // lease(s). This is our commit point. + ringbuf_entry!(Trace::Start { + op, + src_len, + dst_len, + iter + }); + iter += 1; + + // Switch the mux to the requested port. + let current_mux_index = self.current_mux_index.get(); + if device.mux_index != current_mux_index { + deactivate_mux_option( + &CONFIG.mux_options[current_mux_index], + &self.sys, + ); + activate_mux_option( + &CONFIG.mux_options[device.mux_index], + &self.sys, + &self.spi, + ); + // Remember this for later to avoid unnecessary + // switching. + self.current_mux_index.set(device.mux_index); + } - // We're doing this! Check if we need to control CS. - let cs_override = self.lock_holder.get().is_some(); - if !cs_override { - for pin in device.cs { - self.sys.gpio_reset(*pin); + // Make sure SPI is on. + // + // Due to driver limitations we will only move up to 64kiB + // per transaction. It would be worth lifting this + // limitation, maybe. Doing so would require managing data + // in 64kiB chunks (because the peripheral is 16-bit) and + // using the "reload" facility on the peripheral. + self.spi.enable(overall_len, device.clock_divider); + + // Load transfer count and start the state machine. At this + // point we _have_ to move the specified number of bytes + // through (or explicitly cancel, but we don't). + self.spi.start(); + + // As you might expect, we will work from byte 0 to the end + // of each buffer. There are two complications: + // + // 1. Transmit and receive can be at different positions -- + // transmit will tend to lead receive, because the SPI + // unit contains FIFOs. + // + // 2. We're only keeping track of position in the buffers + // we're using: both tx and rx are `Option`. + // + // The BufReader/Writer types manage position tracking for us. + + // Enable interrupt on the conditions we're interested in. + self.spi.enable_transfer_interrupts(); + + self.spi.clear_eot(); + + // We're doing this! Check if we need to control CS. + let cs_override = self.lock_holder.get().is_some(); + if !cs_override { + for pin in device.cs { + self.sys.gpio_reset(*pin); + } } - } - // We use this to exert backpressure on the TX state machine as the RX - // FIFO fills. Its initial value is the configured FIFO size, because - // the FIFO size varies on SPI blocks on the H7; it would be nice if we - // could read the configured FIFO size out of the block, but that does - // not appear to be possible. - // - // See reference manual table 409 for details. - let mut tx_permits = FIFO_DEPTH; - - // Track number of bytes sent and received. Sent bytes will lead - // received bytes. Received bytes indicate overall progress and - // completion. - let mut tx_count = 0; - let mut rx_count = 0; - - // The end of the exchange is signaled by rx_count reaching the - // overall_len. This is true even if the caller's rx lease is shorter or - // missing, because we have to pull bytes from the FIFO to avoid overrun - // conditions. - while rx_count < overall_len { - // At the end of this loop we're going to sleep if there's no - // obvious work to be done. Sleeping is not free, so, we only do it - // if this flag is set. (It defaults to set, we'll clear it if work - // appears below.) - let mut should_sleep = true; - - // TX engine. We continue moving bytes while these three conditions - // hold: - // - More bytes need to be sent. - // - Permits are available. - // - The TX FIFO has space. - while tx_count < overall_len - && tx_permits > 0 - && self.spi.can_tx_frame() - { - // The next byte to TX will come from the caller, if we haven't - // run off the end of their lease, or the fixed padding byte if - // we have. - let byte = if let Some(txbuf) = &mut tx { - if let Some(b) = txbuf.read() { - b + // We use this to exert backpressure on the TX state machine as the RX + // FIFO fills. Its initial value is the configured FIFO size, because + // the FIFO size varies on SPI blocks on the H7; it would be nice if we + // could read the configured FIFO size out of the block, but that does + // not appear to be possible. + // + // See reference manual table 409 for details. + let mut tx_permits = FIFO_DEPTH; + + // Track number of bytes sent and received. Sent bytes will lead + // received bytes. Received bytes indicate overall progress and + // completion. + let mut tx_count = 0; + let mut rx_count = 0; + + // The end of the exchange is signaled by rx_count reaching the + // overall_len. This is true even if the caller's rx lease is shorter or + // missing, because we have to pull bytes from the FIFO to avoid overrun + // conditions. + while rx_count < overall_len { + // At the end of this loop we're going to sleep if there's no + // obvious work to be done. Sleeping is not free, so, we only do it + // if this flag is set. (It defaults to set, we'll clear it if work + // appears below.) + let mut should_sleep = true; + + // TX engine. We continue moving bytes while these three conditions + // hold: + // - More bytes need to be sent. + // - Permits are available. + // - The TX FIFO has space. + while tx_count < overall_len + && tx_permits > 0 + && self.spi.can_tx_frame() + { + // The next byte to TX will come from the caller, if we haven't + // run off the end of their lease, or the fixed padding byte if + // we have. + let byte = if let Some(txbuf) = &mut tx { + if let Some(b) = txbuf.read() { + b + } else { + // We've hit the end of the lease. Stop checking. + tx = None; + 0 + } } else { - // We've hit the end of the lease. Stop checking. - tx = None; 0 - } - } else { - 0 - }; - - ringbuf_entry!(Trace::Tx(byte)); - self.spi.send8(byte); - tx_count += 1; - - // Consume one TX permit to make sure we don't overrun the RX - // fifo. - tx_permits -= 1; - - if tx_permits == 0 || tx_count == overall_len { - // We're either done, or we need to idle until the RX engine - // catches up. Either way, stop generating interrupts. - self.spi.disable_can_tx_interrupt(); - } + }; - // We don't adjust should_sleep in the TX engine because, if we - // leave this loop, we've done all the TX work we can -- and - // we're about to check for RX work unconditionally below. So, - // from the perspective of the TX engine, should_sleep is always - // true at this point, and the RX engine gets to make the final - // decision. - } + ringbuf_entry!(Trace::Tx(byte)); + self.spi.send8(byte); + tx_count += 1; + + // Consume one TX permit to make sure we don't overrun the RX + // fifo. + tx_permits -= 1; + + if tx_permits == 0 || tx_count == overall_len { + // We're either done, or we need to idle until the RX engine + // catches up. Either way, stop generating interrupts. + self.spi.disable_can_tx_interrupt(); + } - // Drain bytes from the RX FIFO. - while self.spi.can_rx_byte() { - // We didn't check rx_count < overall_len above because, if we - // got to that point, it would mean the SPI hardware gave us - // more bytes than we sent. This would be bad. And so, we'll - // detect that condition aggressively: - if rx_count >= overall_len { - panic!(); + // We don't adjust should_sleep in the TX engine because, if we + // leave this loop, we've done all the TX work we can -- and + // we're about to check for RX work unconditionally below. So, + // from the perspective of the TX engine, should_sleep is always + // true at this point, and the RX engine gets to make the final + // decision. } - // Pull byte from RX FIFO. - let b = self.spi.recv8(); - ringbuf_entry!(Trace::Rx(b)); - rx_count += 1; + // Drain bytes from the RX FIFO. + while self.spi.can_rx_byte() { + // We didn't check rx_count < overall_len above because, if we + // got to that point, it would mean the SPI hardware gave us + // more bytes than we sent. This would be bad. And so, we'll + // detect that condition aggressively: + if rx_count >= overall_len { + panic!(); + } - // Allow another byte to be inserted in the TX FIFO. - tx_permits += 1; + // Pull byte from RX FIFO. + let b = self.spi.recv8(); + ringbuf_entry!(Trace::Rx(b)); + rx_count += 1; + + // Allow another byte to be inserted in the TX FIFO. + tx_permits += 1; + + // Deposit the byte if we're still within the bounds of the + // caller's incoming lease. + if let Some(rx_reader) = &mut rx { + if rx_reader.write(b).is_err() { + // We're off the end. Stop checking. + rx = None; + } + } - // Deposit the byte if we're still within the bounds of the - // caller's incoming lease. - if let Some(rx_reader) = &mut rx { - if rx_reader.write(b).is_err() { - // We're off the end. Stop checking. - rx = None; + // By releasing a TX permit, we might have unblocked the TX + // engine. We can detect this when tx_permits goes 0->1. If this + // occurs, we should turn its interrupt back on, but only if + // it's still working. + if tx_permits == 1 && tx_count < overall_len { + self.spi.enable_can_tx_interrupt(); } - } - // By releasing a TX permit, we might have unblocked the TX - // engine. We can detect this when tx_permits goes 0->1. If this - // occurs, we should turn its interrupt back on, but only if - // it's still working. - if tx_permits == 1 && tx_count < overall_len { - self.spi.enable_can_tx_interrupt(); + // We've done some work, which means some time has elapsed, + // which means it's possible that room in the TX FIFO has opened + // up. So, let's not sleep. + should_sleep = false; } - // We've done some work, which means some time has elapsed, - // which means it's possible that room in the TX FIFO has opened - // up. So, let's not sleep. - should_sleep = false; - } + if should_sleep { + ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); - if should_sleep { - ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); + if self.spi.check_overrun() { + panic!(); + } - if self.spi.check_overrun() { - panic!(); + // Allow the controller interrupt to post to our + // notification set. + sys_irq_control(self.irq_mask, true); + // Wait for our notification set to get, well, set. We ignore + // the result of this because an error would mean the kernel + // violated the ABI, which we can't usefully respond to. + let _ = + sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); } + } - // Allow the controller interrupt to post to our - // notification set. - sys_irq_control(self.irq_mask, true); - // Wait for our notification set to get, well, set. We ignore - // the result of this because an error would mean the kernel - // violated the ABI, which we can't usefully respond to. - let _ = sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); + // Because we've pulled all the bytes from the RX FIFO, we should be + // able to observe the EOT condition here. + if !self.spi.check_eot() { + panic!(); } - } + self.spi.clear_eot(); - // Because we've pulled all the bytes from the RX FIFO, we should be - // able to observe the EOT condition here. - if !self.spi.check_eot() { - panic!(); + // Wrap up the transfer and restore things to a reasonable + // state. + self.spi.end(); } - self.spi.clear_eot(); - - // Wrap up the transfer and restore things to a reasonable - // state. - self.spi.end(); // Deassert (set) CS, if we asserted it in the first place. if !cs_override { From 7bd5c3d2a41e2819257b999b32b6093acf1b2ab1 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 30 Mar 2024 10:36:35 -0700 Subject: [PATCH 02/26] wip --- Cargo.lock | 3 - drv/ksz8463/src/lib.rs | 12 +- drv/spi-api/Cargo.toml | 2 - drv/spi-api/src/lib.rs | 90 ++--- drv/stm32h7-spi-server-core/src/lib.rs | 472 ++++++++++++------------- drv/stm32h7-spi-server/src/main.rs | 10 +- drv/stm32h7-spi/src/lib.rs | 17 + drv/vsc-err/Cargo.toml | 1 - drv/vsc-err/src/lib.rs | 8 - idl/spi.idol | 6 +- 10 files changed, 283 insertions(+), 338 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e77dbf8fc1..df610976eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1676,8 +1676,6 @@ dependencies = [ "build-spi", "build-util", "counters", - "derive-idol-err", - "gateway-messages", "hubpack", "idol", "idol-runtime", @@ -5450,7 +5448,6 @@ name = "vsc-err" version = "0.1.0" dependencies = [ "counters", - "drv-spi-api", "idol-runtime", ] diff --git a/drv/ksz8463/src/lib.rs b/drv/ksz8463/src/lib.rs index ae47b6eead..02bfef88f0 100644 --- a/drv/ksz8463/src/lib.rs +++ b/drv/ksz8463/src/lib.rs @@ -3,7 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. #![no_std] -use drv_spi_api::{SpiDevice, SpiError, SpiServer}; +use drv_spi_api::{SpiDevice, SpiServer}; use ringbuf::*; use userlib::hl::sleep_for; @@ -14,19 +14,13 @@ pub use registers::{MIBCounter, Register}; #[derive(Copy, Clone, Eq, PartialEq, counters::Count)] pub enum Error { - SpiError(#[count(children)] SpiError), + SpiServerDied, WrongChipId(u16), } -impl From for Error { - fn from(s: SpiError) -> Self { - Self::SpiError(s) - } -} - impl From for Error { fn from(_: idol_runtime::ServerDeath) -> Self { - Self::SpiError(SpiError::TaskRestarted) + Self::SpiServerDied } } diff --git a/drv/spi-api/Cargo.toml b/drv/spi-api/Cargo.toml index da063a4526..12d8d6a1cc 100644 --- a/drv/spi-api/Cargo.toml +++ b/drv/spi-api/Cargo.toml @@ -7,7 +7,6 @@ edition = "2021" [dependencies] -gateway-messages.workspace = true hubpack.workspace = true idol-runtime.workspace = true num-traits.workspace = true @@ -15,7 +14,6 @@ serde.workspace = true zerocopy.workspace = true counters = { path = "../../lib/counters" } -derive-idol-err.path = "../../lib/derive-idol-err" userlib.path = "../../sys/userlib" # This section is here to discourage RLS/rust-analyzer from doing test builds, diff --git a/drv/spi-api/src/lib.rs b/drv/spi-api/src/lib.rs index 5ac81bd488..ff957adc73 100644 --- a/drv/spi-api/src/lib.rs +++ b/drv/spi-api/src/lib.rs @@ -6,52 +6,9 @@ #![no_std] -// For Idol-generated code that fully qualifies error type names. -use crate as drv_spi_api; -use derive_idol_err::IdolError; -use gateway_messages::SpiError as GwSpiError; -use hubpack::SerializedSize; -use serde::{Deserialize, Serialize}; +use idol_runtime::ServerDeath; use userlib::*; -#[derive( - Copy, - Clone, - Debug, - FromPrimitive, - Eq, - PartialEq, - IdolError, - SerializedSize, - Serialize, - Deserialize, - counters::Count, -)] -#[repr(u32)] -pub enum SpiError { - /// Transfer size is 0 or exceeds maximum - BadTransferSize = 1, - - /// Server restarted - #[idol(server_death)] - TaskRestarted = 4, -} - -impl From for SpiError { - fn from(_: idol_runtime::ServerDeath) -> Self { - SpiError::TaskRestarted - } -} - -impl From for GwSpiError { - fn from(value: SpiError) -> Self { - match value { - SpiError::BadTransferSize => Self::BadTransferSize, - SpiError::TaskRestarted => Self::TaskRestarted, - } - } -} - #[derive( Copy, Clone, Debug, Eq, PartialEq, zerocopy::AsBytes, FromPrimitive, )] @@ -81,11 +38,15 @@ pub trait SpiServer { device_index: u8, src: &[u8], dest: &mut [u8], - ) -> Result<(), SpiError>; + ) -> Result<(), ServerDeath>; - fn write(&self, device_index: u8, src: &[u8]) -> Result<(), SpiError>; + fn write(&self, device_index: u8, src: &[u8]) -> Result<(), ServerDeath>; - fn read(&self, device_index: u8, dest: &mut [u8]) -> Result<(), SpiError>; + fn read( + &self, + device_index: u8, + dest: &mut [u8], + ) -> Result<(), ServerDeath>; /// Variant of `lock` that returns a resource management object that, when /// dropped, will issue `release`. This makes it much easier to do fallible @@ -96,7 +57,7 @@ pub trait SpiServer { &self, device_index: u8, assert_cs: CsState, - ) -> Result, idol_runtime::ServerDeath> + ) -> Result, ServerDeath> where Self: Sized, { @@ -119,9 +80,9 @@ pub trait SpiServer { &self, device_index: u8, cs_state: CsState, - ) -> Result<(), idol_runtime::ServerDeath>; + ) -> Result<(), ServerDeath>; - fn release(&self) -> Result<(), idol_runtime::ServerDeath>; + fn release(&self) -> Result<(), ServerDeath>; } impl SpiServer for Spi { @@ -130,14 +91,18 @@ impl SpiServer for Spi { device_index: u8, src: &[u8], dest: &mut [u8], - ) -> Result<(), SpiError> { + ) -> Result<(), ServerDeath> { Spi::exchange(self, device_index, src, dest) } - fn write(&self, device_index: u8, src: &[u8]) -> Result<(), SpiError> { + fn write(&self, device_index: u8, src: &[u8]) -> Result<(), ServerDeath> { Spi::write(self, device_index, src) } - fn read(&self, device_index: u8, dest: &mut [u8]) -> Result<(), SpiError> { + fn read( + &self, + device_index: u8, + dest: &mut [u8], + ) -> Result<(), ServerDeath> { Spi::read(self, device_index, dest) } @@ -145,11 +110,11 @@ impl SpiServer for Spi { &self, device_index: u8, cs_state: CsState, - ) -> Result<(), idol_runtime::ServerDeath> { + ) -> Result<(), ServerDeath> { Spi::lock(self, device_index, cs_state) } - fn release(&self) -> Result<(), idol_runtime::ServerDeath> { + fn release(&self) -> Result<(), ServerDeath> { Spi::release(self) } } @@ -184,7 +149,7 @@ impl SpiDevice { &self, source: &[u8], sink: &mut [u8], - ) -> Result<(), SpiError> { + ) -> Result<(), ServerDeath> { self.server.exchange(self.device_index, source, sink) } @@ -192,7 +157,7 @@ impl SpiDevice { /// /// If the controller is not locked, this will assert CS before driving the /// clock and release it after. - pub fn write(&self, source: &[u8]) -> Result<(), SpiError> { + pub fn write(&self, source: &[u8]) -> Result<(), ServerDeath> { self.server.write(self.device_index, source) } @@ -200,7 +165,7 @@ impl SpiDevice { /// /// If the controller is not locked, this will assert CS before driving the /// clock and release it after. - pub fn read(&self, dest: &mut [u8]) -> Result<(), SpiError> { + pub fn read(&self, dest: &mut [u8]) -> Result<(), ServerDeath> { self.server.read(self.device_index, dest) } @@ -223,17 +188,14 @@ impl SpiDevice { /// /// If your task tries to lock two different `SpiDevice`s at once, the /// second one to attempt will get `BadDevice`. - pub fn lock( - &self, - assert_cs: CsState, - ) -> Result<(), idol_runtime::ServerDeath> { + pub fn lock(&self, assert_cs: CsState) -> Result<(), ServerDeath> { self.server.lock(self.device_index, assert_cs) } /// Releases a previous lock on the SPI controller (by your task). /// /// This will also deassert CS, if you had overridden it. - pub fn release(&self) -> Result<(), idol_runtime::ServerDeath> { + pub fn release(&self) -> Result<(), ServerDeath> { self.server.release() } @@ -245,7 +207,7 @@ impl SpiDevice { pub fn lock_auto( &self, assert_cs: CsState, - ) -> Result, idol_runtime::ServerDeath> { + ) -> Result, ServerDeath> { self.server.lock_auto(self.device_index, assert_cs) } } diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index 90e331b4ae..c0b7538d67 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -69,7 +69,8 @@ enum Trace { op: SpiOperation, src_len: u16, dst_len: u16, - iter: usize, + src_reload_len: Option, + dst_reload_len: Option, }, Tx(u8), Rx(u8), @@ -103,19 +104,6 @@ pub enum TransferError { #[derive(Copy, Clone, Eq, PartialEq)] pub struct LockError(()); -impl From for RequestError { - fn from(value: TransferError) -> Self { - match value { - TransferError::BadTransferSize => { - RequestError::Runtime(SpiError::BadTransferSize) - } - TransferError::BadDevice => { - RequestError::Fail(ClientError::BadMessageContents) - } - } - } -} - impl From for RequestError { fn from(_: LockError) -> RequestError { RequestError::Fail(ClientError::BadMessageContents) @@ -345,232 +333,230 @@ impl SpiServerCore { return Err(TransferError::BadTransferSize); } - let mut iter = 0; - loop { - // Get the required transfer lengths in the src and dest directions. - // - // If the size is greater than `u16::MAX`, transfer `u16::MAX` bytes and - // then keep looping. - let src_len = - tx.as_ref().map(|tx| tx.remaining_size()).unwrap_or(0) as u16; - let dst_len = - rx.as_ref().map(|rx| rx.remaining_size()).unwrap_or(0) as u16; - let overall_len = src_len.max(dst_len); - - // If we've consumed all the remaining buffer in both directions, - // we're done! - if overall_len == 0 { - break; - } + // Get the required transfer lengths in the src and dest directions. + let src_len = tx.as_ref().map(|tx| tx.remaining_size()).unwrap_or(0); + let dst_len = rx.as_ref().map(|rx| rx.remaining_size()).unwrap_or(0); + let src_reload_len = if src_len > u16::MAX { + Some(src_len - u16::MAX) + } else { + None + }; + let dst_reload_len = if dst > u16::MAX { + Some(dst_len - u16::MAX) + } else { + None + }; + let overall_len = (src_len as u16).max(dst_len as u16); + let overall_reload_len = src_reload_len.max(dst_reload_len); + + // We have a reasonable-looking request containing reasonable-looking + // lease(s). This is our commit point. + ringbuf_entry!(Trace::Start { + op, + src_len, + dst_len, + src_reload_len, + dst_reload_len, + }); + iter += 1; + + // Switch the mux to the requested port. + let current_mux_index = self.current_mux_index.get(); + if device.mux_index != current_mux_index { + deactivate_mux_option( + &CONFIG.mux_options[current_mux_index], + &self.sys, + ); + activate_mux_option( + &CONFIG.mux_options[device.mux_index], + &self.sys, + &self.spi, + ); + // Remember this for later to avoid unnecessary + // switching. + self.current_mux_index.set(device.mux_index); + } - // We have a reasonable-looking request containing reasonable-looking - // lease(s). This is our commit point. - ringbuf_entry!(Trace::Start { - op, - src_len, - dst_len, - iter - }); - iter += 1; - - // Switch the mux to the requested port. - let current_mux_index = self.current_mux_index.get(); - if device.mux_index != current_mux_index { - deactivate_mux_option( - &CONFIG.mux_options[current_mux_index], - &self.sys, - ); - activate_mux_option( - &CONFIG.mux_options[device.mux_index], - &self.sys, - &self.spi, - ); - // Remember this for later to avoid unnecessary - // switching. - self.current_mux_index.set(device.mux_index); - } + // Make sure SPI is on. + self.spi.enable(overall_len, device.clock_divider); - // Make sure SPI is on. - // - // Due to driver limitations we will only move up to 64kiB - // per transaction. It would be worth lifting this - // limitation, maybe. Doing so would require managing data - // in 64kiB chunks (because the peripheral is 16-bit) and - // using the "reload" facility on the peripheral. - self.spi.enable(overall_len, device.clock_divider); - - // Load transfer count and start the state machine. At this - // point we _have_ to move the specified number of bytes - // through (or explicitly cancel, but we don't). - self.spi.start(); - - // As you might expect, we will work from byte 0 to the end - // of each buffer. There are two complications: - // - // 1. Transmit and receive can be at different positions -- - // transmit will tend to lead receive, because the SPI - // unit contains FIFOs. - // - // 2. We're only keeping track of position in the buffers - // we're using: both tx and rx are `Option`. - // - // The BufReader/Writer types manage position tracking for us. - - // Enable interrupt on the conditions we're interested in. - self.spi.enable_transfer_interrupts(); - - self.spi.clear_eot(); - - // We're doing this! Check if we need to control CS. - let cs_override = self.lock_holder.get().is_some(); - if !cs_override { - for pin in device.cs { - self.sys.gpio_reset(*pin); - } - } + // If there are more than 64 KiB bytes to transfer, set the TSER value + // to cause the SPI peripheral to begin another transfer chunk when the + // current transfer completes. + if let Some(reload_len) = overall_reload_len { + self.spi.enable_reload(reload_len); + } - // We use this to exert backpressure on the TX state machine as the RX - // FIFO fills. Its initial value is the configured FIFO size, because - // the FIFO size varies on SPI blocks on the H7; it would be nice if we - // could read the configured FIFO size out of the block, but that does - // not appear to be possible. - // - // See reference manual table 409 for details. - let mut tx_permits = FIFO_DEPTH; - - // Track number of bytes sent and received. Sent bytes will lead - // received bytes. Received bytes indicate overall progress and - // completion. - let mut tx_count = 0; - let mut rx_count = 0; - - // The end of the exchange is signaled by rx_count reaching the - // overall_len. This is true even if the caller's rx lease is shorter or - // missing, because we have to pull bytes from the FIFO to avoid overrun - // conditions. - while rx_count < overall_len { - // At the end of this loop we're going to sleep if there's no - // obvious work to be done. Sleeping is not free, so, we only do it - // if this flag is set. (It defaults to set, we'll clear it if work - // appears below.) - let mut should_sleep = true; - - // TX engine. We continue moving bytes while these three conditions - // hold: - // - More bytes need to be sent. - // - Permits are available. - // - The TX FIFO has space. - while tx_count < overall_len - && tx_permits > 0 - && self.spi.can_tx_frame() - { - // The next byte to TX will come from the caller, if we haven't - // run off the end of their lease, or the fixed padding byte if - // we have. - let byte = if let Some(txbuf) = &mut tx { - if let Some(b) = txbuf.read() { - b - } else { - // We've hit the end of the lease. Stop checking. - tx = None; - 0 - } - } else { - 0 - }; + // Load transfer count and start the state machine. At this + // point we _have_ to move the specified number of bytes + // through (or explicitly cancel, but we don't). + self.spi.start(); + + // As you might expect, we will work from byte 0 to the end + // of each buffer. There are two complications: + // + // 1. Transmit and receive can be at different positions -- + // transmit will tend to lead receive, because the SPI + // unit contains FIFOs. + // + // 2. We're only keeping track of position in the buffers + // we're using: both tx and rx are `Option`. + // + // The BufReader/Writer types manage position tracking for us. - ringbuf_entry!(Trace::Tx(byte)); - self.spi.send8(byte); - tx_count += 1; + // Enable interrupt on the conditions we're interested in. + self.spi.enable_transfer_interrupts(); - // Consume one TX permit to make sure we don't overrun the RX - // fifo. - tx_permits -= 1; + self.spi.clear_eot(); + + // We're doing this! Check if we need to control CS. + let cs_override = self.lock_holder.get().is_some(); + if !cs_override { + for pin in device.cs { + self.sys.gpio_reset(*pin); + } + } - if tx_permits == 0 || tx_count == overall_len { - // We're either done, or we need to idle until the RX engine - // catches up. Either way, stop generating interrupts. - self.spi.disable_can_tx_interrupt(); + // We use this to exert backpressure on the TX state machine as the RX + // FIFO fills. Its initial value is the configured FIFO size, because + // the FIFO size varies on SPI blocks on the H7; it would be nice if we + // could read the configured FIFO size out of the block, but that does + // not appear to be possible. + // + // See reference manual table 409 for details. + let mut tx_permits = FIFO_DEPTH; + + // Track number of bytes sent and received. Sent bytes will lead + // received bytes. Received bytes indicate overall progress and + // completion. + let mut tx_count = 0; + let mut rx_count = 0; + + // The end of the exchange is signaled by rx_count reaching the + // overall_len. This is true even if the caller's rx lease is shorter or + // missing, because we have to pull bytes from the FIFO to avoid overrun + // conditions. + while rx_count < overall_len { + // At the end of this loop we're going to sleep if there's no + // obvious work to be done. Sleeping is not free, so, we only do it + // if this flag is set. (It defaults to set, we'll clear it if work + // appears below.) + let mut should_sleep = true; + + // TX engine. We continue moving bytes while these three conditions + // hold: + // - More bytes need to be sent. + // - Permits are available. + // - The TX FIFO has space. + while tx_count < overall_len + && tx_permits > 0 + && self.spi.can_tx_frame() + { + // The next byte to TX will come from the caller, if we haven't + // run off the end of their lease, or the fixed padding byte if + // we have. + let byte = if let Some(txbuf) = &mut tx { + if let Some(b) = txbuf.read() { + b + } else { + // We've hit the end of the lease. Stop checking. + tx = None; + 0 } + } else { + 0 + }; + + ringbuf_entry!(Trace::Tx(byte)); + self.spi.send8(byte); + tx_count += 1; + + // Consume one TX permit to make sure we don't overrun the RX + // fifo. + tx_permits -= 1; + + if tx_permits == 0 || tx_count == overall_len { + // We're either done, or we need to idle until the RX engine + // catches up. Either way, stop generating interrupts. + self.spi.disable_can_tx_interrupt(); + } + + // We don't adjust should_sleep in the TX engine because, if we + // leave this loop, we've done all the TX work we can -- and + // we're about to check for RX work unconditionally below. So, + // from the perspective of the TX engine, should_sleep is always + // true at this point, and the RX engine gets to make the final + // decision. + } - // We don't adjust should_sleep in the TX engine because, if we - // leave this loop, we've done all the TX work we can -- and - // we're about to check for RX work unconditionally below. So, - // from the perspective of the TX engine, should_sleep is always - // true at this point, and the RX engine gets to make the final - // decision. + // Drain bytes from the RX FIFO. + while self.spi.can_rx_byte() { + // We didn't check rx_count < overall_len above because, if we + // got to that point, it would mean the SPI hardware gave us + // more bytes than we sent. This would be bad. And so, we'll + // detect that condition aggressively: + if rx_count >= overall_len { + panic!(); } - // Drain bytes from the RX FIFO. - while self.spi.can_rx_byte() { - // We didn't check rx_count < overall_len above because, if we - // got to that point, it would mean the SPI hardware gave us - // more bytes than we sent. This would be bad. And so, we'll - // detect that condition aggressively: - if rx_count >= overall_len { - panic!(); - } + // Pull byte from RX FIFO. + let b = self.spi.recv8(); + ringbuf_entry!(Trace::Rx(b)); + rx_count += 1; - // Pull byte from RX FIFO. - let b = self.spi.recv8(); - ringbuf_entry!(Trace::Rx(b)); - rx_count += 1; - - // Allow another byte to be inserted in the TX FIFO. - tx_permits += 1; - - // Deposit the byte if we're still within the bounds of the - // caller's incoming lease. - if let Some(rx_reader) = &mut rx { - if rx_reader.write(b).is_err() { - // We're off the end. Stop checking. - rx = None; - } - } + // Allow another byte to be inserted in the TX FIFO. + tx_permits += 1; - // By releasing a TX permit, we might have unblocked the TX - // engine. We can detect this when tx_permits goes 0->1. If this - // occurs, we should turn its interrupt back on, but only if - // it's still working. - if tx_permits == 1 && tx_count < overall_len { - self.spi.enable_can_tx_interrupt(); + // Deposit the byte if we're still within the bounds of the + // caller's incoming lease. + if let Some(rx_reader) = &mut rx { + if rx_reader.write(b).is_err() { + // We're off the end. Stop checking. + rx = None; } + } - // We've done some work, which means some time has elapsed, - // which means it's possible that room in the TX FIFO has opened - // up. So, let's not sleep. - should_sleep = false; + // By releasing a TX permit, we might have unblocked the TX + // engine. We can detect this when tx_permits goes 0->1. If this + // occurs, we should turn its interrupt back on, but only if + // it's still working. + if tx_permits == 1 && tx_count < overall_len { + self.spi.enable_can_tx_interrupt(); } - if should_sleep { - ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); + // We've done some work, which means some time has elapsed, + // which means it's possible that room in the TX FIFO has opened + // up. So, let's not sleep. + should_sleep = false; + } - if self.spi.check_overrun() { - panic!(); - } + if should_sleep { + ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); - // Allow the controller interrupt to post to our - // notification set. - sys_irq_control(self.irq_mask, true); - // Wait for our notification set to get, well, set. We ignore - // the result of this because an error would mean the kernel - // violated the ABI, which we can't usefully respond to. - let _ = - sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); + if self.spi.check_overrun() { + panic!(); } - } - // Because we've pulled all the bytes from the RX FIFO, we should be - // able to observe the EOT condition here. - if !self.spi.check_eot() { - panic!(); + // Allow the controller interrupt to post to our + // notification set. + sys_irq_control(self.irq_mask, true); + // Wait for our notification set to get, well, set. We ignore + // the result of this because an error would mean the kernel + // violated the ABI, which we can't usefully respond to. + let _ = sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); } - self.spi.clear_eot(); + } - // Wrap up the transfer and restore things to a reasonable - // state. - self.spi.end(); + // Because we've pulled all the bytes from the RX FIFO, we should be + // able to observe the EOT condition here. + if !self.spi.check_eot() { + panic!(); } + self.spi.clear_eot(); + + // Wrap up the transfer and restore things to a reasonable + // state. + self.spi.end(); // Deassert (set) CS, if we asserted it in the first place. if !cs_override { @@ -748,36 +734,36 @@ impl SpiServer for SpiServerCore { device_index: u8, src: &[u8], dest: &mut [u8], - ) -> Result<(), SpiError> { - SpiServerCore::exchange(self, device_index, src, dest).map_err(|e| { - match e { - // If the SPI server was in a remote task, this case would - // return a reply-fault; therefore, panicking the task when the - // SPI driver is local to that task is appropriate. - TransferError::BadDevice => panic!(), - TransferError::BadTransferSize => SpiError::BadTransferSize, - } - }) + ) -> Result<(), idol_runtime::ServerDeath> { + // If the SPI server were running in a remote task, errors returned here + // would be converted into reply-faults. Since it's running locally, + // panicking here is equivalent. + SpiServerCore::exchange(self, device_index, src, dest).unwrap_lite(); + Ok(()) } - fn write(&self, device_index: u8, src: &[u8]) -> Result<(), SpiError> { - SpiServerCore::write(self, device_index, src).map_err(|e| match e { - // If the SPI server was in a remote task, this case would - // return a reply-fault; therefore, panicking the task when the - // SPI driver is local to that task is appropriate. - TransferError::BadDevice => panic!(), - TransferError::BadTransferSize => SpiError::BadTransferSize, - }) + fn write( + &self, + device_index: u8, + src: &[u8], + ) -> Result<(), idol_runtime::ServerDeath> { + // If the SPI server were running in a remote task, errors returned here + // would be converted into reply-faults. Since it's running locally, + // panicking here is equivalent. + SpiServerCore::write(self, device_index, src).unwrap_lite(); + Ok(()) } - fn read(&self, device_index: u8, dest: &mut [u8]) -> Result<(), SpiError> { - SpiServerCore::read(self, device_index, dest).map_err(|e| match e { - // If the SPI server was in a remote task, this case would - // return a reply-fault; therefore, panicking the task when the - // SPI driver is local to that task is appropriate. - TransferError::BadDevice => panic!(), - TransferError::BadTransferSize => SpiError::BadTransferSize, - }) + fn read( + &self, + device_index: u8, + dest: &mut [u8], + ) -> Result<(), idol_runtime::ServerDeath> { + // If the SPI server were running in a remote task, errors returned here + // would be converted into reply-faults. Since it's running locally, + // panicking here is equivalent. + SpiServerCore::read(self, device_index, dest).unwrap_lite(); + Ok(()) } fn lock( diff --git a/drv/stm32h7-spi-server/src/main.rs b/drv/stm32h7-spi-server/src/main.rs index 83a8df3583..da2c8075cc 100644 --- a/drv/stm32h7-spi-server/src/main.rs +++ b/drv/stm32h7-spi-server/src/main.rs @@ -61,7 +61,7 @@ impl InOrderSpiImpl for ServerImpl { _: &RecvMessage, device_index: u8, dest: LenLimit, 65535>, - ) -> Result<(), RequestError> { + ) -> Result<(), RequestError> { self.core .read::>( device_index, @@ -75,13 +75,13 @@ impl InOrderSpiImpl for ServerImpl { _: &RecvMessage, device_index: u8, src: LenLimit, 65535>, - ) -> Result<(), RequestError> { + ) -> Result<(), RequestError> { self.core .write::>( device_index, src.into_inner().into(), ) - .map_err(RequestError::from) + .map_err(|_| idol_runtime::ClientError::BadMessageContents.fail()) } fn exchange( @@ -90,14 +90,14 @@ impl InOrderSpiImpl for ServerImpl { device_index: u8, src: LenLimit, 65535>, dest: LenLimit, 65535>, - ) -> Result<(), RequestError> { + ) -> Result<(), RequestError> { self.core .exchange::, LeaseBufWriter<_, BUFSIZ>>( device_index, src.into_inner().into(), dest.into_inner().into(), ) - .map_err(RequestError::from) + .map_err(|_| idol_runtime::ClientError::BadMessageContents.fail()) } fn lock( diff --git a/drv/stm32h7-spi/src/lib.rs b/drv/stm32h7-spi/src/lib.rs index 8a720bd42a..c64ccfe60c 100644 --- a/drv/stm32h7-spi/src/lib.rs +++ b/drv/stm32h7-spi/src/lib.rs @@ -117,6 +117,23 @@ impl Spi { self.reg.cr1.modify(|_, w| w.spe().set_bit()); } + /// Set an additional 16-bit number of bytes to transfer when the current + /// value in `TSIZE` has been transferred. + /// + /// Per the manual: + /// > When the number of data programmed into `TSIZE`` is transacted and if + /// > `TSER`` contains a non-zero value, the content of `TSER`` is copied + /// > into `TSIZE`, and `TSER` value is cleared automatically. + /// > The transaction is then extended by a number of data corresponding to + /// > the value reloaded into `TSIZE``. The `EOT` event is not raised in + /// > this case as the transaction continues. + /// > --- RM0433 Rev 8, p. 2177 + pub fn enable_reload(&self, tser: u16) { + self.reg.cr2.modify(|_, w| w.tser().bits(tser)); + // Let's receive an interrupt when the reloaded transfer begins. + self.reg.ier.modify(|_, w| w.tserfie().set_bit()) + } + pub fn start(&self) { self.reg.cr1.modify(|_, w| w.cstart().set_bit()); // Clear EOT flag diff --git a/drv/vsc-err/Cargo.toml b/drv/vsc-err/Cargo.toml index 366182a862..54444ddf37 100644 --- a/drv/vsc-err/Cargo.toml +++ b/drv/vsc-err/Cargo.toml @@ -5,7 +5,6 @@ edition = "2021" [dependencies] counters = { path = "../../lib/counters" } -drv-spi-api = { path = "../spi-api" } idol-runtime = { workspace = true } # This section is here to discourage RLS/rust-analyzer from doing test builds, diff --git a/drv/vsc-err/src/lib.rs b/drv/vsc-err/src/lib.rs index a20a739732..9c82b5ba0b 100644 --- a/drv/vsc-err/src/lib.rs +++ b/drv/vsc-err/src/lib.rs @@ -9,12 +9,10 @@ #![no_std] -use drv_spi_api::SpiError; use idol_runtime::ServerDeath; #[derive(Copy, Clone, Eq, PartialEq, counters::Count)] pub enum VscError { - SpiError(#[count(children)] SpiError), ServerDied, /// Error code produced by a proxy device handling PHY register /// reads/writes. @@ -100,12 +98,6 @@ pub enum VscError { OutOfRange, } -impl From for VscError { - fn from(s: SpiError) -> Self { - Self::SpiError(s) - } -} - impl From for VscError { fn from(_s: ServerDeath) -> Self { Self::ServerDied diff --git a/idl/spi.idol b/idl/spi.idol index a601b66e1a..5bdb4a3451 100644 --- a/idl/spi.idol +++ b/idl/spi.idol @@ -14,7 +14,7 @@ Interface( }, reply: Result( ok: "()", - err: CLike("drv_spi_api::SpiError"), + err: ServerDeath, ), ), "write": ( @@ -27,7 +27,7 @@ Interface( }, reply: Result( ok: "()", - err: CLike("drv_spi_api::SpiError"), + err: ServerDeath, ), ), "exchange": ( @@ -41,7 +41,7 @@ Interface( }, reply: Result( ok: "()", - err: CLike("drv_spi_api::SpiError"), + err: ServerDeath, ), ), "lock": ( From 67e2320dcbe8fc9a4b2b4dc896f9fe5571448423 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 2 Apr 2024 09:51:25 -0700 Subject: [PATCH 03/26] WIP TSER --- drv/stm32h7-spi-server-core/src/lib.rs | 319 ++++++++++++++----------- drv/stm32h7-spi/src/lib.rs | 29 ++- 2 files changed, 202 insertions(+), 146 deletions(-) diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index c0b7538d67..3071d80abf 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -32,7 +32,7 @@ use drv_stm32h7_spi as spi_core; use drv_stm32xx_sys_api as sys_api; use sys_api::PinSet; -use core::{cell::Cell, convert::Infallible}; +use core::{cell::Cell, cmp, convert::Infallible}; //////////////////////////////////////////////////////////////////////////////// @@ -65,12 +65,16 @@ pub struct SpiServerCore { #[derive(Copy, Clone, PartialEq, counters::Count)] enum Trace { Start { + #[count(children)] + op: SpiOperation, + src_total_len: u32, + dst_total_len: u32, + }, + Reload { #[count(children)] op: SpiOperation, src_len: u16, dst_len: u16, - src_reload_len: Option, - dst_reload_len: Option, }, Tx(u8), Rx(u8), @@ -325,40 +329,28 @@ impl SpiServerCore { panic!(); } + let mut src_total_len = + tx.as_ref().map(|tx| tx.remaining_size()).unwrap_or(0) as u32; + let mut dst_total_len = + rx.as_ref().map(|rx| rx.remaining_size()).unwrap_or(0) as u32; + // Zero-byte SPI transactions don't make sense and we'll // decline them. - let tx_len = tx.as_ref().map(BufWrite::remaining_size).unwrap_or(0); - let rx_len = rx.as_ref().map(BufReader::remaining_size).unwrap_or(0); - if tx_len + rx_len == 0 { + if src_total_len + dst_total_len == 0 { + // TODO(eliza): perhaps we should just return `Ok` here, so that we + // can completely remove the notion of a "bad transfer size" error? return Err(TransferError::BadTransferSize); } // Get the required transfer lengths in the src and dest directions. - let src_len = tx.as_ref().map(|tx| tx.remaining_size()).unwrap_or(0); - let dst_len = rx.as_ref().map(|rx| rx.remaining_size()).unwrap_or(0); - let src_reload_len = if src_len > u16::MAX { - Some(src_len - u16::MAX) - } else { - None - }; - let dst_reload_len = if dst > u16::MAX { - Some(dst_len - u16::MAX) - } else { - None - }; - let overall_len = (src_len as u16).max(dst_len as u16); - let overall_reload_len = src_reload_len.max(dst_reload_len); // We have a reasonable-looking request containing reasonable-looking // lease(s). This is our commit point. ringbuf_entry!(Trace::Start { op, - src_len, - dst_len, - src_reload_len, - dst_reload_len, + src_total_len, + dst_total_len, }); - iter += 1; // Switch the mux to the requested port. let current_mux_index = self.current_mux_index.get(); @@ -377,16 +369,17 @@ impl SpiServerCore { self.current_mux_index.set(device.mux_index); } + let mut overall_len = + cmp::max(src_total_len as u16, dst_total_len as u16); + // Subtract the first 16-bit transfer size from the total. + src_total_len = + src_total_len.saturating_sub((src_total_len as u16) as u32); + dst_total_len = + dst_total_len.saturating_sub((dst_total_len as u16) as u32); + // Make sure SPI is on. self.spi.enable(overall_len, device.clock_divider); - // If there are more than 64 KiB bytes to transfer, set the TSER value - // to cause the SPI peripheral to begin another transfer chunk when the - // current transfer completes. - if let Some(reload_len) = overall_reload_len { - self.spi.enable_reload(reload_len); - } - // Load transfer count and start the state machine. At this // point we _have_ to move the specified number of bytes // through (or explicitly cancel, but we don't). @@ -417,133 +410,173 @@ impl SpiServerCore { } } - // We use this to exert backpressure on the TX state machine as the RX - // FIFO fills. Its initial value is the configured FIFO size, because - // the FIFO size varies on SPI blocks on the H7; it would be nice if we - // could read the configured FIFO size out of the block, but that does - // not appear to be possible. - // - // See reference manual table 409 for details. - let mut tx_permits = FIFO_DEPTH; - - // Track number of bytes sent and received. Sent bytes will lead - // received bytes. Received bytes indicate overall progress and - // completion. - let mut tx_count = 0; - let mut rx_count = 0; - - // The end of the exchange is signaled by rx_count reaching the - // overall_len. This is true even if the caller's rx lease is shorter or - // missing, because we have to pull bytes from the FIFO to avoid overrun - // conditions. - while rx_count < overall_len { - // At the end of this loop we're going to sleep if there's no - // obvious work to be done. Sleeping is not free, so, we only do it - // if this flag is set. (It defaults to set, we'll clear it if work - // appears below.) - let mut should_sleep = true; - - // TX engine. We continue moving bytes while these three conditions - // hold: - // - More bytes need to be sent. - // - Permits are available. - // - The TX FIFO has space. - while tx_count < overall_len - && tx_permits > 0 - && self.spi.can_tx_frame() - { - // The next byte to TX will come from the caller, if we haven't - // run off the end of their lease, or the fixed padding byte if - // we have. - let byte = if let Some(txbuf) = &mut tx { - if let Some(b) = txbuf.read() { - b + // If the transfer size exceeds `u16::MAX`, we'll loop here multiple + // times, using the `TSER` register to load the next chunk. + loop { + // Are there more bytes to send/receive? If so, we'll need to set + // the `TSER` register to start the next 16-bit transfer when this + // one finishes. + if src_total_len > 0 || dst_total_len > 0 { + let src_chunk = src_total_len as u16; + let dst_chunk = dst_total_len as u16; + let reload_size = cmp::max(src_chunk, dst_chunk); + self.spi.enable_reload(reload_size); + + src_total_len = src_total_len.saturating_sub(src_chunk as u32); + dst_total_len -= dst_total_len.saturating_sub(dst_chunk as u32); + } + + // We use this to exert backpressure on the TX state machine as the RX + // FIFO fills. Its initial value is the configured FIFO size, because + // the FIFO size varies on SPI blocks on the H7; it would be nice if we + // could read the configured FIFO size out of the block, but that does + // not appear to be possible. + // + // See reference manual table 409 for details. + let mut tx_permits = FIFO_DEPTH; + + // Track number of bytes sent and received. Sent bytes will lead + // received bytes. Received bytes indicate overall progress and + // completion. + let mut tx_count = 0; + let mut rx_count = 0; + + // The end of the exchange is signaled by rx_count reaching the + // overall_len. This is true even if the caller's rx lease is shorter or + // missing, because we have to pull bytes from the FIFO to avoid overrun + // conditions. + while rx_count < overall_len { + // At the end of this loop we're going to sleep if there's no + // obvious work to be done. Sleeping is not free, so, we only do it + // if this flag is set. (It defaults to set, we'll clear it if work + // appears below.) + let mut should_sleep = true; + + // TX engine. We continue moving bytes while these three conditions + // hold: + // - More bytes need to be sent. + // - Permits are available. + // - The TX FIFO has space. + while tx_count < overall_len + && tx_permits > 0 + && self.spi.can_tx_frame() + { + // The next byte to TX will come from the caller, if we haven't + // run off the end of their lease, or the fixed padding byte if + // we have. + let byte = if let Some(txbuf) = &mut tx { + if let Some(b) = txbuf.read() { + b + } else { + // We've hit the end of the lease. Stop checking. + tx = None; + 0 + } } else { - // We've hit the end of the lease. Stop checking. - tx = None; 0 - } - } else { - 0 - }; - - ringbuf_entry!(Trace::Tx(byte)); - self.spi.send8(byte); - tx_count += 1; - - // Consume one TX permit to make sure we don't overrun the RX - // fifo. - tx_permits -= 1; - - if tx_permits == 0 || tx_count == overall_len { - // We're either done, or we need to idle until the RX engine - // catches up. Either way, stop generating interrupts. - self.spi.disable_can_tx_interrupt(); - } + }; - // We don't adjust should_sleep in the TX engine because, if we - // leave this loop, we've done all the TX work we can -- and - // we're about to check for RX work unconditionally below. So, - // from the perspective of the TX engine, should_sleep is always - // true at this point, and the RX engine gets to make the final - // decision. - } + ringbuf_entry!(Trace::Tx(byte)); + self.spi.send8(byte); + tx_count += 1; + + // Consume one TX permit to make sure we don't overrun the RX + // fifo. + tx_permits -= 1; - // Drain bytes from the RX FIFO. - while self.spi.can_rx_byte() { - // We didn't check rx_count < overall_len above because, if we - // got to that point, it would mean the SPI hardware gave us - // more bytes than we sent. This would be bad. And so, we'll - // detect that condition aggressively: - if rx_count >= overall_len { - panic!(); + if tx_permits == 0 || tx_count == overall_len { + // We're either done, or we need to idle until the RX engine + // catches up. Either way, stop generating interrupts. + self.spi.disable_can_tx_interrupt(); + } + + // We don't adjust should_sleep in the TX engine because, if we + // leave this loop, we've done all the TX work we can -- and + // we're about to check for RX work unconditionally below. So, + // from the perspective of the TX engine, should_sleep is always + // true at this point, and the RX engine gets to make the final + // decision. } - // Pull byte from RX FIFO. - let b = self.spi.recv8(); - ringbuf_entry!(Trace::Rx(b)); - rx_count += 1; + // Drain bytes from the RX FIFO. + while self.spi.can_rx_byte() { + // We didn't check rx_count < overall_len above because, if we + // got to that point, it would mean the SPI hardware gave us + // more bytes than we sent. This would be bad. And so, we'll + // detect that condition aggressively: + if rx_count >= overall_len { + panic!(); + } - // Allow another byte to be inserted in the TX FIFO. - tx_permits += 1; + // Pull byte from RX FIFO. + let b = self.spi.recv8(); + ringbuf_entry!(Trace::Rx(b)); + rx_count += 1; + + // Allow another byte to be inserted in the TX FIFO. + tx_permits += 1; + + // Deposit the byte if we're still within the bounds of the + // caller's incoming lease. + if let Some(rx_reader) = &mut rx { + if rx_reader.write(b).is_err() { + // We're off the end. Stop checking. + rx = None; + } + } - // Deposit the byte if we're still within the bounds of the - // caller's incoming lease. - if let Some(rx_reader) = &mut rx { - if rx_reader.write(b).is_err() { - // We're off the end. Stop checking. - rx = None; + // By releasing a TX permit, we might have unblocked the TX + // engine. We can detect this when tx_permits goes 0->1. If this + // occurs, we should turn its interrupt back on, but only if + // it's still working. + if tx_permits == 1 && tx_count < overall_len { + self.spi.enable_can_tx_interrupt(); } - } - // By releasing a TX permit, we might have unblocked the TX - // engine. We can detect this when tx_permits goes 0->1. If this - // occurs, we should turn its interrupt back on, but only if - // it's still working. - if tx_permits == 1 && tx_count < overall_len { - self.spi.enable_can_tx_interrupt(); + // We've done some work, which means some time has elapsed, + // which means it's possible that room in the TX FIFO has opened + // up. So, let's not sleep. + should_sleep = false; } - // We've done some work, which means some time has elapsed, - // which means it's possible that room in the TX FIFO has opened - // up. So, let's not sleep. - should_sleep = false; - } + if should_sleep { + ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); - if should_sleep { - ringbuf_entry!(Trace::WaitISR(self.spi.read_status())); + if self.spi.check_overrun() { + panic!(); + } - if self.spi.check_overrun() { - panic!(); + // Allow the controller interrupt to post to our + // notification set. + sys_irq_control(self.irq_mask, true); + // Wait for our notification set to get, well, set. We ignore + // the result of this because an error would mean the kernel + // violated the ABI, which we can't usefully respond to. + let _ = + sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); } + } - // Allow the controller interrupt to post to our - // notification set. - sys_irq_control(self.irq_mask, true); - // Wait for our notification set to get, well, set. We ignore - // the result of this because an error would mean the kernel - // violated the ABI, which we can't usefully respond to. - let _ = sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); + // If there's more work to do, the TSERF interrupt should have fired. + if self.spi.check_tserf() { + // Another chunk was reloaded; update the remaining transfer + // size, clear the `TSERF` interrupt, and continue. + let src_len = src_total_len as u16; + let dst_len = dst_total_len as u16; + overall_len = + cmp::max(src_total_len as u16, dst_total_len as u16); + + // Clear the IRQ + self.spi.clear_tserf(); + + ringbuf_entry!(Trace::Reload { + op, + src_len, + dst_len, + }); + } else { + // Otherwise, we're done. + break; } } diff --git a/drv/stm32h7-spi/src/lib.rs b/drv/stm32h7-spi/src/lib.rs index c64ccfe60c..fc3c83f282 100644 --- a/drv/stm32h7-spi/src/lib.rs +++ b/drv/stm32h7-spi/src/lib.rs @@ -121,16 +121,29 @@ impl Spi { /// value in `TSIZE` has been transferred. /// /// Per the manual: - /// > When the number of data programmed into `TSIZE`` is transacted and if - /// > `TSER`` contains a non-zero value, the content of `TSER`` is copied + /// > When the number of data programmed into `TSIZE` is transacted and if + /// > `TSER` contains a non-zero value, the content of `TSER` is copied /// > into `TSIZE`, and `TSER` value is cleared automatically. /// > The transaction is then extended by a number of data corresponding to /// > the value reloaded into `TSIZE``. The `EOT` event is not raised in /// > this case as the transaction continues. /// > --- RM0433 Rev 8, p. 2177 pub fn enable_reload(&self, tser: u16) { - self.reg.cr2.modify(|_, w| w.tser().bits(tser)); + self.reg.cr2.modify(|r, w| { + // This is (probably) a PAC bug: the `stm32h7` PAC doesn't let us write + // to the TSER field in CR2, only read from it. But, the STM32 manual + // (RM0433) specifically describes writing to TSER in order to extend a + // transfer. So, let's do that... + // + // If the PAC exposed writing to TSER, this should just be + // self.reg.cr2.modify(|_, w| w.tser().bits(tser); + let bits = r.tsize().bits() as u32 | ((tser as u32) << 16); + unsafe { w.bits(bits) } + }); // Let's receive an interrupt when the reloaded transfer begins. + // N.B. This is separate from `enable_transfer_interrupts` because we + // only care about receiving the TSERFIE interrupt when the TSER + // register is in use. self.reg.ier.modify(|_, w| w.tserfie().set_bit()) } @@ -286,6 +299,16 @@ impl Spi { self.reg.ifcr.write(|w| w.eotc().set_bit()); } + /// Returns `true` if the `TSER` register has reloaded the next transfer + /// chunk size. + pub fn check_tserf(&self) -> bool { + self.reg.sr.read().tserf().is_loaded() + } + + pub fn clear_tserf(&self) { + self.reg.ifcr.write(|w| w.tserfc().set_bit()); + } + pub fn read_status(&self) -> u32 { self.reg.sr.read().bits() } From 2bcbcf1876a270de0ac4552e79ed89d42a9c9ed4 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 2 Apr 2024 10:03:23 -0700 Subject: [PATCH 04/26] wip2 --- drv/stm32h7-spi-server-core/src/lib.rs | 6 +++--- drv/stm32h7-spi-server/src/main.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index 3071d80abf..f240a7b789 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -197,7 +197,7 @@ impl SpiServerCore { &self, device_index: u8, dest: BufWrite, - ) -> Result<(), TransferError> { + ) -> Result<(), Infallible> { self.ready_writey::<&[u8], _>( SpiOperation::read, device_index, @@ -210,7 +210,7 @@ impl SpiServerCore { &self, device_index: u8, src: BufRead, - ) -> Result<(), TransferError> { + ) -> Result<(), Infallible> { self.ready_writey::<_, &mut [u8]>( SpiOperation::write, device_index, @@ -224,7 +224,7 @@ impl SpiServerCore { device_index: u8, src: BufRead, dest: BufWrite, - ) -> Result<(), TransferError> { + ) -> Result<(), Infallible> { self.ready_writey( SpiOperation::exchange, device_index, diff --git a/drv/stm32h7-spi-server/src/main.rs b/drv/stm32h7-spi-server/src/main.rs index da2c8075cc..af62ec4e95 100644 --- a/drv/stm32h7-spi-server/src/main.rs +++ b/drv/stm32h7-spi-server/src/main.rs @@ -67,7 +67,7 @@ impl InOrderSpiImpl for ServerImpl { device_index, dest.into_inner().into(), ) - .map_err(RequestError::from) + .map_err(|_| idol_runtime::ClientError::BadMessageContents.fail()) } fn write( From 433297704b83368c1139f17bc3f3c6197ac06ee1 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Fri, 29 Mar 2024 12:44:08 -0700 Subject: [PATCH 05/26] build: re-enable task packing. Now that release 7 is closer to finalized and we've gained some confidence in the kernel fixes for #1672, it would be nice to regain the benefits of task packing and stop fretting so much about fragmentation. --- app/demo-stm32g0-nucleo/app-g070.toml | 2 +- app/demo-stm32h7-nucleo/app-h753.toml | 2 +- app/donglet/app-g031-i2c.toml | 2 +- app/donglet/app-g031.toml | 2 +- app/sidecar/base.toml | 2 +- build/xtask/src/dist.rs | 5 ++--- test/tests-stm32g0/app-g070.toml | 2 +- 7 files changed, 8 insertions(+), 9 deletions(-) diff --git a/app/demo-stm32g0-nucleo/app-g070.toml b/app/demo-stm32g0-nucleo/app-g070.toml index 8918b93ad9..b4e1f04fff 100644 --- a/app/demo-stm32g0-nucleo/app-g070.toml +++ b/app/demo-stm32g0-nucleo/app-g070.toml @@ -7,7 +7,7 @@ stacksize = 944 [kernel] name = "demo-stm32g0-nucleo" -requires = {flash = 19040, ram = 1632} +requires = {flash = 19264, ram = 1632} features = ["g070"] stacksize = 640 diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index 8c2f9ea51e..0fd3a5a4db 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -6,7 +6,7 @@ stacksize = 896 [kernel] name = "demo-stm32h7-nucleo" -requires = {flash = 24544, ram = 5120} +requires = {flash = 24704, ram = 5120} features = ["h753", "dump"] [tasks.jefe] diff --git a/app/donglet/app-g031-i2c.toml b/app/donglet/app-g031-i2c.toml index e9ce2aa035..218a6c2b3d 100644 --- a/app/donglet/app-g031-i2c.toml +++ b/app/donglet/app-g031-i2c.toml @@ -6,7 +6,7 @@ board = "donglet-g031" [kernel] name = "app-donglet" -requires = {flash = 18752, ram = 1616} +requires = {flash = 18944, ram = 1616} features = ["g031"] stacksize = 936 diff --git a/app/donglet/app-g031.toml b/app/donglet/app-g031.toml index ed9d541d44..57c8dab432 100644 --- a/app/donglet/app-g031.toml +++ b/app/donglet/app-g031.toml @@ -6,7 +6,7 @@ board = "donglet-g031" [kernel] name = "app-donglet" -requires = {flash = 18944, ram = 1820} +requires = {flash = 19168, ram = 1820} features = ["g031"] stacksize = 936 diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index d92bcee194..57b4c43793 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -6,7 +6,7 @@ fwid = true [kernel] name = "sidecar" -requires = {flash = 25792, ram = 6256} +requires = {flash = 25952, ram = 6256} features = ["dump"] [caboose] diff --git a/build/xtask/src/dist.rs b/build/xtask/src/dist.rs index 0a4b813b6a..b26da2fcbb 100644 --- a/build/xtask/src/dist.rs +++ b/build/xtask/src/dist.rs @@ -375,8 +375,7 @@ pub fn package( // Build a set of requests for the memory allocator let mut task_reqs = HashMap::new(); for (t, sz) in task_sizes { - // XXX disabled because leases can't span regions - let _n = sz.len() + let n = sz.len() + cfg .toml .extern_regions_for(t, &cfg.toml.image_names[0]) @@ -394,7 +393,7 @@ pub fn package( t, TaskRequest { memory: sz, - spare_regions: 0, + spare_regions: 7 - n, }, ); } diff --git a/test/tests-stm32g0/app-g070.toml b/test/tests-stm32g0/app-g070.toml index 64f5872046..0976d155bb 100644 --- a/test/tests-stm32g0/app-g070.toml +++ b/test/tests-stm32g0/app-g070.toml @@ -6,7 +6,7 @@ memory = "memory-g070.toml" [kernel] name = "demo-stm32g0-nucleo" -requires = {flash = 19040, ram = 2828} +requires = {flash = 19112, ram = 2828} features = ["g070"] stacksize = 2048 From 3fd9d21b600353c90ab31c175df2a2c1b5cfc1d8 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Wed, 27 Mar 2024 17:46:39 -0700 Subject: [PATCH 06/26] power: add comments to gimlet rail sag trace facility This should help avoid the situation where someone unknowingly removes or modifies this in a way that breaks racktest. --- task/power/src/bsp/gimlet_bcdef.rs | 45 ++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/task/power/src/bsp/gimlet_bcdef.rs b/task/power/src/bsp/gimlet_bcdef.rs index b787720024..e6c6071900 100644 --- a/task/power/src/bsp/gimlet_bcdef.rs +++ b/task/power/src/bsp/gimlet_bcdef.rs @@ -77,9 +77,45 @@ pub(crate) fn get_state() -> PowerState { } } +/// Number of seconds (really, timer firings) between writes to the trace +/// buffer. +const TRACE_SECONDS: u32 = 10; + +/// Number of trace records to store. +/// +/// TODO: explain rationale for this value. +const TRACE_DEPTH: usize = 52; + +/// This enum and its corresponding ringbuf are being used to attempt to isolate +/// cases of this bug: +/// +/// https://github.com/oxidecomputer/mfg-quality/issues/140 +/// +/// Unless that bug report is closed or says otherwise, be careful modifying +/// this type, as you may break data collection. +/// +/// The basic theory here is: +/// +/// - Every `TRACE_SECONDS` seconds, the task wakes up and writes one `Now` +/// entry. +/// +/// - We then record one `Max5970` trace entry per MAX5970 being monitored. +/// +/// Tooling can then collect this ringbuf periodically and get recent events. #[derive(Copy, Clone, PartialEq)] enum Trace { + /// Written before trace records; the `u32` is the number of times the task + /// has woken up to process its timer. This is not exactly equivalent to + /// seconds because of the way the timer is maintained, but is approximately + /// seconds. Now(u32), + + /// Trace record written for each MAX5970. + /// + /// The `last_bounce_detected` field and those starting with `crossbounce_` + /// are copied from running state and may not be updated on every trace + /// event. The other fields are read while emitting the trace record and + /// should be current. Max5970 { sensor: SensorId, last_bounce_detected: Option, @@ -101,8 +137,12 @@ enum Trace { None, } -ringbuf!(Trace, 52, Trace::None); +ringbuf!(Trace, TRACE_DEPTH, Trace::None); +/// Records fields from `dev` and merges them with previous state from `peaks`, +/// updating `peaks` in the process. +/// +/// If any I2C operation fails, this will abort its work and return. fn trace_max5970( dev: &Max5970, sensor: SensorId, @@ -129,6 +169,7 @@ fn trace_max5970( _ => return, }; + // TODO: this update should probably happen after all I/O is done. if peaks.iout.bounced(min_iout, max_iout) || peaks.vout.bounced(min_vout, max_vout) { @@ -251,7 +292,7 @@ impl State { // // Trace the detailed state every ten seconds, provided that we are in A0. // - if state == PowerState::A0 && self.fired % 10 == 0 { + if state == PowerState::A0 && self.fired % TRACE_SECONDS == 0 { ringbuf_entry!(Trace::Now(self.fired)); for ((dev, sensor), peak) in CONTROLLER_CONFIG From 3cf06d757c3dbe7eed8bb716367d9af2d42b1d91 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Sat, 30 Mar 2024 15:04:41 -0700 Subject: [PATCH 07/26] nucleo-user-button: fix backwards comment The comment in the `nucleo-user-button` demo task gets the order of arguments to `sys.gpio_irq_control` backwards. Since this example task is intended to demonstrate how to use this API, we should definitely fix this. Thanks @bcantrill for catching it! --- task/nucleo-user-button/src/main.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/task/nucleo-user-button/src/main.rs b/task/nucleo-user-button/src/main.rs index a46efff193..6c5def3103 100644 --- a/task/nucleo-user-button/src/main.rs +++ b/task/nucleo-user-button/src/main.rs @@ -17,7 +17,7 @@ use userlib::*; #[cfg(not(any( target_board = "nucleo-h753zi", - target_board = "nucleo-h743zi2" + target_board = "nucleo-h743zi2", )))] compile_error!( "the `nucleo-user-button` task is only supported on the Nucleo H753ZI and H743ZI2 boards" @@ -90,7 +90,7 @@ pub fn main() -> ! { loop { // The first argument to `gpio_irq_control` is the mask of interrupts to - // enable, while the second is the mask to disable. So, enable the + // disable, while the second is the mask to enable. So, enable the // button notification. let disable_mask = 0; ringbuf_entry!(Trace::GpioIrqControl { @@ -110,6 +110,7 @@ pub fn main() -> ! { ) // Recv from the kernel never returns an error. .unwrap_lite(); + let notif = recvmsg.operation; ringbuf_entry!(Trace::Notification(notif)); From 4cbc197a3f66cc71b67212b59942427bbae53303 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 1 Apr 2024 10:14:05 -0700 Subject: [PATCH 08/26] stm32xx-sys: add more complete EXTI docs --- drv/stm32xx-sys/src/main.rs | 137 +++++++++++++++++++++++++++++++++++- 1 file changed, 135 insertions(+), 2 deletions(-) diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 526bf7a54b..8e1b96c787 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -34,7 +34,8 @@ //! EXTI unit. From a user perspective, this works as follows: //! //! 1. `sys` gets configured in the `app.toml` to route interrupts on a specific -//! pin to a specific task, using a specific notification. +//! pin to a specific task, using a specific notification. See [the section +//! on EXTI configuration](#configuring-exti-notification) for details. //! 2. That task configures the pin using `sys.gpio_configure` and friends //! (probably as an input, possibly with a pull resistor). //! 3. That task _also_ configures the pin's edge sensitivity, using @@ -73,7 +74,7 @@ //! //! 4. Repeat. //! -//! ## Configuring EXTI GPIO notifications +//! ## Configuring EXTI notifications //! //! In order for a task to receive notifications about GPIO pin interrupts from //! `sys`, we must first add the following to the task's config section in @@ -152,6 +153,138 @@ //! # The name of the client task and the notification to post to it. //! owner = { name = "my-great-task", notification = "my-gpio-notification" } //! ``` +//! +//! ## Using EXTI notifications +//! +//! Once a task has been configured to receive notifications from `sys` for EXTI +//! GPIO interrupts, as discussed above, it can use the `sys` task's IPC +//! interface to configure and control the EXTI interrupts. +//! +//! First, the task must ensure that it includes the generated constants for +//! notifications. If the task does not already include this code, it must add a +//! call to `build_util::build_notifications()` in its `build.rs`, and include +//! the generated using: +//! +//! ```rust +//! include!(concat!(env!("OUT_DIR"), "/notifications.rs")); +//! ``` +//! +//! In order to receive notifications, the task must first configure the pin on +//! which it wishes to receive interrupts in input mode, using the +//! [`Sys::gpio_configure_input`] IPC interface. Optionally, if pull-up or +//! pull-down resistors are required, they may be configured by this IPC as +//! well. Then, the task must use the [`Sys::gpio_irq_configure`] IPC to +//! configure the edge sensitivity for the GPIO interrupts mapped to a +//! notification mask. Interrupts can trigger either on the rising edge, the +//! falling edge, or both. These configurations only need to be performed once, +//! unless the task wishes to change the pin's configuration at runtime. +//! +//! For example, if we wish to use the EXTI notification +//! `"my-gpio-notification"` for pin PC13, as shown in the above section, we +//! might add the following to our task's `main`: +//! +//! ```rust,no-run +//! # mod notifications { pub const MY_GPIO_NOTIFICATION_MASK: u32 = 1 << 0; } +//! use drv_stm32xx_sys_api::{PinSet, Port, Pull}; +//! use userlib::*; +//! +//! task_slot!(SYS, sys); +//! +//! #[export_name = "main"] +//! pub fn main() -> ! { +//! let sys = drv_stm32xx_sys_api::Sys::from(SYS.get_task_id()); +//! +//! // Configure the pin as an input, without pull-up or pull-down +//! // resistors: +//! sys.gpio_configure_input( +//! PinSet { +//! port: Port::C, +//! pin_mask: (1 << 13), +//! }, +//! Pull::None, +//! ); +//! +//! // Now, configure the pin to trigger interrupts on the rising edge: +//! sys.gpio_irq_configure( +//! notifications::MY_GPIO_NOTIFICATION_MASK, +//! true, // whether to enable rising edge interrupts +//! false, // whether to enable falling edge interrupts +//! ); +//! +//! // Eventually we will do other things here +//! } +//!``` +//! +//! Once this is done, the task can enable a GPIO interrupt by calling the +//! [`Sys::gpio_irq_control`] IPC. This IPC takes two arguments: a mask of +//! notification bits to disable, and a mask of notification bits to enable. +//! Once the interrupt is enabled, the task will receive a notification when the +//! GPIO pin's level changes, based on the edge sensitivity configured above. +//! Once the interrupt has triggered a notification, it will be automatically +//! disabled until the task calls `gpio_irq_control` again to re-enable it. +//! +//! Thus, continuing from the above example, to wait for GPIO notifications in a +//! loop, we would write something like this: +//! +//! ```rust,no-run +//! # fn handle_interrupt() {} +//! # mod notifications { pub const MY_GPIO_NOTIFICATION_MASK: u32 = 1 << 0; } +//! use drv_stm32xx_sys_api::{PinSet, Port, Pull}; +//! use userlib::*; +//! +//! task_slot!(SYS, sys); +//! +//! #[export_name = "main"] +//! pub fn main() -> ! { +//! let sys = drv_stm32xx_sys_api::Sys::from(SYS.get_task_id()); +//! +//! sys.gpio_configure_input( +//! PinSet { +//! port: Port::C, +//! pin_mask: (1 << 13), +//! }, +//! Pull::None, +//! ); +//! +//! sys.gpio_irq_configure( +//! notifications::MY_GPIO_NOTIFICATION_MASK, +//! true, // whether to enable rising edge interrupts +//! false, // whether to enable falling edge interrupts +//! ); +//! +//! // Wait to recieve notifications for our GPIO interrupt in a loop: +//! loop { +//! // First, enable the interrupt, so that we can receive our +//! // notification: +//! sys.gpio_irq_control(0, notifications::MY_GPIO_NOTIFICATION_MASK); +//! +//! // Wait for a notification: +//! // +//! // We only care about notifications, so we can pass a zero-sized +//! // recv buffer, and the kernel's task ID. +//! let recvmsg = sys_recv_closed( +//! &mut [], +//! notifications::BUTTON_MASK, +//! TaskId::KERNEL, +//! ) +//! // Recv from the kernel never returns an error. +//! .unwrap_lite(); +//! +//! // Now, do ... whatever it is this task is supposed to do when +//! // the interrupt fires. +//! handle_interrupt(); +//! +//! // When the loop repeats, the call to `sys.gpio_irq_control()` will +//! // enable the interrupt again. +//! } +//! } +//!``` +//! +//! For a more complete example of using GPIO interrupts, see the +//! [`nucleo-user-button`] demo task, which toggles a user LED on an +//! STM32H7-NUCLEO dev board when the user button is pressed. +//! +//! [`nucleo-user-button`]: https://github.com/oxidecomputer/hubris/tree/master/task/nucleo-user-button #![no_std] #![no_main] From 409c9a5b5ac32649e142c496583763d1e4b32ab4 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 2 Apr 2024 12:33:28 -0700 Subject: [PATCH 09/26] stm32xx-sys: configure GPIO IRQ edge with an enum (#1706) The current `Sys::gpio_irq_configure` IPC interface is a bit confusing, as it takes two `bool`s for the rising and falling edge sensitivities, as in: ```rust sys.gpio_irq_configure(mask, true, false); ``` This isn't particularly clear for a reader, who then has to remember the order of the arguments to determine which edge sensitivity is being enabled here. This is a case of ["boolean blindness"][1]. This commit improves the interface by changing it to take an `enum` with variants for `Rising`, `Falling`, and `Both`. Now, the same call to the IPC looks like this: ```rust sys.gpio_irq_configure(mask, Edge::Rising); ``` which should be much more obvious to a reader, who no longer needs to remember the argument ordering to decypher calls to this function. Also, this has the advantage of preventing the case where both rising and falling edge sensitivities are disabled at compile-time. Currently, this is a runtime error (reply fault), but with the new interface, a caller can provide an invalid configuration if they hand-write the input buffer rather than using the Idol client stub, so it should be harder to mess up. [1]: https://runtimeverification.com/blog/code-smell-boolean-blindness --- Cargo.lock | 1 + app/demo-stm32h7-nucleo/app-h743.toml | 2 +- app/demo-stm32h7-nucleo/app-h753.toml | 2 +- drv/stm32xx-sys-api/Cargo.toml | 1 + drv/stm32xx-sys-api/src/lib.rs | 46 +++++++++++++++++++++++++++ drv/stm32xx-sys/src/main.rs | 36 ++++++--------------- idl/stm32xx-sys.idol | 9 +++--- task/nucleo-user-button/src/main.rs | 29 ++++++----------- 8 files changed, 73 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index df610976eb..40b77a284b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2042,6 +2042,7 @@ dependencies = [ "idol", "idol-runtime", "num-traits", + "serde", "userlib", "zerocopy 0.6.6", ] diff --git a/app/demo-stm32h7-nucleo/app-h743.toml b/app/demo-stm32h7-nucleo/app-h743.toml index b77041786e..af3ab50143 100644 --- a/app/demo-stm32h7-nucleo/app-h743.toml +++ b/app/demo-stm32h7-nucleo/app-h743.toml @@ -107,7 +107,7 @@ priority = 3 start = true task-slots = ["sys", "user_leds"] notifications = ["button"] -config = { led = 1, rising = false, falling = true } +config = { led = 1, edge = "Edge::Rising" } [tasks.udpecho] name = "task-udpecho" diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index 0fd3a5a4db..8c0017f0b3 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -116,7 +116,7 @@ priority = 3 start = true task-slots = ["sys", "user_leds"] notifications = ["button"] -config = { led = 1, rising = false, falling = true } +config = { led = 1, edge = "Edge::Rising" } [tasks.udpecho] name = "task-udpecho" diff --git a/drv/stm32xx-sys-api/Cargo.toml b/drv/stm32xx-sys-api/Cargo.toml index 9161364957..d469746341 100644 --- a/drv/stm32xx-sys-api/Cargo.toml +++ b/drv/stm32xx-sys-api/Cargo.toml @@ -9,6 +9,7 @@ cfg-if.workspace = true idol-runtime.workspace = true num-traits.workspace = true zerocopy.workspace = true +serde.workspace = true counters = { path = "../../lib/counters" } derive-idol-err = { path = "../../lib/derive-idol-err" } diff --git a/drv/stm32xx-sys-api/src/lib.rs b/drv/stm32xx-sys-api/src/lib.rs index b944ae14ce..d710630c20 100644 --- a/drv/stm32xx-sys-api/src/lib.rs +++ b/drv/stm32xx-sys-api/src/lib.rs @@ -20,6 +20,7 @@ cfg_if::cfg_if! { use derive_idol_err::IdolError; use userlib::*; +use zerocopy::AsBytes; pub use drv_stm32xx_gpio_common::{ Alternate, Mode, OutputType, PinSet, Port, Pull, Speed, @@ -32,6 +33,25 @@ pub enum RccError { NoSuchPeripheral = 1, } +/// Configures edge sensitivity for a GPIO interrupt +#[derive( + Copy, Clone, FromPrimitive, PartialEq, Eq, AsBytes, serde::Deserialize, +)] +// NOTE: This `repr` attribute is *not* necessary for +// serialization/deserialization, but it is used to allow casting to `u8` in the +// `Edge::{is_rising, is_falling}` methods. The current implementation of those +// methods with bit-and tests generates substantially fewer instructions than +// using `matches!` (see: https://godbolt.org/z/j5fdPfz3c). +#[repr(u8)] +pub enum Edge { + /// The interrupt will trigger on the rising edge only. + Rising = 0b01, + /// The interrupt will trigger on the falling edge only. + Falling = 0b10, + /// The interrupt will trigger on both teh rising and falling edge. + Both = 0b11, +} + impl Sys { /// Requests that the clock to a peripheral be turned on. /// @@ -285,4 +305,30 @@ impl Sys { } } +impl Edge { + /// Returns `true` if this edge sensitivity should trigger on the rising + /// edge. + pub fn is_rising(&self) -> bool { + *self as u8 & Self::Rising as u8 != 0 + } + + /// Returns `true` if this edge sensitivity should trigger on the falling + /// edge. + pub fn is_falling(&self) -> bool { + *self as u8 & Self::Falling as u8 != 0 + } +} + +impl core::ops::BitOr for Edge { + type Output = Self; + + fn bitor(self, rhs: Self) -> Self::Output { + match (self, rhs) { + (Edge::Rising, Edge::Rising) => Edge::Rising, + (Edge::Falling, Edge::Falling) => Edge::Falling, + _ => Edge::Both, + } + } +} + include!(concat!(env!("OUT_DIR"), "/client_stub.rs")); diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 8e1b96c787..962612ba8c 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -207,8 +207,7 @@ //! // Now, configure the pin to trigger interrupts on the rising edge: //! sys.gpio_irq_configure( //! notifications::MY_GPIO_NOTIFICATION_MASK, -//! true, // whether to enable rising edge interrupts -//! false, // whether to enable falling edge interrupts +//! Edge::Rising, // or `Edge::Falling`, `Edge::Both` //! ); //! //! // Eventually we will do other things here @@ -229,7 +228,7 @@ //! ```rust,no-run //! # fn handle_interrupt() {} //! # mod notifications { pub const MY_GPIO_NOTIFICATION_MASK: u32 = 1 << 0; } -//! use drv_stm32xx_sys_api::{PinSet, Port, Pull}; +//! use drv_stm32xx_sys_api::{PinSet, Port, Pull, Edge}; //! use userlib::*; //! //! task_slot!(SYS, sys); @@ -248,8 +247,7 @@ //! //! sys.gpio_irq_configure( //! notifications::MY_GPIO_NOTIFICATION_MASK, -//! true, // whether to enable rising edge interrupts -//! false, // whether to enable falling edge interrupts +//! Edge::Rising, // or `Edge::Falling`, `Edge::Both` //! ); //! //! // Wait to recieve notifications for our GPIO interrupt in a loop: @@ -320,7 +318,7 @@ cfg_if! { } use drv_stm32xx_gpio_common::{server::get_gpio_regs, Port}; -use drv_stm32xx_sys_api::{Group, RccError}; +use drv_stm32xx_sys_api::{Edge, Group, RccError}; use idol_runtime::{ClientError, NotificationHandler, RequestError}; #[cfg(not(feature = "test"))] use task_jefe_api::{Jefe, ResetReason}; @@ -742,8 +740,7 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { &mut self, rm: &RecvMessage, mask: u32, - rising: bool, - falling: bool, + edge: Edge, ) -> Result<(), RequestError> { // We want to only include code for this if exti is requested. // Unfortunately the _operation_ is available unconditionally, but we'll @@ -751,21 +748,6 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { cfg_if! { if #[cfg(feature = "exti")] { - // Guardrail: a call where both "rising" and "falling" are false - // will effectively disable the interrupt, in a _different_ way - // from gpio_irq_control. Currently we can't imagine a case - // where this is useful, but we _can_ imagine cases where - // tolerating it would hide bugs in code, and so we're making it - // an error. - // - // If you arrive here in the future wondering why this is being - // treated as an error, that's why. If you need this - // functionality, you can probably just remove this check and - // comment. - if !rising && !falling { - return Err(ClientError::BadMessageContents.fail()); - } - // Keep track of which bits in the caller-provided masks // actually matched things. let mut used_bits = 0u32; @@ -785,7 +767,7 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // Set or clear Rising Trigger Selection // Register bit according to the rising flag self.exti.rtsr1.modify(|r, w| { - let new_value = if rising { + let new_value = if edge.is_rising() { r.bits() | (1 << i) } else { r.bits() & !(1 << i) @@ -798,7 +780,7 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // Set or clear Falling Trigger Selection // Register bit according to the rising flag self.exti.ftsr1.modify(|r, w| { - let new_value = if falling { + let new_value = if edge.is_falling() { r.bits() | (1 << i) } else { r.bits() & !(1 << i) @@ -832,7 +814,7 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { } else { // Suppress unused variable warnings (yay conditional // compilation) - let _ = (rm, mask, rising, falling); + let _ = (rm, mask, edge); // Fault any clients who try to use this in an image where it's // not included. @@ -1190,7 +1172,7 @@ cfg_if! { include!(concat!(env!("OUT_DIR"), "/notifications.rs")); mod idl { - use super::{Port, RccError}; + use super::{Edge, Port, RccError}; include!(concat!(env!("OUT_DIR"), "/server_stub.rs")); } diff --git a/idl/stm32xx-sys.idol b/idl/stm32xx-sys.idol index 16d240de30..48a50bfc3d 100644 --- a/idl/stm32xx-sys.idol +++ b/idl/stm32xx-sys.idol @@ -99,14 +99,13 @@ Interface( // Configures some set of EXTI sources associated with the caller, // using the caller's notification bit space to name them. Any // sources included in the `mask` will be affected. - // - // If neither rising nor falling is true, the pin will not be - // capable of producing an interrupt, even if enabled. "gpio_irq_configure": ( args: { "mask": "u32", - "rising": "bool", - "falling": "bool", + "sensitivity": ( + type: "Edge", + recv: FromPrimitive("u8"), + ), }, reply: Simple("()"), idempotent: true, diff --git a/task/nucleo-user-button/src/main.rs b/task/nucleo-user-button/src/main.rs index 6c5def3103..c05e47a2f0 100644 --- a/task/nucleo-user-button/src/main.rs +++ b/task/nucleo-user-button/src/main.rs @@ -11,7 +11,7 @@ #![no_std] #![no_main] -use drv_stm32xx_sys_api::{PinSet, Port, Pull}; +use drv_stm32xx_sys_api::{Edge, PinSet, Port, Pull}; use ringbuf::ringbuf_entry; use userlib::*; @@ -29,10 +29,8 @@ task_slot!(SYS, sys); task_config::optional_task_config! { /// The index of the user LED to toggle led: usize, - /// Whether to enable rising-edge interrupts - rising: bool, - /// Whether to enable falling-edge interrupts - falling: bool, + /// Edge sensitivity for the button interrupt + edge: Edge, } // In real life, we might not actually want to trace all of these events, but @@ -42,13 +40,8 @@ task_config::optional_task_config! { enum Trace { #[count(skip)] None, - /// We called the `Sys.gpio_irq_configure` IPC with these arguments. - GpioIrqConfigure { - mask: u32, - rising: bool, - falling: bool, - }, + GpioIrqConfigure { mask: u32, edge: Edge }, /// We called the `Sys.gpio_irq_control` IPC with these arguments. GpioIrqControl { enable_mask: u32, disable_mask: u32 }, @@ -67,11 +60,10 @@ pub fn main() -> ! { let user_leds = drv_user_leds_api::UserLeds::from(USER_LEDS.get_task_id()); let sys = drv_stm32xx_sys_api::Sys::from(SYS.get_task_id()); - let (led, rising, falling) = if let Some(config) = TASK_CONFIG { - (config.led, config.rising, config.falling) - } else { - (0, false, true) - }; + let Config { led, edge } = TASK_CONFIG.unwrap_or(Config { + led: 0, + edge: Edge::Rising, + }); sys.gpio_configure_input( PinSet { @@ -83,10 +75,9 @@ pub fn main() -> ! { ringbuf_entry!(Trace::GpioIrqConfigure { mask: notifications::BUTTON_MASK, - rising, - falling, + edge, }); - sys.gpio_irq_configure(notifications::BUTTON_MASK, rising, falling); + sys.gpio_irq_configure(notifications::BUTTON_MASK, edge); loop { // The first argument to `gpio_irq_control` is the mask of interrupts to From 852fb9e3c8f9795d289e266ccf932f19f7d77087 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 2 Apr 2024 14:19:03 -0700 Subject: [PATCH 10/26] stm32xx: generate counters for EXTI IRQs (#1698) --- Cargo.lock | 1 + build/stm32xx-sys/src/lib.rs | 58 ++++++++++++++++++++++++++++-------- drv/stm32xx-sys/Cargo.toml | 6 ++-- drv/stm32xx-sys/src/main.rs | 29 +++++++++++++----- 4 files changed, 72 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 40b77a284b..c2c131042f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2016,6 +2016,7 @@ dependencies = [ "build-stm32xx-sys", "build-util", "cfg-if", + "counters", "drv-stm32xx-gpio-common", "drv-stm32xx-sys-api", "drv-stm32xx-uid", diff --git a/build/stm32xx-sys/src/lib.rs b/build/stm32xx-sys/src/lib.rs index de630ecccf..b5498a091c 100644 --- a/build/stm32xx-sys/src/lib.rs +++ b/build/stm32xx-sys/src/lib.rs @@ -81,19 +81,20 @@ impl SysConfig { &self, ) -> anyhow::Result { #[derive(Debug)] - struct DispatchEntry<'a> { - _name: &'a str, + struct DispatchEntry { port: Port, task: syn::Ident, note: syn::Ident, + name: syn::Ident, } const NUM_EXTI_IRQS: usize = 16; - let mut dispatch_table: [Option>; NUM_EXTI_IRQS] = + let mut dispatch_table: [Option; NUM_EXTI_IRQS] = Default::default(); + let mut has_any_notifications = false; for ( - _name, + name, &GpioIrqConfig { port, pin, @@ -106,13 +107,19 @@ impl SysConfig { anyhow::bail!("pin {pin} is already mapped to IRQ {curr:?}") } Some(slot) => { + has_any_notifications = true; let task = syn::parse_str(&owner.name)?; let note = quote::format_ident!( "{}_MASK", owner.notification.to_uppercase().replace('-', "_") ); + + let name = quote::format_ident!( + "{}", + name.replace('-', "_") + ); *slot = Some(DispatchEntry { - _name, + name, port, task, note, @@ -126,22 +133,49 @@ impl SysConfig { let dispatches = dispatch_table.iter().map(|slot| match slot { Some(DispatchEntry { - port, task, note, .. + name, + port, + task, + note, + .. }) => quote! { - Some(( - #port, - userlib::TaskId::for_index_and_gen( + Some(ExtiDispatch { + port: #port, + task: userlib::TaskId::for_index_and_gen( hubris_num_tasks::Task::#task as usize, userlib::Generation::ZERO, ), - crate::notifications::#task::#note, - )) + mask: crate::notifications::#task::#note, + name: ExtiIrq::#name, + }) }, None => quote! { None }, }); + let counter_type = if has_any_notifications { + let irq_names = dispatch_table + .iter() + .filter_map(|slot| Some(&slot.as_ref()?.name)); + quote! { + #[derive(Copy, Clone, PartialEq, Eq, counters::Count)] + #[allow(nonstandard_style)] + pub(crate) enum ExtiIrq { + #( #irq_names ),* + } + } + } else { + // If there are no EXTI notifications enabled, just use `()` as the counter + // type, as it does implement `counters::Count`, but has no values, so + // we don't get a "matching on an uninhabited type" error. + quote! { + pub(crate) type ExtiIrq = (); + } + }; + Ok(quote! { - pub(crate) const EXTI_DISPATCH_TABLE: [Option<(Port, TaskId, u32)>; #NUM_EXTI_IRQS] = [ + #counter_type + + pub(crate) const EXTI_DISPATCH_TABLE: [Option; #NUM_EXTI_IRQS] = [ #( #dispatches ),* ]; }) diff --git a/drv/stm32xx-sys/Cargo.toml b/drv/stm32xx-sys/Cargo.toml index 9ac31aed96..9ed1aee85f 100644 --- a/drv/stm32xx-sys/Cargo.toml +++ b/drv/stm32xx-sys/Cargo.toml @@ -10,6 +10,7 @@ drv-stm32xx-uid = { path = "../../drv/stm32xx-uid" } hubris-num-tasks = { path = "../../sys/num-tasks", features = ["task-enum"], optional = true } task-jefe-api = { path="../../task/jefe-api" } userlib = { path = "../../sys/userlib" } +counters = { path = "../../lib/counters", optional = true } bitflags = { workspace = true } cfg-if = { workspace = true } @@ -34,14 +35,15 @@ g030 = ["family-stm32g0", "stm32g0/stm32g030", "drv-stm32xx-sys-api/g030", "drv- g031 = ["family-stm32g0", "stm32g0/stm32g031", "drv-stm32xx-sys-api/g031", "drv-stm32xx-gpio-common/model-stm32g031"] g070 = ["family-stm32g0", "stm32g0/stm32g070", "drv-stm32xx-sys-api/g070", "drv-stm32xx-gpio-common/model-stm32g070"] g0b1 = ["family-stm32g0", "stm32g0/stm32g0b1", "drv-stm32xx-sys-api/g0b1", "drv-stm32xx-gpio-common/model-stm32g0b1"] + no-ipc-counters = ["idol/no-counters"] # Enable external interrupt controller support. -exti = ["dep:hubris-num-tasks"] +exti = ["dep:hubris-num-tasks", "dep:counters"] # Disables the Jefe dependency, for use in tests where the test-runner task is # used as supervisor, rather than Jefe. -# +# # TODO(eliza): eventually, it would be much better if tasks that depend on the # sys driver could run separately from the kernel tests, and use a real Jefe # rather than the test supervisor. But, for now, this makes it build. diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 962612ba8c..22b5bc920e 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -396,7 +396,7 @@ fn main() -> ! { for (i, entry) in generated::EXTI_DISPATCH_TABLE.iter().enumerate() { // Process entries that are filled in... - if let &Some((port, _, _)) = entry { + if let &Some(ExtiDispatch { port, .. }) = entry { let register = i >> 2; let slot = i % 2; @@ -533,6 +533,9 @@ fn main() -> ! { } } +#[cfg(feature = "exti")] +counters::counters!(__EXTI_IRQ_COUNTERS, generated::ExtiIrq); + struct ServerImpl<'a> { rcc: &'a device::rcc::RegisterBlock, @@ -663,9 +666,9 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { generated::EXTI_DISPATCH_TABLE.iter().enumerate() { // Only use populated rows in the table - if let Some((_, tid, mask)) = entry { + if let &Some(ExtiDispatch { mask, task, .. }) = entry { // Ignore anything assigned to another task - if tid.index() == rm.sender.index() { + if task.index() == rm.sender.index() { // Apply disable first so that including the same // thing in both masks is a no-op. @@ -756,10 +759,10 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { generated::EXTI_DISPATCH_TABLE.iter().enumerate() { // Only use populated rows in the table - if let Some((_, tid, entry_mask)) = entry { + if let Some(ExtiDispatch { task, mask: entry_mask, .. }) = entry { // Operate only on rows assigned to the sending task // _and_ that are relevant based on the arguments. - if tid.index() == rm.sender.index() + if task.index() == rm.sender.index() && mask & entry_mask != 0 { used_bits |= mask; @@ -824,6 +827,14 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { } } +#[cfg(feature = "exti")] +struct ExtiDispatch { + port: Port, + task: TaskId, + mask: u32, + name: generated::ExtiIrq, +} + impl NotificationHandler for ServerImpl<'_> { fn current_notification_mask(&self) -> u32 { cfg_if! { @@ -872,9 +883,11 @@ impl NotificationHandler for ServerImpl<'_> { // - Clear the pending bit (we have to do this // manually unlike native interrupts). - if let &Some((_, taskid, mask)) = entry { - let taskid = sys_refresh_task_id(taskid); - sys_post(taskid, mask); + if let &Some(ExtiDispatch { task, mask, name, .. }) = entry { + counters::count!(__EXTI_IRQ_COUNTERS, name); + + let task = sys_refresh_task_id(task); + sys_post(task, mask); } else { // spurious interrupt. // TODO: probably add this to a counter; it's From e676e419a18d8c7a108809c0db163717261abace Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Mon, 1 Apr 2024 14:21:52 -0700 Subject: [PATCH 11/26] sensor: remove noop 1 Hz timer task/sensor was configuring a timer to wake it every 1 Hz. Each time it woke up, it would snooze its alarm clock and go back to sleep. While I definitely sympathize with that desire, this is essentially dead code that looks like a bug, so this commit removes it. I'm not sure what the history here is. In addition to removing noise from the code, this knocks 114 bytes off the sensor task text size. --- app/gimlet/base.toml | 1 - app/gimletlet/app.toml | 1 - app/psc/base.toml | 1 - app/sidecar/base.toml | 1 - task/sensor/build.rs | 1 - task/sensor/src/main.rs | 22 ++++++---------------- 6 files changed, 6 insertions(+), 21 deletions(-) diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 5e38e71245..0649f968ea 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -214,7 +214,6 @@ priority = 4 max-sizes = {flash = 16384, ram = 8192 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.host_sp_comms] name = "task-host-sp-comms" diff --git a/app/gimletlet/app.toml b/app/gimletlet/app.toml index 78a26012a3..40559d81ba 100644 --- a/app/gimletlet/app.toml +++ b/app/gimletlet/app.toml @@ -60,7 +60,6 @@ priority = 5 max-sizes = {flash = 16384, ram = 2048 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.dump_agent] name = "task-dump-agent" diff --git a/app/psc/base.toml b/app/psc/base.toml index eaaf62c64a..8679a610ee 100644 --- a/app/psc/base.toml +++ b/app/psc/base.toml @@ -219,7 +219,6 @@ priority = 3 max-sizes = {flash = 16384, ram = 8192 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.sensor_polling] name = "task-sensor-polling" diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index 57b4c43793..e52b5366bc 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -184,7 +184,6 @@ priority = 4 max-sizes = {flash = 16384, ram = 8192 } stacksize = 1024 start = true -notifications = ["timer"] [tasks.ecp5_mainboard] name = "drv-fpga-server" diff --git a/task/sensor/build.rs b/task/sensor/build.rs index c58bbe9b62..da98c3962d 100644 --- a/task/sensor/build.rs +++ b/task/sensor/build.rs @@ -4,7 +4,6 @@ fn main() -> Result<(), Box> { build_util::expose_target_board(); - build_util::build_notifications()?; idol::Generator::new() .with_counters( idol::CounterSettings::default().with_server_counters(false), diff --git a/task/sensor/src/main.rs b/task/sensor/src/main.rs index a2b8afaee3..f426ca1ce9 100644 --- a/task/sensor/src/main.rs +++ b/task/sensor/src/main.rs @@ -71,11 +71,8 @@ struct ServerImpl { err_time: SensorArray, nerrors: SensorArray, - deadline: u64, } -const TIMER_INTERVAL: u64 = 1000; - impl idl::InOrderSensorImpl for ServerImpl { fn get( &mut self, @@ -243,24 +240,20 @@ impl ServerImpl { impl NotificationHandler for ServerImpl { fn current_notification_mask(&self) -> u32 { - notifications::TIMER_MASK + // We don't use notifications, don't listen for any. + 0 } fn handle_notification(&mut self, _bits: u32) { - self.deadline = self.deadline.wrapping_add(TIMER_INTERVAL); - sys_set_timer(Some(self.deadline), notifications::TIMER_MASK); + unreachable!() } } #[export_name = "main"] fn main() -> ! { - let deadline = sys_get_timer().now; - - // - // This will put our timer in the past, and should immediately kick us. - // - sys_set_timer(Some(deadline), notifications::TIMER_MASK); - + // N.B. if you are staring at this macro thinking that it looks like it + // doesn't do anything and might be obsolescent, the key is the :upper. This + // macro exists exclusively to uppercase the field names below. macro_rules! declare_server { ($($name:ident: $t:ty = $n:expr;)*) => {{ paste::paste! { @@ -271,7 +264,6 @@ fn main() -> ! { }; let ($($name),*) = ($(SensorArray($name)),*); ServerImpl { - deadline, $($name),* } }} @@ -308,5 +300,3 @@ mod idl { include!(concat!(env!("OUT_DIR"), "/server_stub.rs")); } - -include!(concat!(env!("OUT_DIR"), "/notifications.rs")); From ab78c7e8b02758d21dcf89a29a5f58bd99dc54a3 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 2 Apr 2024 15:57:24 -0700 Subject: [PATCH 12/26] userlib: add no-panic feature When enabled, this will turn any panics in a task into linker errors. That's not ideal, but it's better than expecting people to manually read disassembly. --- sys/userlib/Cargo.toml | 1 + sys/userlib/src/lib.rs | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/sys/userlib/Cargo.toml b/sys/userlib/Cargo.toml index 062e309098..fa9a9f9aee 100644 --- a/sys/userlib/Cargo.toml +++ b/sys/userlib/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [features] default = ["critical-section"] panic-messages = [] +no-panic = [] critical-section = ["dep:critical-section"] [dependencies] diff --git a/sys/userlib/src/lib.rs b/sys/userlib/src/lib.rs index 8e27eba9ca..3abd3abffa 100644 --- a/sys/userlib/src/lib.rs +++ b/sys/userlib/src/lib.rs @@ -1296,7 +1296,7 @@ pub unsafe extern "C" fn _start() -> ! { /// task, to ensure that memory is available for the panic message, even if the /// resources have been trimmed aggressively using `xtask sizes` and `humility /// stackmargin`. -#[cfg(feature = "panic-messages")] +#[cfg(all(not(feature = "no-panic"), feature = "panic-messages"))] #[panic_handler] fn panic(info: &core::panic::PanicInfo<'_>) -> ! { // Implementation Note @@ -1443,12 +1443,26 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { /// Panic handler for tasks without the `panic-messages` feature enabled. This /// kills the task with a fixed message, `"PANIC"`. While this is less helpful /// than a proper panic message, the stack trace can still be informative. -#[cfg(not(feature = "panic-messages"))] +#[cfg(all(not(feature = "no-panic"), not(feature = "panic-messages")))] #[panic_handler] fn panic(_: &core::panic::PanicInfo<'_>) -> ! { sys_panic(b"PANIC") } +/// Panic handler for when panics are not permitted in a task. This is enabled +/// by the `no-panic` feature and causes a link error if a panic is introduced. +#[cfg(feature = "no-panic")] +#[panic_handler] +fn panic(_: &core::panic::PanicInfo<'_>) -> ! { + extern "C" { + fn you_have_introduced_a_panic_which_is_not_permitted() -> !; + } + + // Safety: this function does not exist, this code will not pass the linker + // and is thus not reachable. + unsafe { you_have_introduced_a_panic_which_is_not_permitted() } +} + #[inline(always)] pub fn sys_refresh_task_id(task_id: TaskId) -> TaskId { let tid = unsafe { sys_refresh_task_id_stub(task_id.0 as u32) }; From 02bfc32072e6c005597a0b4525fbf3785dde298b Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Wed, 3 Apr 2024 11:14:32 -0700 Subject: [PATCH 13/26] userlib: fail if no-panic and panic-messages are both set --- sys/userlib/src/lib.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sys/userlib/src/lib.rs b/sys/userlib/src/lib.rs index 3abd3abffa..c537e3001b 100644 --- a/sys/userlib/src/lib.rs +++ b/sys/userlib/src/lib.rs @@ -1288,6 +1288,15 @@ pub unsafe extern "C" fn _start() -> ! { } } +// Make the no-panic and panic-messages features mutually exclusive. +#[cfg(all(feature = "no-panic", feature = "panic-messages"))] +compile_error!( + "Both the userlib/panic-messages and userlib/no-panic feature flags \ + are set! This doesn't make a lot of sense and is probably not what \ + you wanted. (If you have a use case for this combination, update \ + this check in userlib.)" +); + /// Panic handler for user tasks with the `panic-messages` feature enabled. This /// handler will try its best to generate a panic message, up to a maximum /// buffer size (configured below). From b69055699f65accaf1d65a67610200f0cc7798c4 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Mon, 1 Apr 2024 09:53:51 -0700 Subject: [PATCH 14/26] kern: improve comments in RECV impl These comments by 2020-me didn't super make sense to 2024-me, so I have rewritten them to hopefully be a little more clear and reveal some context that was previously implicit. --- sys/kern/src/syscalls.rs | 63 ++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 12 deletions(-) diff --git a/sys/kern/src/syscalls.rs b/sys/kern/src/syscalls.rs index f61a1ca53b..77211d63c3 100644 --- a/sys/kern/src/syscalls.rs +++ b/sys/kern/src/syscalls.rs @@ -191,12 +191,35 @@ fn send(tasks: &mut [Task], caller: usize) -> Result { /// /// `caller` is a valid task index (i.e. not directly from user code). /// +/// # Returns +/// +/// If the operation found a sender and delivered a message, +/// `Ok(NextTask::Same)` to drop us right back into the caller. +/// +/// If the operation did not find a sender but was otherwise valid (i.e. needs +/// to block the sender), `Ok(something_else)` to context switch away from the +/// sender. +/// +/// This may also return `Ok(something_else)` to context switch to the +/// supervisor even if a message was successfully delivered, but only if at +/// least one blocked sender with invalid configuration was found and faulted +/// along the way. +/// +/// In terms of errors, +/// +/// `Err(UserError::Recoverable(..))` is only used to return Dead Codes, and +/// only in closed receive specifically. +/// +/// All other errors are `Err(UserError::Unrecoverable(_))` that result in a +/// fault delivered to the caller. This indicates that the location where the +/// caller requested to receive a delivered message isn't accessible or valid. +/// /// # Panics /// /// If `caller` is out of range for `tasks`. fn recv(tasks: &mut [Task], caller: usize) -> Result { - // We allow tasks to atomically replace their notification mask at each - // receive. We simultaneously find out if there are notifications pending. + // `take_notifications` interprets the new notification mask and finds out + // if notifications are pending. if let Some(firing) = tasks[caller].take_notifications() { // Pending! Deliver an artificial message from the kernel. tasks[caller].save_mut().set_recv_result( @@ -226,7 +249,12 @@ fn recv(tasks: &mut [Task], caller: usize) -> Result { // outcomes here. // First possibility: that task you're asking about is DEAD. + // + // N.B. this is actually the only point in the RECV implementation where + // the sender may receive an error code (as opposed to being faulted or + // just blocking waiting for a valid sender). let sender_idx = task::check_task_id_against_table(tasks, sender_id)?; + // Second possibility: task has a message for us. if tasks[sender_idx].state().is_sending_to(caller_id) { // Oh hello sender! @@ -238,15 +266,24 @@ fn recv(tasks: &mut [Task], caller: usize) -> Result { } Err(interact) => { // Delivery failed because of fault events in one or both - // tasks. We need to apply the fault status, and then if we - // didn't have to murder the caller, we'll retry receiving a - // message. + // tasks. `apply_to_src` extracts the src (sender) side and + // applies it directly to the task; if there was a problem + // on the dst (recv'r, us) side we ? it. It's a FaultInfo, + // so if dst screwed up the caller task will be faulted. + // + // If there was no problem, the caller will be informed that + // the task has died, and will generally opt to retry the + // recv as an open recv. + // + // The wake hint here may wake the supervisor before the + // caller regains control. let wake_hint = interact.apply_to_src(tasks, sender_idx)?; + // No fault in the caller at least, carry on. next_task = next_task.combine(wake_hint); } } } - // Third possibility: we need to block; fall through below. + // Third possibility: we need to block; fall through below. } else { // Open Receive @@ -269,20 +306,22 @@ fn recv(tasks: &mut [Task], caller: usize) -> Result { }) { // Oh hello sender! match deliver(tasks, sender, caller) { - Ok(_) => { + Ok(()) => { // Delivery succeeded! Sender is now blocked in reply. Go ahead // and let the caller resume. return Ok(next_task); } Err(interact) => { // Delivery failed because of fault events in one or both - // tasks. We need to apply the fault status, and then if we - // didn't have to murder the caller, we'll retry receiving a - // message. + // tasks. Because we're transferring a message from the + // sender to the recv'r (us, the caller) we use apply_to_src + // to potentially fault the sender, and then apply the dst + // side to the caller using ?. let wake_hint = interact.apply_to_src(tasks, sender)?; + // No fault in the caller, at least. This may wake the + // supervisor. next_task = next_task.combine(wake_hint); - // Okay, if we didn't just return, retry the search from a new - // position. + // Retry the search from our new position. last = sender; } } From aa2fadb8269a28515784b3c8e32be2ae53658768 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Mon, 1 Apr 2024 11:05:11 -0700 Subject: [PATCH 15/26] Introduce and use sys_recv_notification. This is an alternate API for accessing the common "receive only kernel notifications" pattern. That specific use of the sys_recv syscall is defined as not being able to fail. Different users have handled that in different ways -- some panic, some ignore the result. The latter embeds the assumption of that behavior in many different places in the codebase, a pattern I'm trying to keep an eye on. This change adds a new sys_recv_notification userlib function that bundles up this common pattern. Because of its central placement in userlib, it seems like a more appropriate place to encode the assumption about the syscall not failing, and so I've done that, using unreachable_unchecked to explain it to the compiler. This removes a panic site from every notification-wait in programs that were previously using unwrap or unwrap_lite. --- drv/gimlet-seq-server/src/main.rs | 5 +-- drv/lpc55-sha256/src/lib.rs | 14 ++----- drv/lpc55-spi-server/src/main.rs | 6 +-- drv/lpc55-sprot-server/src/main.rs | 9 +---- drv/lpc55-update-server/src/main.rs | 11 +---- drv/psc-seq-server/src/main.rs | 2 +- drv/stm32h7-eth/src/lib.rs | 6 +-- drv/stm32h7-hash/src/lib.rs | 3 +- drv/stm32h7-qspi/src/lib.rs | 16 +++----- drv/stm32h7-spi-server-core/src/lib.rs | 7 +--- drv/stm32h7-update-server/src/main.rs | 7 +--- drv/stm32xx-i2c-server/src/main.rs | 16 ++++---- sys/userlib/src/hl.rs | 6 +-- sys/userlib/src/lib.rs | 56 +++++++++++++++++++++----- task/gimlet-inspector/src/main.rs | 14 +------ task/net/src/bsp/gimlet_bcdef.rs | 9 ++--- task/net/src/bsp/psc_a.rs | 9 ++--- task/net/src/bsp/psc_bc.rs | 9 ++--- task/nucleo-user-button/src/main.rs | 13 +----- task/sp_measure/src/main.rs | 4 +- task/spd/src/main.rs | 13 ++---- task/uartecho/src/main.rs | 6 +-- task/udpecho/src/main.rs | 14 +------ task/udprpc/src/main.rs | 14 +------ test/test-suite/src/main.rs | 9 +++++ 25 files changed, 107 insertions(+), 171 deletions(-) diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs index 980c23d656..c0ba2d5438 100644 --- a/drv/gimlet-seq-server/src/main.rs +++ b/drv/gimlet-seq-server/src/main.rs @@ -153,10 +153,7 @@ fn main() -> ! { // irrelevant. But, `rustc` doesn't realize that this should // never return, we'll stick it in a `loop` anyway so the main // function can return `!` - // - // We don't care if this returns an error, because we're just - // doing it to die as politely as possible. - let _ = sys_recv_closed(&mut [], 0, TaskId::KERNEL); + sys_recv_notification(0); } } } diff --git a/drv/lpc55-sha256/src/lib.rs b/drv/lpc55-sha256/src/lib.rs index 68df54cd54..b667732167 100644 --- a/drv/lpc55-sha256/src/lib.rs +++ b/drv/lpc55-sha256/src/lib.rs @@ -55,7 +55,7 @@ //! (This restriction would be straightforward to lift if required.) use core::num::Wrapping; -use userlib::{sys_irq_control, sys_recv_closed, TaskId}; +use userlib::{sys_irq_control, sys_recv_notification}; // These constants describe intrinsic properties of the SHA256 algorithm and // should not be changed. @@ -201,11 +201,7 @@ impl<'a> Hasher<'a> { // Wait for it! sys_irq_control(self.notification_mask, true); - let _ = sys_recv_closed( - &mut [], - self.notification_mask, - TaskId::KERNEL, - ); + sys_recv_notification(self.notification_mask); // Turn it back off lest it spam us in the future. self.engine.intenclr.write(|w| w.digest().set_bit()); @@ -235,11 +231,7 @@ impl<'a> Hasher<'a> { // Wait for it! sys_irq_control(self.notification_mask, true); - let _ = sys_recv_closed( - &mut [], - self.notification_mask, - TaskId::KERNEL, - ); + sys_recv_notification(self.notification_mask); // Turn it back off lest it spam us in the future. self.engine.intenclr.write(|w| w.waiting().set_bit()); diff --git a/drv/lpc55-spi-server/src/main.rs b/drv/lpc55-spi-server/src/main.rs index d57372f9ec..7a30e65278 100644 --- a/drv/lpc55-spi-server/src/main.rs +++ b/drv/lpc55-spi-server/src/main.rs @@ -83,11 +83,7 @@ fn main() -> ! { let mut rx_done = false; loop { - if sys_recv_closed(&mut [], notifications::SPI_IRQ_MASK, TaskId::KERNEL) - .is_err() - { - panic!() - } + sys_recv_notification(notifications::SPI_IRQ_MASK); ringbuf_entry!(Trace::Irq); diff --git a/drv/lpc55-sprot-server/src/main.rs b/drv/lpc55-sprot-server/src/main.rs index e3a623bb1c..7c60463929 100644 --- a/drv/lpc55-sprot-server/src/main.rs +++ b/drv/lpc55-sprot-server/src/main.rs @@ -57,7 +57,7 @@ use drv_sprot_api::{ use lpc55_pac as device; use ringbuf::{ringbuf, ringbuf_entry}; use userlib::{ - sys_irq_control, sys_recv_closed, task_slot, TaskId, UnwrapLite, + sys_irq_control, sys_recv_notification, task_slot, TaskId, UnwrapLite, }; mod handler; @@ -216,12 +216,7 @@ impl Io { loop { sys_irq_control(notifications::SPI_IRQ_MASK, true); - sys_recv_closed( - &mut [], - notifications::SPI_IRQ_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SPI_IRQ_MASK); // Is CSn asserted by the SP? let intstat = self.spi.intstat(); diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 06aafa80ce..f0c497876a 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -639,12 +639,7 @@ fn indirect_flash_read_words( flash.enable_interrupt_sources(); sys_irq_control(notifications::FLASH_IRQ_MASK, true); - // RECV from the kernel cannot produce an error, so ignore it. - let _ = sys_recv_closed( - &mut [], - notifications::FLASH_IRQ_MASK, - TaskId::KERNEL, - ); + sys_recv_notification(notifications::FLASH_IRQ_MASK); flash.disable_interrupt_sources(); } } @@ -773,9 +768,7 @@ fn do_block_write( fn wait_for_flash_interrupt() { sys_irq_control(notifications::FLASH_IRQ_MASK, true); - // RECV from the kernel cannot produce an error, so ignore it. - let _ = - sys_recv_closed(&mut [], notifications::FLASH_IRQ_MASK, TaskId::KERNEL); + sys_recv_notification(notifications::FLASH_IRQ_MASK); } fn same_image(which: UpdateTarget) -> bool { diff --git a/drv/psc-seq-server/src/main.rs b/drv/psc-seq-server/src/main.rs index cff7bcf882..59d6e8abfc 100644 --- a/drv/psc-seq-server/src/main.rs +++ b/drv/psc-seq-server/src/main.rs @@ -45,6 +45,6 @@ fn main() -> ! { // We have nothing else to do, so sleep forever via waiting for a message // from the kernel that won't arrive. loop { - _ = sys_recv_closed(&mut [], 0, TaskId::KERNEL); + sys_recv_notification(0); } } diff --git a/drv/stm32h7-eth/src/lib.rs b/drv/stm32h7-eth/src/lib.rs index 2079d09d58..74bfaf24bc 100644 --- a/drv/stm32h7-eth/src/lib.rs +++ b/drv/stm32h7-eth/src/lib.rs @@ -435,11 +435,7 @@ impl Ethernet { // has disabled itself before proceeding. loop { userlib::sys_irq_control(self.mdio_timer_irq_mask, true); - let _ = userlib::sys_recv_closed( - &mut [], - self.mdio_timer_irq_mask, - userlib::TaskId::KERNEL, - ); + userlib::sys_recv_notification(self.mdio_timer_irq_mask); if !self.mdio_timer.cr1.read().cen().bit() { break; } diff --git a/drv/stm32h7-hash/src/lib.rs b/drv/stm32h7-hash/src/lib.rs index 652b25baa5..c82bbe2b1d 100644 --- a/drv/stm32h7-hash/src/lib.rs +++ b/drv/stm32h7-hash/src/lib.rs @@ -311,8 +311,7 @@ impl Hash { hl::sleep_for(1); } } - let _rm = sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); if self.reg.sr.read().dcis().bit() { break; } diff --git a/drv/stm32h7-qspi/src/lib.rs b/drv/stm32h7-qspi/src/lib.rs index efaaa468d1..92253a4e6c 100644 --- a/drv/stm32h7-qspi/src/lib.rs +++ b/drv/stm32h7-qspi/src/lib.rs @@ -15,7 +15,7 @@ use stm32h7::stm32h743 as device; use stm32h7::stm32h753 as device; use drv_qspi_api::Command; -use userlib::{sys_irq_control, sys_recv_closed, TaskId}; +use userlib::{sys_irq_control, sys_recv_notification}; use zerocopy::AsBytes; const FIFO_SIZE: usize = 32; @@ -198,9 +198,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = - sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); if self.reg.sr.read().ftf().bit() { break; } @@ -218,8 +216,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); } self.reg.cr.modify(|_, w| w.tcie().clear_bit()); } @@ -284,9 +281,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = - sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); // Try the check again. We may retry the check on spurious // wakeups, but, spurious wakeups are expected to be pretty @@ -321,8 +316,7 @@ impl Qspi { // Unmask our interrupt. sys_irq_control(self.interrupt, true); // And wait for it to arrive. - let _rm = sys_recv_closed(&mut [], self.interrupt, TaskId::KERNEL) - .unwrap(); + sys_recv_notification(self.interrupt); } // Clean up by disabling our interrupt sources. diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index f240a7b789..b78a949f45 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -549,11 +549,8 @@ impl SpiServerCore { // Allow the controller interrupt to post to our // notification set. sys_irq_control(self.irq_mask, true); - // Wait for our notification set to get, well, set. We ignore - // the result of this because an error would mean the kernel - // violated the ABI, which we can't usefully respond to. - let _ = - sys_recv_closed(&mut [], self.irq_mask, TaskId::KERNEL); + // Wait for our notification set to get, well, set. + sys_recv_notification(self.irq_mask); } } diff --git a/drv/stm32h7-update-server/src/main.rs b/drv/stm32h7-update-server/src/main.rs index bb1ff367ee..a263a5c4a8 100644 --- a/drv/stm32h7-update-server/src/main.rs +++ b/drv/stm32h7-update-server/src/main.rs @@ -264,12 +264,7 @@ impl<'a> ServerImpl<'a> { // Wait for EOP notification via interrupt. loop { - sys_recv_closed( - &mut [], - notifications::FLASH_IRQ_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::FLASH_IRQ_MASK); if self.flash.bank2().sr.read().eop().bit() { break; } else { diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index def47b7cb1..aeebe16793 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -343,17 +343,17 @@ fn main() -> ! { wfi: |notification, timeout| { const TIMER_NOTIFICATION: u32 = 1 << 31; - let dead = sys_get_timer().now.checked_add(timeout.0).unwrap_lite(); + // If the driver passes in a timeout that is large enough that it + // would overflow the kernel's 64-bit timestamp space... well, we'll + // do the best we can without compiling in an unlikely panic. + let dead = sys_get_timer().now.saturating_add(timeout.0); + sys_set_timer(Some(dead), TIMER_NOTIFICATION); - let m = sys_recv_closed( - &mut [], - notification | TIMER_NOTIFICATION, - TaskId::KERNEL, - ) - .unwrap_lite(); + let notification = + sys_recv_notification(notification | TIMER_NOTIFICATION); - if m.operation == TIMER_NOTIFICATION { + if notification == TIMER_NOTIFICATION { I2cControlResult::TimedOut } else { sys_set_timer(None, TIMER_NOTIFICATION); diff --git a/sys/userlib/src/hl.rs b/sys/userlib/src/hl.rs index 00e7be98f5..7fb7b5f6b4 100644 --- a/sys/userlib/src/hl.rs +++ b/sys/userlib/src/hl.rs @@ -7,7 +7,7 @@ //! This is intended to provide a more ergonomic interface than the raw //! syscalls. -use abi::TaskId; +use abi::{Generation, TaskId}; use core::marker::PhantomData; use unwrap_lite::UnwrapLite; use zerocopy::{AsBytes, FromBytes, LayoutVerified}; @@ -147,8 +147,8 @@ where O: FromPrimitive, E: Into, { - let rm = - sys_recv(buffer, mask, source).map_err(|_| ClosedRecvError::Dead)?; + let rm = sys_recv(buffer, mask, source) + .map_err(|code| ClosedRecvError::Dead(Generation::from(code as u8)))?; let sender = rm.sender; if rm.sender == TaskId::KERNEL { notify(state, rm.operation); diff --git a/sys/userlib/src/lib.rs b/sys/userlib/src/lib.rs index c537e3001b..f7858bc48c 100644 --- a/sys/userlib/src/lib.rs +++ b/sys/userlib/src/lib.rs @@ -237,12 +237,14 @@ unsafe extern "C" fn sys_send_stub(_args: &mut SendArgs<'_>) -> RcLen { /// let it, but it always receives _something_. #[inline(always)] pub fn sys_recv_open(buffer: &mut [u8], notification_mask: u32) -> RecvMessage { - // The open-receive version of the syscall is defined as being unable to - // fail, and so we should always get a success here. (This is not using - // `unwrap` because that generates handling code with formatting.) match sys_recv(buffer, notification_mask, None) { Ok(rm) => rm, - Err(_) => panic!(), + Err(_) => { + // Safety: the open-receive version of the syscall is defined as + // being unable to fail in the kernel ABI, so this path can't happen + // modulo a kernel bug. + unsafe { core::hint::unreachable_unchecked() } + } } } @@ -259,20 +261,32 @@ pub fn sys_recv_open(buffer: &mut [u8], notification_mask: u32) -> RecvMessage { /// /// If `sender` is stale (i.e. refers to a deceased generation of the task) when /// you call this, or if `sender` is rebooted while you're blocked in this -/// operation, this will fail with `ClosedRecvError::Dead`. +/// operation, this will fail with `ClosedRecvError::Dead`, indicating the +/// `sender`'s new generation (not that a server generally cares). #[inline(always)] pub fn sys_recv_closed( buffer: &mut [u8], notification_mask: u32, sender: TaskId, ) -> Result { - sys_recv(buffer, notification_mask, Some(sender)) - .map_err(|_| ClosedRecvError::Dead) + sys_recv(buffer, notification_mask, Some(sender)).map_err(|code| { + // We're not using the extract_new_generation function here because + // that has a failure code path for cases where the code is not a + // dead code. In this case, sys_recv is defined as being _only_ + // capable of returning a dead code -- otherwise we have a serious + // kernel bug. So to avoid the introduction of a panic that can't + // trigger, we will do this manually: + ClosedRecvError::Dead(Generation::from(code as u8)) + }) } +/// Things that can go wrong (without faulting) during a closed receive +/// operation. #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub enum ClosedRecvError { - Dead, + /// The task you requested to receive from has restarted and the message may + /// never come. + Dead(Generation), } /// General version of RECV that lets you pick closed vs. open receive at @@ -287,8 +301,9 @@ pub fn sys_recv( ) -> Result { use core::mem::MaybeUninit; - // Flatten option into a packed u32. - let specific_sender = specific_sender + // Flatten option into a packed u32; in the C-compatible ABI we provide the + // task ID in the LSBs, and the "some" flag in the MSB. + let specific_sender_bits = specific_sender .map(|tid| (1u32 << 31) | u32::from(tid.0)) .unwrap_or(0); let mut out = MaybeUninit::::uninit(); @@ -297,7 +312,7 @@ pub fn sys_recv( buffer.as_mut_ptr(), buffer.len(), notification_mask, - specific_sender, + specific_sender_bits, out.as_mut_ptr(), ) }; @@ -319,6 +334,25 @@ pub fn sys_recv( } } +/// Convenience wrapper for `sys_recv` for the specific, but common, task of +/// listening for notifications. In this specific use, it has the advantage of +/// never panicking and not returning a `Result` that must be checked. +#[inline(always)] +pub fn sys_recv_notification(notification_mask: u32) -> u32 { + match sys_recv(&mut [], notification_mask, Some(TaskId::KERNEL)) { + Ok(rm) => { + // The notification bits come back from the kernel in the operation + // code field. + rm.operation + } + Err(_) => { + // Safety: Because we passed Some(TaskId::KERNEL), this is defined + // as not being able to happen. + unsafe { core::hint::unreachable_unchecked() } + } + } +} + pub struct RecvMessage { pub sender: TaskId, pub operation: u32, diff --git a/task/gimlet-inspector/src/main.rs b/task/gimlet-inspector/src/main.rs index fad973364b..228a9132d3 100644 --- a/task/gimlet-inspector/src/main.rs +++ b/task/gimlet-inspector/src/main.rs @@ -99,24 +99,14 @@ fn main() -> ! { // packets off our recv queue at the top of our main // loop. Err(SendError::QueueFull) => { - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } } } } Err(RecvError::QueueEmpty) => { // Our incoming queue is empty. Wait for more packets. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(RecvError::ServerRestarted) => { // `net` restarted; just retry. diff --git a/task/net/src/bsp/gimlet_bcdef.rs b/task/net/src/bsp/gimlet_bcdef.rs index ad9a22c8f6..89909f0d8c 100644 --- a/task/net/src/bsp/gimlet_bcdef.rs +++ b/task/net/src/bsp/gimlet_bcdef.rs @@ -17,7 +17,7 @@ use task_jefe_api::Jefe; use task_net_api::{ ManagementCounters, ManagementLinkStatus, MgmtError, PhyError, }; -use userlib::{sys_recv_closed, FromPrimitive, TaskId}; +use userlib::{sys_recv_notification, FromPrimitive}; use vsc7448_pac::types::PhyRegisterAddress; //////////////////////////////////////////////////////////////////////////////// @@ -72,12 +72,9 @@ impl crate::bsp_support::Bsp for BspImpl { None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], + // Only listen to our Jefe notification. + sys_recv_notification( notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, ); } } diff --git a/task/net/src/bsp/psc_a.rs b/task/net/src/bsp/psc_a.rs index b8ad78eca9..7d05d3d1c8 100644 --- a/task/net/src/bsp/psc_a.rs +++ b/task/net/src/bsp/psc_a.rs @@ -17,7 +17,7 @@ use task_jefe_api::Jefe; use task_net_api::{ ManagementCounters, ManagementLinkStatus, MgmtError, PhyError, }; -use userlib::{sys_recv_closed, FromPrimitive, TaskId}; +use userlib::{sys_recv_notification, FromPrimitive, TaskId}; use vsc7448_pac::types::PhyRegisterAddress; //////////////////////////////////////////////////////////////////////////////// @@ -65,12 +65,9 @@ impl bsp_support::Bsp for BspImpl { Some(PowerState::Init) | None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], + // Only listen to our Jefe notification. + sys_recv_notification( notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, ); } } diff --git a/task/net/src/bsp/psc_bc.rs b/task/net/src/bsp/psc_bc.rs index ad56af1fb0..0c003c31e9 100644 --- a/task/net/src/bsp/psc_bc.rs +++ b/task/net/src/bsp/psc_bc.rs @@ -17,7 +17,7 @@ use task_jefe_api::Jefe; use task_net_api::{ ManagementCounters, ManagementLinkStatus, MgmtError, PhyError, }; -use userlib::{sys_recv_closed, FromPrimitive, TaskId}; +use userlib::{sys_recv_notification, FromPrimitive}; use vsc7448_pac::types::PhyRegisterAddress; //////////////////////////////////////////////////////////////////////////////// @@ -70,12 +70,9 @@ impl bsp_support::Bsp for BspImpl { Some(PowerState::Init) | None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], + // Only listen to our Jefe notification. + sys_recv_notification( notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, ); } } diff --git a/task/nucleo-user-button/src/main.rs b/task/nucleo-user-button/src/main.rs index c05e47a2f0..3b4c131348 100644 --- a/task/nucleo-user-button/src/main.rs +++ b/task/nucleo-user-button/src/main.rs @@ -91,18 +91,7 @@ pub fn main() -> ! { sys.gpio_irq_control(disable_mask, notifications::BUTTON_MASK); // Wait for the user button to be pressed. - // - // We only care about notifications, so we can pass a zero-sized recv - // buffer, and the kernel's task ID. - let recvmsg = sys_recv_closed( - &mut [], - notifications::BUTTON_MASK, - TaskId::KERNEL, - ) - // Recv from the kernel never returns an error. - .unwrap_lite(); - - let notif = recvmsg.operation; + let notif = sys_recv_notification(notifications::BUTTON_MASK); ringbuf_entry!(Trace::Notification(notif)); // If the notification is for the button, toggle the LED. diff --git a/task/sp_measure/src/main.rs b/task/sp_measure/src/main.rs index d2bfdba9e4..d64d69f499 100644 --- a/task/sp_measure/src/main.rs +++ b/task/sp_measure/src/main.rs @@ -70,9 +70,7 @@ fn main() -> ! { // Wait for a notification that will never come, politer than // busy looping forever - if sys_recv_closed(&mut [], 1, TaskId::KERNEL).is_err() { - panic!(); - } + sys_recv_notification(1); } } diff --git a/task/spd/src/main.rs b/task/spd/src/main.rs index 64ad7a2fe6..bbefb6689d 100644 --- a/task/spd/src/main.rs +++ b/task/spd/src/main.rs @@ -31,7 +31,7 @@ use ringbuf::{ringbuf, ringbuf_entry}; use task_jefe_api::Jefe; use task_packrat_api::Packrat; use userlib::{ - sys_irq_control, sys_recv_closed, task_slot, FromPrimitive, TaskId, + sys_irq_control, sys_recv_notification, task_slot, FromPrimitive, }; task_slot!(SYS, sys); @@ -104,13 +104,8 @@ fn main() -> ! { None => { // This happens before we're in a valid power state. // - // Only listen to our Jefe notification. Discard any error - // since this can't fail but the compiler doesn't know that. - let _ = sys_recv_closed( - &mut [], - notifications::JEFE_STATE_CHANGE_MASK, - TaskId::KERNEL, - ); + // Only listen to our Jefe notification. + sys_recv_notification(notifications::JEFE_STATE_CHANGE_MASK); } } } @@ -254,7 +249,7 @@ fn main() -> ! { sys_irq_control(notification, true); }, wfi: |notification| { - let _ = sys_recv_closed(&mut [], notification, TaskId::KERNEL); + sys_recv_notification(notification); }, }; diff --git a/task/uartecho/src/main.rs b/task/uartecho/src/main.rs index 801128b4ff..3a66a175b8 100644 --- a/task/uartecho/src/main.rs +++ b/task/uartecho/src/main.rs @@ -47,11 +47,7 @@ fn main() -> ! { loop { // Wait for uart interrupt; if we haven't enabled tx interrupts, this // blocks until there's data to receive. - let _ = sys_recv_closed( - &mut [], - notifications::USART_IRQ_MASK, - TaskId::KERNEL, - ); + sys_recv_notification(notifications::USART_IRQ_MASK); // Walk through our tx state machine to handle echoing lines back; note // that many of these cases intentionally break after refilling diff --git a/task/udpecho/src/main.rs b/task/udpecho/src/main.rs index 587b975041..69cf047069 100644 --- a/task/udpecho/src/main.rs +++ b/task/udpecho/src/main.rs @@ -38,12 +38,7 @@ fn main() -> ! { Ok(()) => break, Err(SendError::QueueFull) => { // Our outgoing queue is full; wait for space. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(SendError::ServerRestarted) => { // Welp, lost an echo, we'll just soldier on. @@ -53,12 +48,7 @@ fn main() -> ! { } Err(RecvError::QueueEmpty) => { // Our incoming queue is empty. Wait for more packets. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(RecvError::ServerRestarted) => { // `net` restarted, the poor thing; just retry. diff --git a/task/udprpc/src/main.rs b/task/udprpc/src/main.rs index 1edc4c8876..9556470a96 100644 --- a/task/udprpc/src/main.rs +++ b/task/udprpc/src/main.rs @@ -159,24 +159,14 @@ fn main() -> ! { // packets off our recv queue at the top of our main // loop. Err(SendError::QueueFull) => { - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } } } } Err(RecvError::QueueEmpty) => { // Our incoming queue is empty. Wait for more packets. - sys_recv_closed( - &mut [], - notifications::SOCKET_MASK, - TaskId::KERNEL, - ) - .unwrap_lite(); + sys_recv_notification(notifications::SOCKET_MASK); } Err(RecvError::ServerRestarted) => { // `net` restarted (probably due to the watchdog); just retry. diff --git a/test/test-suite/src/main.rs b/test/test-suite/src/main.rs index ab826a9391..bd840a8a56 100644 --- a/test/test-suite/src/main.rs +++ b/test/test-suite/src/main.rs @@ -1171,6 +1171,8 @@ fn test_timer_notify() { let deadline = start_time + 2; sys_set_timer(Some(deadline), ARBITRARY_NOTIFICATION); + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. let rm = sys_recv_closed(&mut [], ARBITRARY_NOTIFICATION, TaskId::KERNEL) .unwrap(); @@ -1193,6 +1195,8 @@ fn test_timer_notify_past() { let deadline = start_time; sys_set_timer(Some(deadline), ARBITRARY_NOTIFICATION); + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. let rm = sys_recv_closed(&mut [], ARBITRARY_NOTIFICATION, TaskId::KERNEL) .unwrap(); @@ -1417,6 +1421,8 @@ fn test_irq_notif() { trigger_test_irq(); + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. let rm = sys_recv_closed(&mut [], notifications::TEST_IRQ_MASK, TaskId::KERNEL) .unwrap(); @@ -1465,6 +1471,9 @@ fn test_irq_status() { // do a `RECV` call to consume the posted notification. Now, we have the // second IRQ pending, and no notification posted. + // + // Deliberately not using the sys_recv_notification convenience operation so + // that we can examine more of the results. sys_recv_closed(&mut [], notifications::TEST_IRQ_MASK, TaskId::KERNEL) .unwrap(); From 58614c07be93b2d6a26ab1d3701982e9b7e5aefa Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 3 Apr 2024 16:05:53 -0400 Subject: [PATCH 16/26] Fix whitespace inconsistency in Idol files (#1705) There should be no non-whitespace changes in this PR! - Always use 4x spaces for indentation - Don't include empty `args: {}` - Don't include a space before `:` or after `(` --- idl/attest.idol | 5 ++- idl/gimlet-seq.idol | 1 + idl/ignition.idol | 2 +- idl/lpc55-pins.idol | 83 ++++++++++++++++++++--------------------- idl/lpc55-update.idol | 41 ++++++++++---------- idl/sbrmi.idol | 12 +++--- idl/sidecar-seq.idol | 2 + idl/sprot.idol | 53 +++++++++++++------------- idl/stm32h7-update.idol | 21 ++++++----- idl/syscon.idol | 67 ++++++++++++++++----------------- idl/validate.idol | 2 +- idl/vpd.idol | 2 +- 12 files changed, 148 insertions(+), 143 deletions(-) diff --git a/idl/attest.idol b/idl/attest.idol index bbfff102c2..c4267a515b 100644 --- a/idl/attest.idol +++ b/idl/attest.idol @@ -16,7 +16,7 @@ Interface( "cert": ( doc: "Get a cert from the RoT-R", args: { - "index" : "u32", + "index" : "u32", "offset" : "u32", }, leases: { @@ -71,6 +71,7 @@ Interface( ), "log_len": ( doc: "Get length of the serialized measurement log", + args: {}, reply: Result( ok: "u32", err: Complex("AttestError"), @@ -80,6 +81,7 @@ Interface( ), "attest": ( doc: "Get an attestation", + args: {}, leases: { "nonce": (type: "[u8]", read: true, max_len: Some(128)), "dest": (type: "[u8]", write: true), @@ -92,6 +94,7 @@ Interface( ), "attest_len": ( doc: "Get the length of an attestation", + args: {}, reply: Result( ok: "u32", err: Complex("AttestError"), diff --git a/idl/gimlet-seq.idol b/idl/gimlet-seq.idol index 321fc19dde..96201f2b79 100644 --- a/idl/gimlet-seq.idol +++ b/idl/gimlet-seq.idol @@ -5,6 +5,7 @@ Interface( ops: { "get_state": ( doc: "Return the power state", + args: {}, reply: Simple(( type: "drv_gimlet_state::PowerState", recv: FromPrimitive("u8"), diff --git a/idl/ignition.idol b/idl/ignition.idol index ca4e16c94f..90b146bbab 100644 --- a/idl/ignition.idol +++ b/idl/ignition.idol @@ -88,7 +88,7 @@ Interface( err: CLike("drv_ignition_api::IgnitionError"), ), ), - "link_events": ( + "link_events": ( doc: "Return all transceiver events for the given port", args: { "port": "u8", diff --git a/idl/lpc55-pins.idol b/idl/lpc55-pins.idol index 390587b324..69e5b66afd 100644 --- a/idl/lpc55-pins.idol +++ b/idl/lpc55-pins.idol @@ -1,46 +1,45 @@ Interface( name: "Pins", ops: { - "iocon_configure_raw": ( - args : { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - "conf": "u32", - }, - reply: Simple("()"), - idempotent: true, - ), - "set_dir": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - "dir": ( type: "Direction", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "set_val": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - "val": ( type : "Value", recv: FromPrimitive("u8")), - }, - reply: Simple("()"), - idempotent: true, - ), - "read_val": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - }, - reply: Simple((type : "Value", recv: FromPrimitive("u8"))), - idempotent: true, - ), - "toggle": ( - args: { - "pin": ( type : "Pin", recv: FromPrimitive("u32")), - }, - reply: Result( - ok: "()", - err: ServerDeath, - ) - ), - - } + "iocon_configure_raw": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + "conf": "u32", + }, + reply: Simple("()"), + idempotent: true, + ), + "set_dir": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + "dir": (type: "Direction", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "set_val": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + "val": (type: "Value", recv: FromPrimitive("u8")), + }, + reply: Simple("()"), + idempotent: true, + ), + "read_val": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + }, + reply: Simple((type: "Value", recv: FromPrimitive("u8"))), + idempotent: true, + ), + "toggle": ( + args: { + "pin": (type: "Pin", recv: FromPrimitive("u32")), + }, + reply: Result( + ok: "()", + err: ServerDeath, + ) + ), + } ) diff --git a/idl/lpc55-update.idol b/idl/lpc55-update.idol index 1bc217c76f..4b2d9d60d9 100644 --- a/idl/lpc55-update.idol +++ b/idl/lpc55-update.idol @@ -5,7 +5,7 @@ Interface( ops: { "block_size": ( doc: "Get the block size for the update API. This is the length expected for the `write_one_block` call", - args: { }, + args: {}, reply: Result( ok: "usize", err: CLike("drv_update_api::UpdateError"), @@ -13,10 +13,10 @@ Interface( ), "prep_image_update": ( doc: "Do any necessary preparation for writing the image. This may include erasing flash and unlocking registers", - args : { + args: { "image_type": "UpdateTarget", }, - reply : Result( + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -25,9 +25,9 @@ Interface( "write_one_block": ( doc: "Write a single block of an update image to the designated location.", args: { - "block_num" : "usize", + "block_num": "usize", }, - leases : { + leases: { "block": (type: "[u8]", read: true, max_len: Some(1024)), }, reply: Result ( @@ -37,31 +37,31 @@ Interface( ), "abort_update": ( doc: "Cancel the current update in progress. Must call prep_image_update again before restarting.", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "finish_image_update": ( doc: "Do any necessary work post image write", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "current_version": ( doc: "Get the current image version", - args : { }, - reply : Simple("ImageVersion"), + args: {}, + reply: Simple("ImageVersion"), idempotent: true, encoding: Hubpack ), "status": ( doc: "Get info about installed images (deprecated - use rot_boot_info)", - args: { }, - reply : Result( + args: {}, + reply: Result( ok: "stage0_handoff::RotBootState", err: Complex("HandoffDataLoadError"), ), @@ -70,8 +70,8 @@ Interface( ), "rot_boot_info": ( doc: "RoT Boot selection and preference info", - args: { }, - reply : Result( + args: {}, + reply: Result( ok: "RotBootInfo", err: CLike("drv_update_api::UpdateError") ), @@ -112,7 +112,7 @@ Interface( "slot": "SlotId", "duration": "SwitchDuration", }, - reply : Result( + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -121,7 +121,8 @@ Interface( ), "reset": ( doc: "Reset unless an update is in progress.", - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -131,9 +132,9 @@ Interface( "read_rot_page": ( doc: "Read CMPA/CFPA page", args: { - "page": "RotPage", - }, - leases : { + "page": "RotPage", + }, + leases: { "data": (type: "[u8]", write: true, max_len: Some(512)), }, reply: Result ( diff --git a/idl/sbrmi.idol b/idl/sbrmi.idol index 43ea7c0938..74f16981f1 100644 --- a/idl/sbrmi.idol +++ b/idl/sbrmi.idol @@ -3,27 +3,27 @@ Interface( name: "Sbrmi", ops: { - "nthreads": ( + "nthreads": ( reply: Result( ok: "u8", err: CLike("SbrmiError"), ), idempotent: true, - ), - "enabled": ( + ), + "enabled": ( reply: Result( ok: "[u8; 16]", err: CLike("SbrmiError"), ), idempotent: true, - ), - "alert": ( + ), + "alert": ( reply: Result( ok: "[u8; 16]", err: CLike("SbrmiError"), ), idempotent: true, - ), + ), "cpuid": ( args: { "thread": "u8", diff --git a/idl/sidecar-seq.idol b/idl/sidecar-seq.idol index e1ce53302b..3e037f520a 100644 --- a/idl/sidecar-seq.idol +++ b/idl/sidecar-seq.idol @@ -29,6 +29,7 @@ Interface( ), "tofino_seq_state": ( doc: "Return the Tofino sequencer state", + args: {}, reply: Result( ok: ( type: "TofinoSeqState", @@ -39,6 +40,7 @@ Interface( ), "tofino_seq_error": ( doc: "Return the Tofino sequencer error, if any", + args: {}, reply: Result( ok: ( type: "TofinoSeqError", diff --git a/idl/sprot.idol b/idl/sprot.idol index 5315b786b0..822646b111 100644 --- a/idl/sprot.idol +++ b/idl/sprot.idol @@ -5,9 +5,9 @@ Interface( ops: { "status": ( doc: "Return status about the sprot protocol", - reply : Result( - ok: "SprotStatus", - err: Complex("SprotError"), + reply: Result( + ok: "SprotStatus", + err: Complex("SprotError"), ), encoding: Hubpack, idempotent: true, @@ -18,7 +18,7 @@ Interface( ok: "SprotIoStats", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, idempotent: true, ), "rot_state": ( @@ -39,18 +39,18 @@ Interface( ok: "PulseStatus", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, ), // The RoT update API is copy and pasted from idl/update.idol. "block_size": ( doc: "Get the block size for the update API. This is the length expected for the `write_one_block` call", - args: { }, + args: {}, reply: Result( ok: "u32", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, idempotent: true, ), "prep_image_update": ( @@ -58,31 +58,31 @@ Interface( args: { "target": "UpdateTarget", }, - reply : Result( + reply: Result( ok: "()", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, idempotent: true, ), "write_one_block": ( doc: "Write a single block of an update image to the designated location.", - args: { - "block_num" : "u32", + args: { + "block_num": "u32", }, - leases : { + leases: { "block": (type: "[u8]", read: true, max_len: Some(512)), }, reply: Result ( ok: "()", err: Complex("SprotError"), ), - encoding: Hubpack, + encoding: Hubpack, ), "abort_update": ( doc: "Cancel the current update in progress. Must call prep_image_update again before restarting.", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -90,8 +90,8 @@ Interface( ), "finish_image_update": ( doc: "Do any necessary work post image write", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -103,7 +103,7 @@ Interface( "slot": "SlotId", "duration": "SwitchDuration", }, - reply : Result( + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -112,7 +112,7 @@ Interface( ), "reset": ( doc: "Reset", - reply : Result( + reply: Result( ok: "()", err: Complex("SprotError"), ), @@ -124,7 +124,7 @@ Interface( args: { "address": "u32", }, - reply : Result( + reply: Result( ok: "()", err: Complex("DumpOrSprotError"), ), @@ -153,7 +153,7 @@ Interface( ok: "()", err: Complex("RawCabooseOrSprotError"), ), - leases : { + leases: { "out": (type: "[u8]", write: true), }, idempotent: true, @@ -180,7 +180,7 @@ Interface( "cert_len": ( doc: "Get length of a cert in the cert chain", args: { - "index" : "u32", + "index": "u32", }, reply: Result( ok: "u32", @@ -192,8 +192,8 @@ Interface( "cert": ( doc: "Get a cert from the alias cert chaing", args: { - "index" : "u32", - "offset" : "u32", + "index": "u32", + "offset": "u32", }, leases: { "dest": (type: "[u8]", write: true), @@ -237,7 +237,7 @@ Interface( "log": ( doc: "Get the measurement log", args: { - "offset" : "u32", + "offset": "u32", }, leases: { "dest": (type: "[u8]", write: true), @@ -259,12 +259,11 @@ Interface( ), "attest": ( doc: "Get an attestation", - args: {}, leases: { "nonce": (type: "[u8]", read: true, max_len: Some(128)), "dest": (type: "[u8]", write: true), }, - reply: Result( + reply: Result( ok: "()", err: Complex("AttestOrSprotError"), ), diff --git a/idl/stm32h7-update.idol b/idl/stm32h7-update.idol index 88f2cca665..0131827410 100644 --- a/idl/stm32h7-update.idol +++ b/idl/stm32h7-update.idol @@ -5,7 +5,7 @@ Interface( ops: { "block_size": ( doc: "Get the block size for the update API. This is the length expected for the `write_one_block` call", - args: { }, + args: {}, reply: Result( ok: "usize", err: CLike("drv_update_api::UpdateError"), @@ -13,7 +13,8 @@ Interface( ), "prep_image_update": ( doc: "Do any necessary preparation for writing the image. This may include erasing flash and unlocking registers", - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), @@ -21,9 +22,9 @@ Interface( "write_one_block": ( doc: "Write a single block of an update image to the designated location.", args: { - "block_num" : "usize", + "block_num": "usize", }, - leases : { + leases: { "block": (type: "[u8]", read: true, max_len: Some(1024)), }, reply: Result ( @@ -33,24 +34,24 @@ Interface( ), "abort_update": ( doc: "Cancel the current update in progress. Must call prep_image_update again before restarting.", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "finish_image_update": ( doc: "Do any necessary work post image write", - args : { }, - reply : Result( + args: {}, + reply: Result( ok: "()", err: CLike("drv_update_api::UpdateError"), ), ), "current_version": ( doc: "Get the current image version", - args : { }, - reply : Simple("ImageVersion"), + args: {}, + reply: Simple("ImageVersion"), idempotent: true, encoding: Hubpack ), diff --git a/idl/syscon.idol b/idl/syscon.idol index 2c7619ac12..1b0437b39c 100644 --- a/idl/syscon.idol +++ b/idl/syscon.idol @@ -1,38 +1,37 @@ Interface( name: "Syscon", ops: { - "enable_clock": ( - args : { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "disable_clock": ( - args: { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "enter_reset": ( - args: { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "leave_reset": ( - args: { - "peripheral": ( type : "Peripheral", recv: FromPrimitive("u32")), - }, - reply: Simple("()"), - idempotent: true, - ), - "chip_reset": ( - reply: Simple("()"), - idempotent: true, - ), - - } + "enable_clock": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "disable_clock": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "enter_reset": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "leave_reset": ( + args: { + "peripheral": (type: "Peripheral", recv: FromPrimitive("u32")), + }, + reply: Simple("()"), + idempotent: true, + ), + "chip_reset": ( + reply: Simple("()"), + idempotent: true, + ), + } ) diff --git a/idl/validate.idol b/idl/validate.idol index 7c233765aa..e1c69a1c3e 100644 --- a/idl/validate.idol +++ b/idl/validate.idol @@ -8,7 +8,7 @@ Interface( "index": "u32", }, reply: Result( - ok: (type : "ValidateOk", recv: FromPrimitive("u8")), + ok: (type: "ValidateOk", recv: FromPrimitive("u8")), err: CLike("ValidateError"), ), idempotent: true, diff --git a/idl/vpd.idol b/idl/vpd.idol index 45c3010b1d..2de32681c3 100644 --- a/idl/vpd.idol +++ b/idl/vpd.idol @@ -54,7 +54,7 @@ Interface( err: CLike("VpdError"), ), ), - "num_vpd_devices": ( + "num_vpd_devices": ( doc: "Returns the total number of VPD devices in the system", args: {}, reply: Simple("usize"), From 4a780d01ebc0ffa399c1523dd2fe078dd89d7877 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Wed, 3 Apr 2024 13:33:46 -0700 Subject: [PATCH 17/26] stm32xx-sys: fix typo in EXTI pin configuration In the Giant Scary Nested Match we had to write to be able to address the EXTI register array as an array through the PAC, I typo'd the number base being used to select the field within the register, by reusing 2 from the line above. This is the _base 2 logarithm_ of the number we actually wanted, 4, because the operations are shift and mod, respectively. Wheeee Anyway, this fixes the situation where EXTI configuration of any pin p such that p % 4 = 2 or 3 on any port other than A mysteriously senses a different pin in port A instead. --- drv/stm32xx-sys/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 22b5bc920e..b46e0975a2 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -398,7 +398,7 @@ fn main() -> ! { // Process entries that are filled in... if let &Some(ExtiDispatch { port, .. }) = entry { let register = i >> 2; - let slot = i % 2; + let slot = i & 0b11; // This is an array of 4-bit fields spread across 4 32-bit // registers. We're indexing them with i. There is really no From 274b51fdaad9df582ad265d9fe98fcef8259dc94 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 2 Apr 2024 15:59:28 -0700 Subject: [PATCH 18/26] Bump idol to get size improvements. --- Cargo.lock | 4 ++-- app/sidecar/base.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c2c131042f..82f1253411 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2652,7 +2652,7 @@ checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" [[package]] name = "idol" version = "0.4.0" -source = "git+https://github.com/oxidecomputer/idolatry.git#070c4dc890f8c3dc26fcadd227316ee2b771e138" +source = "git+https://github.com/oxidecomputer/idolatry.git#b529f7b82face38cfc3851a641277fb7dfbedc4b" dependencies = [ "indexmap 1.9.1", "once_cell", @@ -2669,7 +2669,7 @@ dependencies = [ [[package]] name = "idol-runtime" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/idolatry.git#070c4dc890f8c3dc26fcadd227316ee2b771e138" +source = "git+https://github.com/oxidecomputer/idolatry.git#b529f7b82face38cfc3851a641277fb7dfbedc4b" dependencies = [ "counters", "userlib", diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index e52b5366bc..9fbf42d22b 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -6,7 +6,7 @@ fwid = true [kernel] name = "sidecar" -requires = {flash = 25952, ram = 6256} +requires = {flash = 25984, ram = 6256} features = ["dump"] [caboose] From c464bb33994c69b2a9632ff414b41e5d87fc9115 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 3 Apr 2024 15:34:15 -0700 Subject: [PATCH 19/26] stm32xx-sys: add interrupt acking to `gpio_irq_control` (#1712) Currently, there is no mechanism for a task woken by a notification mapped to an EXTI GPIO interrupt to determine whether the pin it cares about has actually changed state, rather than receiving a notification posted by some other task (see #1700). In order to make this possible, @cbiffle [suggested] that the `stm32xx-sys` task provide an IPC interface that a task receiving an EXTI notification can call to ack that the IPC actually was caused by a pin state transition. This branch extends the `Sys::gpio_irq_control` IPC to return a bool if any EXTI IRQs mapped to the notifications in a task's notification maskhave triggered since the last time `gpio_irq_control` was called. Adding this functionality to `gpio_irq_control` --- which a task that wishes to receive another EXTI notification must call anyway --- allows us to keep the number of IPC roundtrips the same when a task receives such notifications in a loop. In order to support other cases, where a task only cares about one such notification, the `gpio_irq_control` interface has been changed to determine whether to enable or disable the IRQs in the mask based on a second argument, rather than taking two masks as it did previously, in order to allow a third operation which *only* checks the state of the interrupt, leaving it enabled if it has not been triggered but not re-enabling it if it has. We no longer have the ability to both enable and disable disjunct sets of IRQs in one IPC, but in practice, no task we have currently even *uses* more than one GPIO IRQ anyway. And, getting rid of the double-mask interface also fixes #1714. I've implemented this by tracking a 16-bit bitmap in the `Sys` task that mirrors the EXTI pending register. This is because `sys` must clear the pending bit in order to receive another interrupt from _any_ EXTI source. Closes #1700 Closes #1714 [suggested]: https://github.com/oxidecomputer/hubris/issues/1700#issuecomment-2030458073 --- app/demo-stm32h7-nucleo/app-h743.toml | 2 +- app/demo-stm32h7-nucleo/app-h753.toml | 2 +- drv/stm32xx-sys-api/src/lib.rs | 35 ++++ drv/stm32xx-sys/src/main.rs | 266 +++++++++++++++----------- idl/stm32xx-sys.idol | 34 +++- task/nucleo-user-button/src/main.rs | 44 +++-- 6 files changed, 239 insertions(+), 144 deletions(-) diff --git a/app/demo-stm32h7-nucleo/app-h743.toml b/app/demo-stm32h7-nucleo/app-h743.toml index af3ab50143..438955cb6a 100644 --- a/app/demo-stm32h7-nucleo/app-h743.toml +++ b/app/demo-stm32h7-nucleo/app-h743.toml @@ -6,7 +6,7 @@ stacksize = 896 [kernel] name = "demo-stm32h7-nucleo" -requires = {flash = 24000, ram = 5120} +requires = {flash = 24736, ram = 5120} features = ["h743", "dump"] [tasks.jefe] diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index 8c0017f0b3..f88658be38 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -6,7 +6,7 @@ stacksize = 896 [kernel] name = "demo-stm32h7-nucleo" -requires = {flash = 24704, ram = 5120} +requires = {flash = 24736, ram = 5120} features = ["h753", "dump"] [tasks.jefe] diff --git a/drv/stm32xx-sys-api/src/lib.rs b/drv/stm32xx-sys-api/src/lib.rs index d710630c20..5931eb2301 100644 --- a/drv/stm32xx-sys-api/src/lib.rs +++ b/drv/stm32xx-sys-api/src/lib.rs @@ -52,6 +52,25 @@ pub enum Edge { Both = 0b11, } +/// Describes which operation is performed by the [`Sys::gpio_irq_control`] IPC. +#[derive( + Copy, Clone, FromPrimitive, PartialEq, Eq, AsBytes, serde::Deserialize, +)] +// repr attribute is required for the derived `AsBytes` implementation +#[repr(u8)] +pub enum IrqControl { + /// Disable any interrupts mapped to the provided notification mask. + Disable = 0, + /// Enable any interrupts mapped to the provided notification mask. + Enable, + /// Check if any interrupts mapped to the provided notification mask have + /// been triggered, *without* enabling or disabling the interrupt. + /// + /// If an interrupt is currently enabled, it will remain enabled, while if + /// it is currently disabled, it will remain disabled. + Check, +} + impl Sys { /// Requests that the clock to a peripheral be turned on. /// @@ -331,4 +350,20 @@ impl core::ops::BitOr for Edge { } } +impl From for IrqControl { + fn from(value: bool) -> Self { + if value { + IrqControl::Enable + } else { + IrqControl::Disable + } + } +} + +impl From> for IrqControl { + fn from(value: Option) -> Self { + value.map(Self::from).unwrap_or(Self::Check) + } +} + include!(concat!(env!("OUT_DIR"), "/client_stub.rs")); diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index b46e0975a2..96595fd006 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -228,7 +228,7 @@ //! ```rust,no-run //! # fn handle_interrupt() {} //! # mod notifications { pub const MY_GPIO_NOTIFICATION_MASK: u32 = 1 << 0; } -//! use drv_stm32xx_sys_api::{PinSet, Port, Pull, Edge}; +//! use drv_stm32xx_sys_api::{PinSet, Port, Pull, Edge, IrqControl}; //! use userlib::*; //! //! task_slot!(SYS, sys); @@ -250,30 +250,33 @@ //! Edge::Rising, // or `Edge::Falling`, `Edge::Both` //! ); //! +//! // First, enable the interrupt, so that we can receive our +//! // notification. We loop here to retry if the `sys` task has panicked. +//! while sys +//! .gpio_irq_control(notifications::MY_GPIO_NOTIFICATION_MASK, IrqControl::Enable) +//! .is_err() +//! {} +//! //! // Wait to recieve notifications for our GPIO interrupt in a loop: //! loop { -//! // First, enable the interrupt, so that we can receive our -//! // notification: -//! sys.gpio_irq_control(0, notifications::MY_GPIO_NOTIFICATION_MASK); +//! // Wait for a notification. +//! sys_recv_notification(notifications::MY_GPIO_NOTIFICATION_MASK); //! -//! // Wait for a notification: +//! // Call `sys.gpio_irq_control()` to both re-enable the interrupt +//! // and ask the `sys` task to confirm for us that the interrupt has +//! // actually fired. //! // -//! // We only care about notifications, so we can pass a zero-sized -//! // recv buffer, and the kernel's task ID. -//! let recvmsg = sys_recv_closed( -//! &mut [], -//! notifications::BUTTON_MASK, -//! TaskId::KERNEL, -//! ) -//! // Recv from the kernel never returns an error. -//! .unwrap_lite(); -//! -//! // Now, do ... whatever it is this task is supposed to do when -//! // the interrupt fires. -//! handle_interrupt(); -//! -//! // When the loop repeats, the call to `sys.gpio_irq_control()` will -//! // enable the interrupt again. +//! // If we did *not* want to re-enable the interrupt here, we could +//! // pass `IrqControl::Check` rather than `IrqControl::Enable`. +//! let fired = sys +//! .gpio_irq_control(notifications::MY_GPIO_NOTIFICATION_MASK, IrqControl::Enable) +//! // If the `sys` task panicked, just wait for another notification. +//! .unwrap_or(false) +//! if fired { +//! // If the sys task confirms that our interrupt has fired, do... +//! // whatever it is this task is supposed to do when that happens. +//! handle_interrupt(); +//! } //! } //! } //!``` @@ -318,7 +321,7 @@ cfg_if! { } use drv_stm32xx_gpio_common::{server::get_gpio_regs, Port}; -use drv_stm32xx_sys_api::{Edge, Group, RccError}; +use drv_stm32xx_sys_api::{Edge, Group, IrqControl, RccError}; use idol_runtime::{ClientError, NotificationHandler, RequestError}; #[cfg(not(feature = "test"))] use task_jefe_api::{Jefe, ResetReason}; @@ -523,6 +526,9 @@ fn main() -> ! { // which is an operation that can't actually be used to violate Rust // safety. exti: unsafe { &*device::EXTI::ptr() }, + + #[cfg(feature = "exti")] + exti_cpupr_2: 0, }; #[cfg(feature = "exti")] @@ -543,6 +549,15 @@ struct ServerImpl<'a> { /// pin change interrupts. #[cfg(feature = "exti")] exti: &'a device::exti::RegisterBlock, + + /// A bitfield tracking which EXTI interrupts have fired since the last time + /// their owners have called the `gpio_irq_control` IPC. This is necessary + /// as we must unset the sending bit in the *real* EXTI_CPUPR1 register on + /// receipt of an interrupt in order to receive another one, but we must + /// hang onto the pending state until the task that actually uses that + /// interrupt asks us for it. + #[cfg(feature = "exti")] + exti_cpupr_2: u16, } impl ServerImpl<'_> { @@ -649,60 +664,59 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { fn gpio_irq_control( &mut self, rm: &RecvMessage, - disable_mask: u32, - enable_mask: u32, - ) -> Result<(), RequestError> { + mask: u32, + op: IrqControl, + ) -> Result> { // We want to only include code for this if exti is requested. // Unfortunately the _operation_ is available unconditionally, but we'll // fault any clients who call it if it's unsupported (below). cfg_if! { if #[cfg(feature = "exti")] { - - // Keep track of which bits in the caller-provided masks - // actually matched things. - let mut used_bits = 0u32; - - for (i, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { - // Only use populated rows in the table - if let &Some(ExtiDispatch { mask, task, .. }) = entry { - // Ignore anything assigned to another task - if task.index() == rm.sender.index() { - - // Apply disable first so that including the same - // thing in both masks is a no-op. - if mask & disable_mask != 0 { - // Record that these bits meant something. - used_bits |= mask; - - // Disable this source by _clearing_ the - // corresponding mask bit. - self.exti.cpuimr1.modify(|r, w| { - let new_value = r.bits() & !(1 << i); - // Safety: not actually unsafe, PAC didn't - // model this field right - unsafe { - w.bits(new_value) - } - }); - } - - if mask & enable_mask != 0 { - // Record that these bits meant something. - used_bits |= mask; - - // Enable this source by _setting_ the - // corresponding mask bit. - self.exti.cpuimr1.modify(|r, w| { - let new_value = r.bits() | (1 << i); - // Safety: not actually unsafe, PAC didn't - // model this field right - unsafe { - w.bits(new_value) - } - }); - } + // This mask will later be used for checking the stored + // interrupt pending state in `self.exti_cpupr_2` --- we'll put + // a 1 here for the index of every slot that's mapped to a + // notification in the enable/disable masks. + // + // We'll also use the presence of any bits in this mask in order + // to determine whether the caller actually provided masks that + // map to anything interesting, and reply-fault if they + // didn't. + let mut slot_mask = 0u16; + + for (i, _) in exti_dispatch_for(rm.sender, mask) { + // What bit do we touch for this entry? + let bit = 1 << i; + + // Record that these bits meant something. + slot_mask |= bit; + + match op { + IrqControl::Enable => { + // Enable this source by _setting_ the + // corresponding mask bit. + self.exti.cpuimr1.modify(|r, w| { + let new_value = r.bits() | (bit as u32); + // Safety: not actually unsafe, PAC didn't + // model this field right + unsafe { w.bits(new_value) } + }); + }, + IrqControl::Disable => { + // Disable this source by _clearing_ the + // corresponding mask bit. + self.exti.cpuimr1.modify(|r, w| { + let new_value = r.bits() & !(bit as u32); + // Safety: not actually unsafe, PAC didn't + // model this field right + unsafe { + w.bits(new_value) + } + }); + }, + IrqControl::Check => { + // We are just checking if an IRQ has triggered, + // so don't actually mess with the source's mask + // register at all. } } } @@ -719,18 +733,22 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // work around the configuration system to achieve it -- we // don't want to add too much cost to the common case of "no // error." But we also don't want the error to go undetected. - if enable_mask & used_bits != enable_mask - || disable_mask & used_bits != disable_mask - { - Err(ClientError::BadMessageContents.fail()) - } else { - Ok(()) + if slot_mask == 0 { + return Err(ClientError::BadMessageContents.fail()); } + // Check if any interrupts are pending for the slots mapped to + // the caller's notification masks. + let pending = self.exti_cpupr_2 & slot_mask != 0; + // ...and clear those bits for the next interrupt. + self.exti_cpupr_2 &= !slot_mask; + + Ok(pending) + } else { // Suppress unused variable warnings (yay conditional // compilation) - let _ = (rm, enable_mask, disable_mask); + let _ = (rm, mask, op); // Fault any clients who try to use this in an image where it's // not included. @@ -755,45 +773,34 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // actually matched things. let mut used_bits = 0u32; - for (i, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { - // Only use populated rows in the table - if let Some(ExtiDispatch { task, mask: entry_mask, .. }) = entry { - // Operate only on rows assigned to the sending task - // _and_ that are relevant based on the arguments. - if task.index() == rm.sender.index() - && mask & entry_mask != 0 - { - used_bits |= mask; - - // Set or clear Rising Trigger Selection - // Register bit according to the rising flag - self.exti.rtsr1.modify(|r, w| { - let new_value = if edge.is_rising() { - r.bits() | (1 << i) - } else { - r.bits() & !(1 << i) - }; - unsafe { - w.bits(new_value) - } - }); - - // Set or clear Falling Trigger Selection - // Register bit according to the rising flag - self.exti.ftsr1.modify(|r, w| { - let new_value = if edge.is_falling() { - r.bits() | (1 << i) - } else { - r.bits() & !(1 << i) - }; - unsafe { - w.bits(new_value) - } - }); + for (i, entry) in exti_dispatch_for(rm.sender, mask) { + used_bits |= entry.mask; + + // Set or clear Rising Trigger Selection + // Register bit according to the rising flag + self.exti.rtsr1.modify(|r, w| { + let new_value = if edge.is_rising() { + r.bits() | (1 << i) + } else { + r.bits() & !(1 << i) + }; + unsafe { + w.bits(new_value) } - } + }); + + // Set or clear Falling Trigger Selection + // Register bit according to the rising flag + self.exti.ftsr1.modify(|r, w| { + let new_value = if edge.is_falling() { + r.bits() | (1 << i) + } else { + r.bits() & !(1 << i) + }; + unsafe { + w.bits(new_value) + } + }); } // Check that all the set bits in the caller's provided masks @@ -835,6 +842,26 @@ struct ExtiDispatch { name: generated::ExtiIrq, } +/// Iterates over the indices of EXTI sources mapped to the provided +/// notification `mask` for the task with ID `task`. +#[cfg(feature = "exti")] +fn exti_dispatch_for( + task: TaskId, + mask: u32, +) -> impl Iterator { + generated::EXTI_DISPATCH_TABLE + .iter() + .enumerate() + .filter_map(move |(i, entry)| { + let entry = entry.as_ref()?; + if task.index() == entry.task.index() && mask & entry.mask != 0 { + Some((i, entry)) + } else { + None + } + }) +} + impl NotificationHandler for ServerImpl<'_> { fn current_notification_mask(&self) -> u32 { cfg_if! { @@ -900,6 +927,11 @@ impl NotificationHandler for ServerImpl<'_> { } if bits_to_acknowledge != 0 { + // Save pending bits so that when the tasks that own the + // interrupt(s) that fired call `Sys.gpio_irq_control` to + // check if their IRQs fired, we'll be able to tell them. + self.exti_cpupr_2 |= bits_to_acknowledge; + // Mask and unpend interrupts en masse to save like six // cycles because we'll totally notice in practice // @@ -1185,7 +1217,7 @@ cfg_if! { include!(concat!(env!("OUT_DIR"), "/notifications.rs")); mod idl { - use super::{Edge, Port, RccError}; + use super::{Edge, IrqControl, Port, RccError}; include!(concat!(env!("OUT_DIR"), "/server_stub.rs")); } diff --git a/idl/stm32xx-sys.idol b/idl/stm32xx-sys.idol index 48a50bfc3d..d24cbbf298 100644 --- a/idl/stm32xx-sys.idol +++ b/idl/stm32xx-sys.idol @@ -111,18 +111,34 @@ Interface( idempotent: true, ), - // Changes a subset of EXTI sources mapped to the calling - // task, using the caller's notification bit space to - // name them. Any sources mapped to bits in the disable - // mask will be disabled; any sources mapped to bits in - // the enabled mask will be enabled. + // Performs an operation on a subset of EXTI sources mapped to the + // calling task, using the caller's notification bit space to name + // them, and returns whether any interrupts mapped to the provided + // notification bits have been triggered. + // + // Depending on the value of the `op` argument, this operation can + // either enable the intterrupts mapped to the notification mask + // (if `op` is `IrqControl::Enable`), disable those interrupts (if + // `op` is `IrqControl::Disable`), or do neither (if `op` is + // `IrqControl::Check`). Regardless of which operation is performed, + // this IPC will always return `true` if any interrupt in the + // provided notification mask has been triggered since the last time + // this IPC was called, and resets this status for the next call to + // this IPC. If an interrupt was enabled when this IPC is called with + // `IrqControl::Check` as the `op`, that interrupt will remain + // enabled, and if an interrupt was disabled, it will remain disabled. "gpio_irq_control": ( args: { - "disable_mask": "u32", - "enable_mask": "u32", + "mask": "u32", + "op": ( + type: "IrqControl", + recv: FromPrimitive("u8"), + ), }, - reply: Simple("()"), - idempotent: true, + reply: Result( + ok: "bool", + err: ServerDeath, + ), ), }, ) diff --git a/task/nucleo-user-button/src/main.rs b/task/nucleo-user-button/src/main.rs index 3b4c131348..ae3acf667d 100644 --- a/task/nucleo-user-button/src/main.rs +++ b/task/nucleo-user-button/src/main.rs @@ -11,7 +11,7 @@ #![no_std] #![no_main] -use drv_stm32xx_sys_api::{Edge, PinSet, Port, Pull}; +use drv_stm32xx_sys_api::{Edge, IrqControl, PinSet, Port, Pull}; use ringbuf::ringbuf_entry; use userlib::*; @@ -43,8 +43,14 @@ enum Trace { /// We called the `Sys.gpio_irq_configure` IPC with these arguments. GpioIrqConfigure { mask: u32, edge: Edge }, - /// We called the `Sys.gpio_irq_control` IPC with these arguments. - GpioIrqControl { enable_mask: u32, disable_mask: u32 }, + /// We called the `Sys.gpio_irq_control` IPC with these arguments, and it + /// returned whether the interrupt had fired or not. + GpioIrqControl { + mask: u32, + op: IrqControl, + #[count(children)] + fired: bool, + }, /// We received a notification with these bits set. Notification(u32), @@ -80,25 +86,31 @@ pub fn main() -> ! { sys.gpio_irq_configure(notifications::BUTTON_MASK, edge); loop { - // The first argument to `gpio_irq_control` is the mask of interrupts to - // disable, while the second is the mask to enable. So, enable the - // button notification. - let disable_mask = 0; + // Call `Sys.gpio_irq_control` to enable our interrupt, returning + // whether it has fired. + let fired = match sys + .gpio_irq_control(notifications::BUTTON_MASK, IrqControl::Enable) + { + Ok(fired) => fired, + // If the sys task panicked, okay, let's just try to enable the IRQ + // again. + Err(_) => continue, + }; ringbuf_entry!(Trace::GpioIrqControl { - enable_mask: notifications::BUTTON_MASK, - disable_mask, + mask: notifications::BUTTON_MASK, + op: IrqControl::Enable, + fired }); - sys.gpio_irq_control(disable_mask, notifications::BUTTON_MASK); - // Wait for the user button to be pressed. - let notif = sys_recv_notification(notifications::BUTTON_MASK); - ringbuf_entry!(Trace::Notification(notif)); - - // If the notification is for the button, toggle the LED. - if notif == notifications::BUTTON_MASK { + // If the button has changed state, toggle the LED. + if fired { ringbuf_entry!(Trace::LedToggle { led }); user_leds.led_toggle(led).unwrap_lite(); } + + // Wait for the user button to be pressed. + let notif = sys_recv_notification(notifications::BUTTON_MASK); + ringbuf_entry!(Trace::Notification(notif)); } } From e66e3278d2b92f268b3988f1bd7b0fa677ce9ac8 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 2 Apr 2024 15:58:41 -0700 Subject: [PATCH 20/26] stm32xx-sys: statically ensure absence of panics At least in some configurations. --- app/demo-stm32h7-nucleo/app-h743.toml | 2 +- app/demo-stm32h7-nucleo/app-h753.toml | 2 +- app/gimlet/base.toml | 2 +- app/psc/base.toml | 2 +- app/sidecar/base.toml | 2 +- drv/stm32xx-sys/Cargo.toml | 1 + drv/stm32xx-sys/src/main.rs | 64 ++++++++++++++++++--------- 7 files changed, 49 insertions(+), 26 deletions(-) diff --git a/app/demo-stm32h7-nucleo/app-h743.toml b/app/demo-stm32h7-nucleo/app-h743.toml index 438955cb6a..c8b9c9c708 100644 --- a/app/demo-stm32h7-nucleo/app-h743.toml +++ b/app/demo-stm32h7-nucleo/app-h743.toml @@ -25,7 +25,7 @@ request_reset = ["hiffy"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h743", "exti"] +features = ["h743", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index f88658be38..efa28c1584 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -25,7 +25,7 @@ request_reset = ["hiffy"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753", "exti"] +features = ["h753", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 0649f968ea..df66586908 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -50,7 +50,7 @@ notifications = ["eth-irq", "mdio-timer-irq", "wake-timer", "jefe-state-change"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753", "exti"] +features = ["h753", "exti", "no-panic"] priority = 1 uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"] start = true diff --git a/app/psc/base.toml b/app/psc/base.toml index 8679a610ee..050c16fd07 100644 --- a/app/psc/base.toml +++ b/app/psc/base.toml @@ -34,7 +34,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753"] +features = ["h753", "no-panic"] priority = 1 max-sizes = {flash = 2048, ram = 2048} uses = ["rcc", "gpios", "system_flash"] diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index 9fbf42d22b..98cb98c8ba 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -31,7 +31,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.sys] name = "drv-stm32xx-sys" -features = ["h753"] +features = ["h753", "no-panic"] priority = 1 max-sizes = {flash = 2048, ram = 2048} uses = ["rcc", "gpios", "system_flash"] diff --git a/drv/stm32xx-sys/Cargo.toml b/drv/stm32xx-sys/Cargo.toml index 9ed1aee85f..706b2c6fca 100644 --- a/drv/stm32xx-sys/Cargo.toml +++ b/drv/stm32xx-sys/Cargo.toml @@ -37,6 +37,7 @@ g070 = ["family-stm32g0", "stm32g0/stm32g070", "drv-stm32xx-sys-api/g070", "drv- g0b1 = ["family-stm32g0", "stm32g0/stm32g0b1", "drv-stm32xx-sys-api/g0b1", "drv-stm32xx-gpio-common/model-stm32g0b1"] no-ipc-counters = ["idol/no-counters"] +no-panic = ["userlib/no-panic"] # Enable external interrupt controller support. exti = ["dep:hubris-num-tasks", "dep:counters"] diff --git a/drv/stm32xx-sys/src/main.rs b/drv/stm32xx-sys/src/main.rs index 96595fd006..8e7849c9fb 100644 --- a/drv/stm32xx-sys/src/main.rs +++ b/drv/stm32xx-sys/src/main.rs @@ -397,7 +397,7 @@ fn main() -> ! { // API the way we use peripherals. let syscfg = unsafe { &*device::SYSCFG::ptr() }; - for (i, entry) in generated::EXTI_DISPATCH_TABLE.iter().enumerate() { + for (i, entry) in dispatch_table_iter() { // Process entries that are filled in... if let &Some(ExtiDispatch { port, .. }) = entry { let register = i >> 2; @@ -684,8 +684,10 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { let mut slot_mask = 0u16; for (i, _) in exti_dispatch_for(rm.sender, mask) { - // What bit do we touch for this entry? - let bit = 1 << i; + // What bit do we touch for this entry? (Mask is to ensure + // that the compiler understands this shift cannot + // overflow.) + let bit = 1 << (i & 0xF); // Record that these bits meant something. slot_mask |= bit; @@ -774,15 +776,19 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { let mut used_bits = 0u32; for (i, entry) in exti_dispatch_for(rm.sender, mask) { + // (Mask is to ensure that the compiler understands this + // shift cannot overflow.) + let imask = 1 << (i & 0xF); + used_bits |= entry.mask; // Set or clear Rising Trigger Selection // Register bit according to the rising flag self.exti.rtsr1.modify(|r, w| { let new_value = if edge.is_rising() { - r.bits() | (1 << i) + r.bits() | imask } else { - r.bits() & !(1 << i) + r.bits() & !imask }; unsafe { w.bits(new_value) @@ -793,9 +799,9 @@ impl idl::InOrderSysImpl for ServerImpl<'_> { // Register bit according to the rising flag self.exti.ftsr1.modify(|r, w| { let new_value = if edge.is_falling() { - r.bits() | (1 << i) + r.bits() | imask } else { - r.bits() & !(1 << i) + r.bits() & !imask }; unsafe { w.bits(new_value) @@ -849,17 +855,18 @@ fn exti_dispatch_for( task: TaskId, mask: u32, ) -> impl Iterator { - generated::EXTI_DISPATCH_TABLE - .iter() - .enumerate() - .filter_map(move |(i, entry)| { - let entry = entry.as_ref()?; - if task.index() == entry.task.index() && mask & entry.mask != 0 { - Some((i, entry)) - } else { - None - } - }) + // This is semantically equivalent to iter.enumerate, but winds up handing + // the compiler very different code that avoids an otherwise-difficult panic + // site on an apparently-overflowing addition (that was not actually capable + // of overflowing). + dispatch_table_iter().filter_map(move |(i, entry)| { + let entry = entry.as_ref()?; + if task.index() == entry.task.index() && mask & entry.mask != 0 { + Some((i, entry)) + } else { + None + } + }) } impl NotificationHandler for ServerImpl<'_> { @@ -898,9 +905,13 @@ impl NotificationHandler for ServerImpl<'_> { let mut bits_to_acknowledge = 0u16; - for (pin_idx, entry) in - generated::EXTI_DISPATCH_TABLE.iter().enumerate() - { + for pin_idx in 0..16 { + // TODO: this sure looks like it should be using + // iter.enumerate, doesn't it? Unfortunately that's not + // currently getting inlined by rustc, resulting in rather + // silly code containing panics. This is significantly + // smaller. + let entry = &generated::EXTI_DISPATCH_TABLE[pin_idx]; if pending_and_enabled & 1 << pin_idx != 0 { // A channel is pending! We need to handle this // basically like the kernel handles native hardware @@ -1214,6 +1225,17 @@ cfg_if! { } } +#[cfg(feature = "exti")] +#[inline(always)] +fn dispatch_table_iter( +) -> impl Iterator)> { + // TODO: this sure looks like it should be using iter.enumerate, doesn't it? + // Unfortunately that's not currently getting inlined by rustc, resulting in + // rather silly code containing panics. This is significantly smaller. + (0..generated::EXTI_DISPATCH_TABLE.len()) + .zip(&generated::EXTI_DISPATCH_TABLE) +} + include!(concat!(env!("OUT_DIR"), "/notifications.rs")); mod idl { From e4316f0c3ff51631f03953f97e524c7a5acfa654 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Mon, 1 Apr 2024 15:46:16 -0700 Subject: [PATCH 21/26] Bump the toolchain to latest nightly. Also fixed _some_ new warnings reported by the toolchain, and many places where rustfmt has apparently changed its mind about some things. Removed size constraints for tasks that increased in size rather than playing guess-the-number. This leaves a number of warnings, which I'm hoping to divide up and not have to fix myself. --- .cargo/config | 2 - .cargo/config.toml | 10 ++ Cargo.toml | 2 - app/gimlet/base.toml | 2 +- app/oxide-rot-1/app.toml | 1 - app/sidecar/base.toml | 2 +- build/i2c/src/lib.rs | 21 +-- build/util/src/lib.rs | 2 +- build/xtask/src/caboose_pos.rs | 6 +- build/xtask/src/config.rs | 4 - build/xtask/src/lsp.rs | 6 +- build/xtask/src/sizes.rs | 8 +- drv/gimlet-seq-server/build.rs | 2 +- drv/i2c-devices/src/at24csw080.rs | 1 - drv/i2c-devices/src/ds2482.rs | 2 + drv/i2c-devices/src/max31790.rs | 1 - drv/sidecar-front-io/src/transceivers.rs | 4 +- .../src/tofino2.rs | 1 - drv/sidecar-seq-server/src/tofino.rs | 1 - drv/stm32h7-eth/src/lib.rs | 1 - drv/stm32h7-hash/src/lib.rs | 1 - drv/stm32h7-spi-server-core/src/lib.rs | 2 + drv/stm32h7-sprot-server/src/main.rs | 1 - drv/stm32xx-i2c-server/src/main.rs | 5 +- drv/stm32xx-i2c/src/ltc4306.rs | 5 +- drv/stm32xx-i2c/src/max7358.rs | 5 +- drv/stm32xx-sys-api/src/h7.rs | 4 +- drv/vsc85xx/src/vsc8562.rs | 2 - lib/gnarle/src/lib.rs | 2 - lib/stage0-handoff/src/lib.rs | 2 - rust-toolchain.toml | 2 +- sys/kern/src/fail.rs | 12 +- sys/kern/src/kipc.rs | 1 - sys/kern/src/startup.rs | 27 ++-- sys/kern/src/syscalls.rs | 2 - sys/kern/src/task.rs | 1 - sys/userlib/src/kipc.rs | 4 +- sys/userlib/src/lib.rs | 2 +- sys/userlib/src/units.rs | 1 - task/hiffy/src/main.rs | 3 + task/hiffy/src/stm32h7.rs | 4 +- task/host-sp-comms/src/bsp/gimlet_bcde.rs | 127 +++++++++++------- task/net/src/bsp/gimlet_bcdef.rs | 1 - task/net/src/bsp/gimletlet_mgmt.rs | 1 - task/net/src/bsp/psc_a.rs | 1 - task/net/src/bsp/psc_bc.rs | 1 - task/net/src/bsp/sidecar_bcd.rs | 1 - task/net/src/mgmt.rs | 3 - task/power/src/bsp/gimlet_bcdef.rs | 4 +- task/sensor-api/src/lib.rs | 3 - task/thermal/src/bsp/gimlet_bcdef.rs | 1 - task/thermal/src/bsp/sidecar_bcd.rs | 1 - task/thermal/src/control.rs | 1 + task/thermal/src/main.rs | 1 - 54 files changed, 164 insertions(+), 149 deletions(-) delete mode 100644 .cargo/config create mode 100644 .cargo/config.toml diff --git a/.cargo/config b/.cargo/config deleted file mode 100644 index 35049cbcb1..0000000000 --- a/.cargo/config +++ /dev/null @@ -1,2 +0,0 @@ -[alias] -xtask = "run --package xtask --" diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 0000000000..0e8ce01fb4 --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,10 @@ +[alias] +xtask = "run --package xtask --" + +[build] +# The purpose of this flag is to block crates using version_detect from "detecting" +# features that are no longer supported by the toolchain, because despite its name, +# version_detect is basically "if nightly { return true; }". This setting gets +# overridden within xtask for Hubris programs, so this only affects host tools like +# xtask. +rustflags = ["-Zallow-features=proc_macro_diagnostic,asm_const,naked_functions,used_with_arg"] diff --git a/Cargo.toml b/Cargo.toml index 1874287818..f8b52c3a09 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,5 +1,3 @@ -cargo-features = ["resolver", "named-profiles"] - [workspace] members = [ "build/*", diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index df66586908..b987f314ae 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -123,7 +123,7 @@ features = ["gimlet"] name = "task-thermal" features = ["gimlet"] priority = 5 -max-sizes = {flash = 32768, ram = 8192 } +max-sizes = {flash = 65536, ram = 8192 } stacksize = 6000 start = true task-slots = ["i2c_driver", "sensor", "gimlet_seq", "jefe"] diff --git a/app/oxide-rot-1/app.toml b/app/oxide-rot-1/app.toml index 9d20f495c5..cf17974ad7 100644 --- a/app/oxide-rot-1/app.toml +++ b/app/oxide-rot-1/app.toml @@ -68,7 +68,6 @@ task-slots = ["syscon_driver"] [tasks.sprot] name = "drv-lpc55-sprot-server" priority = 6 -max-sizes = {flash = 48608, ram = 32768} uses = ["flexcomm8", "bootrom"] features = ["spi0"] start = true diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index 98cb98c8ba..2b8c77afcc 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -252,7 +252,7 @@ notifications = ["timer"] name = "task-thermal" features = ["sidecar"] priority = 5 -max-sizes = {flash = 32768, ram = 16384 } +max-sizes = {flash = 65536, ram = 16384 } stacksize = 8096 start = true task-slots = ["i2c_driver", "sensor", "sequencer"] diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index 0e33f24490..ea3a2303ca 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -269,21 +269,6 @@ struct DeviceRefdesKey { kind: Sensor, } -#[derive(Debug, PartialEq, Eq, Hash)] -struct DeviceBusKey { - device: String, - bus: String, - kind: Sensor, -} - -#[derive(Debug, PartialEq, Eq, Hash)] -struct DeviceBusNameKey { - device: String, - bus: String, - name: String, - kind: Sensor, -} - #[derive(Debug, Clone, PartialEq, Eq)] pub struct DeviceSensor { pub name: Option, @@ -573,7 +558,7 @@ impl ConfigGenerator { d.device, d.address ); } - (_, Some(bus)) if buses.get(bus).is_none() => { + (_, Some(bus)) if !buses.contains_key(bus) => { panic!( "device {} at address {:#x} specifies \ unknown bus \"{}\"", @@ -1254,7 +1239,7 @@ impl ConfigGenerator { // returned by `device_descriptions()` below: if we change the ordering // here, it must be updated there as well. for (index, device) in self.devices.iter().enumerate() { - if drivers.get(&device.device).is_some() { + if drivers.contains(&device.device) { let driver = device.device.to_case(Case::UpperCamel); let out = self.generate_device(device, 24); @@ -1471,7 +1456,7 @@ impl ConfigGenerator { { writeln!( &mut self.output, - "\n #[allow(non_camel_case_types)] + "\n #[allow(non_camel_case_types, dead_code)] pub struct Sensors_{struct_name} {{", )?; let mut f = |name, count| match count { diff --git a/build/util/src/lib.rs b/build/util/src/lib.rs index 0fea1a1ec5..c4f0626759 100644 --- a/build/util/src/lib.rs +++ b/build/util/src/lib.rs @@ -262,7 +262,7 @@ pub fn build_notifications() -> Result<()> { ); } if full_task_config.name == "task-jefe" - && full_task_config.notifications.get(0).cloned() + && full_task_config.notifications.first().cloned() != Some("fault".to_string()) { bail!("`jefe` must have \"fault\" as its first notification"); diff --git a/build/xtask/src/caboose_pos.rs b/build/xtask/src/caboose_pos.rs index 836547b768..bbaf5c0a65 100644 --- a/build/xtask/src/caboose_pos.rs +++ b/build/xtask/src/caboose_pos.rs @@ -52,9 +52,9 @@ pub fn get_caboose_pos_table_entry( ) -> Result> { // If the section isn't present, then we're not reading the caboose position // from this task. - let Some(caboose_pos_table_section) = elf::get_section_by_name( - elf, CABOOSE_POS_TABLE_SECTION - ) else { + let Some(caboose_pos_table_section) = + elf::get_section_by_name(elf, CABOOSE_POS_TABLE_SECTION) + else { return Ok(None); }; diff --git a/build/xtask/src/config.rs b/build/xtask/src/config.rs index e5e4d0ecff..bca20d971b 100644 --- a/build/xtask/src/config.rs +++ b/build/xtask/src/config.rs @@ -33,8 +33,6 @@ struct RawConfig { #[serde(default)] image_names: Vec, #[serde(default)] - external_images: Vec, - #[serde(default)] signing: Option, stacksize: Option, kernel: Kernel, @@ -56,7 +54,6 @@ pub struct Config { pub version: u32, pub fwid: bool, pub image_names: Vec, - pub external_images: Vec, pub signing: Option, pub stacksize: Option, pub kernel: Kernel, @@ -176,7 +173,6 @@ impl Config { target: toml.target, board: toml.board, image_names: img_names, - external_images: toml.external_images, chip: toml.chip, epoch: toml.epoch, version: toml.version, diff --git a/build/xtask/src/lsp.rs b/build/xtask/src/lsp.rs index 4c8ea8a8ea..43bc0455e0 100644 --- a/build/xtask/src/lsp.rs +++ b/build/xtask/src/lsp.rs @@ -114,7 +114,9 @@ impl PackageGraph { while let Some((pkg_name, feat)) = todo.pop() { // Anything not in `packages` is something from outside the // workspace, so we don't care about it. - let Some(pkg) = self.0.get(&pkg_name) else { continue }; + let Some(pkg) = self.0.get(&pkg_name) else { + continue; + }; // If we've never seen this package before, then insert all of // its non-optional dependencies with their features. @@ -277,7 +279,7 @@ fn check_task( extra_env: build_cfg.env, hash: "".to_owned(), build_override_command, - app: app_name.clone().to_owned(), + app: app_name.to_owned(), task: task_name.to_owned(), }; diff --git a/build/xtask/src/sizes.rs b/build/xtask/src/sizes.rs index 6970c74db5..d28f318359 100644 --- a/build/xtask/src/sizes.rs +++ b/build/xtask/src/sizes.rs @@ -118,7 +118,7 @@ pub fn run( #[derive(Copy, Clone, Debug)] enum Recommended { - FixedSize(u32), + FixedSize, MaxSize(u32), } #[derive(Clone, Debug)] @@ -183,7 +183,7 @@ fn build_memory_map<'a>( .get(mem_name.to_owned()) .cloned() .map(match name { - "kernel" => Recommended::FixedSize, + "kernel" => |_| Recommended::FixedSize, _ => Recommended::MaxSize, }), }, @@ -265,7 +265,7 @@ fn print_task_table( match chunk.recommended { None => print!("(auto)"), Some(Recommended::MaxSize(m)) => print!("{}", m), - Some(Recommended::FixedSize(_)) => print!("(fixed)"), + Some(Recommended::FixedSize) => print!("(fixed)"), } println!(); } @@ -359,7 +359,7 @@ fn print_memory_map( match chunk.recommended { None => print!("(auto)"), Some(Recommended::MaxSize(m)) => print!("{}", m), - Some(Recommended::FixedSize(_)) => print!("(fixed)"), + Some(Recommended::FixedSize) => print!("(fixed)"), } } println!(); diff --git a/drv/gimlet-seq-server/build.rs b/drv/gimlet-seq-server/build.rs index 210947845b..2f15ecc2db 100644 --- a/drv/gimlet-seq-server/build.rs +++ b/drv/gimlet-seq-server/build.rs @@ -5,7 +5,7 @@ use build_fpga_regmap::fpga_regs; use serde::Deserialize; use sha2::Digest; -use std::{convert::TryInto, fs, io::Write, path::PathBuf}; +use std::{fs, io::Write, path::PathBuf}; #[derive(Deserialize)] #[serde(deny_unknown_fields)] diff --git a/drv/i2c-devices/src/at24csw080.rs b/drv/i2c-devices/src/at24csw080.rs index 710ff08648..c8adb1ca91 100644 --- a/drv/i2c-devices/src/at24csw080.rs +++ b/drv/i2c-devices/src/at24csw080.rs @@ -5,7 +5,6 @@ //! Driver for the AT24CSW080/4 I2C EEPROM use crate::Validate; -use core::convert::TryInto; use drv_i2c_api::*; use userlib::{hl::sleep_for, FromPrimitive, ToPrimitive}; use zerocopy::{AsBytes, FromBytes}; diff --git a/drv/i2c-devices/src/ds2482.rs b/drv/i2c-devices/src/ds2482.rs index fbd80cc548..225cac9136 100644 --- a/drv/i2c-devices/src/ds2482.rs +++ b/drv/i2c-devices/src/ds2482.rs @@ -201,6 +201,8 @@ impl Ds2482 { pub fn search(&mut self) -> Result, Error> { let device = &self.device; + // TODO: lint is buggy in 2024-04-04 toolchain, retest later. + #[allow(clippy::manual_unwrap_or_default)] let branches = match self.branches { Some(branches) => { if branches.0 == 0 { diff --git a/drv/i2c-devices/src/max31790.rs b/drv/i2c-devices/src/max31790.rs index d3966e637c..5eaffe4c5d 100644 --- a/drv/i2c-devices/src/max31790.rs +++ b/drv/i2c-devices/src/max31790.rs @@ -6,7 +6,6 @@ use crate::Validate; use bitfield::bitfield; -use core::convert::TryFrom; use drv_i2c_api::*; use ringbuf::*; use userlib::units::*; diff --git a/drv/sidecar-front-io/src/transceivers.rs b/drv/sidecar-front-io/src/transceivers.rs index 692545ac7e..678d090d34 100644 --- a/drv/sidecar-front-io/src/transceivers.rs +++ b/drv/sidecar-front-io/src/transceivers.rs @@ -848,7 +848,9 @@ impl Transceivers { FpgaController::Left => ldata, FpgaController::Right => rdata, }; - let Some(local_data) = local_data else { continue }; + let Some(local_data) = local_data else { + continue; + }; // loop through the 8 different fields we need to map for (word, out) in local_data.iter().zip(status_masks.iter_mut()) { diff --git a/drv/sidecar-mainboard-controller/src/tofino2.rs b/drv/sidecar-mainboard-controller/src/tofino2.rs index f40779bf99..3a1dc57d0f 100644 --- a/drv/sidecar-mainboard-controller/src/tofino2.rs +++ b/drv/sidecar-mainboard-controller/src/tofino2.rs @@ -4,7 +4,6 @@ use crate::{Addr, MainboardController, Reg}; use bitfield::bitfield; -use core::convert::Into; use derive_more::{From, Into}; use drv_fpga_api::{FpgaError, FpgaUserDesign, WriteOp}; use drv_fpga_user_api::power_rail::*; diff --git a/drv/sidecar-seq-server/src/tofino.rs b/drv/sidecar-seq-server/src/tofino.rs index 5510343e44..9eb3910e43 100644 --- a/drv/sidecar-seq-server/src/tofino.rs +++ b/drv/sidecar-seq-server/src/tofino.rs @@ -4,7 +4,6 @@ use crate::*; use drv_i2c_devices::raa229618::Raa229618; -use drv_sidecar_mainboard_controller::tofino2::{DebugPort, Sequencer}; pub(crate) struct Tofino { pub policy: TofinoSequencerPolicy, diff --git a/drv/stm32h7-eth/src/lib.rs b/drv/stm32h7-eth/src/lib.rs index 74bfaf24bc..615dff53d0 100644 --- a/drv/stm32h7-eth/src/lib.rs +++ b/drv/stm32h7-eth/src/lib.rs @@ -12,7 +12,6 @@ #![no_std] -use core::convert::TryFrom; use core::sync::atomic::{self, Ordering}; #[cfg(feature = "h743")] diff --git a/drv/stm32h7-hash/src/lib.rs b/drv/stm32h7-hash/src/lib.rs index c82bbe2b1d..c67d136834 100644 --- a/drv/stm32h7-hash/src/lib.rs +++ b/drv/stm32h7-hash/src/lib.rs @@ -29,7 +29,6 @@ use drv_hash_api::HashError; #[cfg(feature = "h753")] use stm32h7::stm32h753 as device; -use core::convert::TryInto; use core::mem::size_of; use userlib::*; use zerocopy::AsBytes; diff --git a/drv/stm32h7-spi-server-core/src/lib.rs b/drv/stm32h7-spi-server-core/src/lib.rs index b78a949f45..b2d5a89940 100644 --- a/drv/stm32h7-spi-server-core/src/lib.rs +++ b/drv/stm32h7-spi-server-core/src/lib.rs @@ -465,6 +465,8 @@ impl SpiServerCore { // run off the end of their lease, or the fixed padding byte if // we have. let byte = if let Some(txbuf) = &mut tx { + // TODO: lint is buggy in 2024-04-04 toolchain, retest later + #[allow(clippy::manual_unwrap_or_default)] if let Some(b) = txbuf.read() { b } else { diff --git a/drv/stm32h7-sprot-server/src/main.rs b/drv/stm32h7-sprot-server/src/main.rs index 2bbbf253ae..5425be86a7 100644 --- a/drv/stm32h7-sprot-server/src/main.rs +++ b/drv/stm32h7-sprot-server/src/main.rs @@ -7,7 +7,6 @@ #![deny(elided_lifetimes_in_paths)] use attest_api::{AttestError, HashAlgorithm, NONCE_MAX_SIZE, NONCE_MIN_SIZE}; -use core::convert::Into; use drv_lpc55_update_api::{ RotBootInfo, RotPage, SlotId, SwitchDuration, UpdateTarget, }; diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index aeebe16793..a38be81a72 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -451,7 +451,7 @@ fn main() -> ! { let mut nread = 0; - match controller.write_read( + let controller_result = controller.write_read( addr, winfo.len, |pos| wbuf.read_at(pos), @@ -470,7 +470,8 @@ fn main() -> ! { rbuf.write_at(pos, byte) }, &ctrl, - ) { + ); + match controller_result { Err(code) => { // // NoDevice errors aren't hugely interesting -- diff --git a/drv/stm32xx-i2c/src/ltc4306.rs b/drv/stm32xx-i2c/src/ltc4306.rs index e56fe33170..82f38b64f3 100644 --- a/drv/stm32xx-i2c/src/ltc4306.rs +++ b/drv/stm32xx-i2c/src/ltc4306.rs @@ -93,7 +93,7 @@ fn read_reg_u8( let mut rval = 0u8; let wlen = 1; - match controller.write_read( + let controller_result = controller.write_read( mux.address, wlen, |_| Some(reg), @@ -103,7 +103,8 @@ fn read_reg_u8( Some(()) }, ctrl, - ) { + ); + match controller_result { Err(code) => Err(mux.error_code(code)), _ => Ok(rval), } diff --git a/drv/stm32xx-i2c/src/max7358.rs b/drv/stm32xx-i2c/src/max7358.rs index a0d60330c8..7822342d23 100644 --- a/drv/stm32xx-i2c/src/max7358.rs +++ b/drv/stm32xx-i2c/src/max7358.rs @@ -76,7 +76,7 @@ fn read_regs( rbuf: &mut [u8], ctrl: &I2cControl, ) -> Result<(), ResponseCode> { - match controller.write_read( + let controller_result = controller.write_read( mux.address, 0, |_| Some(0), @@ -86,7 +86,8 @@ fn read_regs( Some(()) }, ctrl, - ) { + ); + match controller_result { Err(code) => Err(mux.error_code(code)), _ => { for (i, &byte) in rbuf.iter().enumerate() { diff --git a/drv/stm32xx-sys-api/src/h7.rs b/drv/stm32xx-sys-api/src/h7.rs index 57559dd254..a8da1b764f 100644 --- a/drv/stm32xx-sys-api/src/h7.rs +++ b/drv/stm32xx-sys-api/src/h7.rs @@ -89,9 +89,9 @@ pub enum Peripheral { #[cfg(any(feature = "h753", feature = "h743"))] Rng = periph(Group::Ahb2, 6), - #[cfg(any(feature = "h753"))] + #[cfg(feature = "h753")] Hash = periph(Group::Ahb2, 5), - #[cfg(any(feature = "h753"))] + #[cfg(feature = "h753")] Crypt = periph(Group::Ahb2, 4), #[cfg(feature = "h7b3")] diff --git a/drv/vsc85xx/src/vsc8562.rs b/drv/vsc85xx/src/vsc8562.rs index ab667c1cd8..fe2104844c 100644 --- a/drv/vsc85xx/src/vsc8562.rs +++ b/drv/vsc85xx/src/vsc8562.rs @@ -2,8 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use core::convert::TryInto; - use zerocopy::{AsBytes, FromBytes}; use crate::{Phy, PhyRw, Trace}; diff --git a/lib/gnarle/src/lib.rs b/lib/gnarle/src/lib.rs index 76a5c46b3a..dff3600ed4 100644 --- a/lib/gnarle/src/lib.rs +++ b/lib/gnarle/src/lib.rs @@ -11,8 +11,6 @@ #![cfg_attr(not(feature = "std"), no_std)] -use core::convert::TryFrom; - /// Internal definition of how long the run count is. Tuning this might improve /// performance, though its current value seems optimal in practice. type RunType = u8; diff --git a/lib/stage0-handoff/src/lib.rs b/lib/stage0-handoff/src/lib.rs index 4d9a5d4183..19afd239d1 100644 --- a/lib/stage0-handoff/src/lib.rs +++ b/lib/stage0-handoff/src/lib.rs @@ -4,8 +4,6 @@ #![cfg_attr(not(test), no_std)] -use core::convert::From; -use core::marker::Sized; use core::ops::Range; use hubpack::SerializedSize; use serde::{Deserialize, Serialize}; diff --git a/rust-toolchain.toml b/rust-toolchain.toml index 832531aca7..38cefdfc1e 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,5 +1,5 @@ [toolchain] -channel = "nightly-2022-11-01" +channel = "nightly-2024-04-04" targets = [ "thumbv6m-none-eabi", "thumbv7em-none-eabihf", "thumbv8m.main-none-eabihf" ] profile = "minimal" components = [ "rustfmt" ] diff --git a/sys/kern/src/fail.rs b/sys/kern/src/fail.rs index a27f762a5f..444de67674 100644 --- a/sys/kern/src/fail.rs +++ b/sys/kern/src/fail.rs @@ -42,8 +42,14 @@ static mut KERNEL_EPITAPH: [u8; EPITAPH_LEN] = [0; EPITAPH_LEN]; #[cfg(not(feature = "nano"))] fn begin_epitaph() -> &'static mut [u8; EPITAPH_LEN] { // We'd love to use an AtomicBool here but we gotta support ARMv6M. - let previous_fail = - core::mem::replace(unsafe { &mut KERNEL_HAS_FAILED }, true); + // This could probably become SyncUnsafeCell in a future where it exists. + // + // Safety: we only access this function from this one site, and only zero or + // one times in practice -- and never from a context where concurrency or + // interrupts are enabled. + let previous_fail = unsafe { + core::ptr::replace(core::ptr::addr_of_mut!(KERNEL_HAS_FAILED), true) + }; if previous_fail { // Welp, you've called begin_epitaph twice, suggesting a recursive // panic. We can't very well panic in response to this since it'll just @@ -56,7 +62,7 @@ fn begin_epitaph() -> &'static mut [u8; EPITAPH_LEN] { // Safety: we can get a mutable reference to the epitaph because only one // execution of this function will successfully set that flag. - unsafe { &mut KERNEL_EPITAPH } + unsafe { &mut *core::ptr::addr_of_mut!(KERNEL_EPITAPH) } } #[cfg(not(feature = "nano"))] diff --git a/sys/kern/src/kipc.rs b/sys/kern/src/kipc.rs index 6dbd2227d4..5e6f6fc501 100644 --- a/sys/kern/src/kipc.rs +++ b/sys/kern/src/kipc.rs @@ -10,7 +10,6 @@ use crate::arch; use crate::err::UserError; use crate::task::{current_id, ArchState, NextTask, Task}; use crate::umem::USlice; -use core::convert::TryFrom; use core::mem::size_of; /// Message dispatcher. diff --git a/sys/kern/src/startup.rs b/sys/kern/src/startup.rs index af5bc1bf26..1c60948052 100644 --- a/sys/kern/src/startup.rs +++ b/sys/kern/src/startup.rs @@ -5,7 +5,7 @@ //! Kernel startup. use crate::atomic::AtomicExt; -use crate::descs::RegionDesc; +use crate::descs::*; use crate::task::Task; use core::mem::MaybeUninit; use core::sync::atomic::{AtomicBool, Ordering}; @@ -51,7 +51,8 @@ pub unsafe fn start_kernel(tick_divisor: u32) -> ! { let task_descs = &HUBRIS_TASK_DESCS; // Safety: this reference will remain unique so long as the "only called // once per boot" contract on this function is upheld. - let task_table = unsafe { &mut HUBRIS_TASK_TABLE_SPACE }; + let task_table = + unsafe { &mut *core::ptr::addr_of_mut!(HUBRIS_TASK_TABLE_SPACE) }; // Initialize our RAM data structures. @@ -100,18 +101,27 @@ pub(crate) fn with_task_table(body: impl FnOnce(&mut [Task]) -> R) -> R { if TASK_TABLE_IN_USE.swap_polyfill(true, Ordering::Acquire) { panic!(); // recursive use of with_task_table } + // Safety: tbh it's never been clear to me why addr_of_mut is unsafe when it + // produces a raw pointer, but, see the Safety justification on the + // derefrence below for the full argument on why we can do this. + let task_table: *mut MaybeUninit<[Task; HUBRIS_TASK_COUNT]> = + unsafe { core::ptr::addr_of_mut!(HUBRIS_TASK_TABLE_SPACE) }; + // Pointer cast valid as MaybeUninit<[T; N]> and [MaybeUninit; N] have + // same in-memory representation. At the time of this writing + // MaybeUninit::transpose is not yet stable. + let task_table: *mut [MaybeUninit; HUBRIS_TASK_COUNT] = + task_table as _; + // This pointer cast is doing the equivalent of + // MaybeUninit::array_assume_init, which at the time of this writing is not + // stable. + let task_table: *mut [Task; HUBRIS_TASK_COUNT] = task_table as _; // Safety: we have observed `TASK_TABLE_IN_USE` being false, which means the // task table is initialized (note that at reset it starts out true) and // that we're not already within a call to with_task_table. Thus, we can // produce a reference to the task table without aliasing, and we can be // confident that the memory it's pointing to is initialized and shed the // MaybeUninit. - let task_table = unsafe { - &mut *(&mut HUBRIS_TASK_TABLE_SPACE - as *mut MaybeUninit<[Task; HUBRIS_TASK_COUNT]> - as *mut [MaybeUninit; HUBRIS_TASK_COUNT] - as *mut [Task; HUBRIS_TASK_COUNT]) - }; + let task_table = unsafe { &mut *task_table }; let r = body(task_table); @@ -120,5 +130,4 @@ pub(crate) fn with_task_table(body: impl FnOnce(&mut [Task]) -> R) -> R { r } -use crate::descs::*; include!(concat!(env!("OUT_DIR"), "/kconfig.rs")); diff --git a/sys/kern/src/syscalls.rs b/sys/kern/src/syscalls.rs index 77211d63c3..5137e5619e 100644 --- a/sys/kern/src/syscalls.rs +++ b/sys/kern/src/syscalls.rs @@ -27,8 +27,6 @@ //! struct* type to make this easy and safe, e.g. `task.save().as_send_args()`. //! See the `task::ArchState` trait for details. -use core::convert::TryFrom; - use abi::{ FaultInfo, IrqStatus, LeaseAttributes, SchedState, Sysnum, TaskId, TaskState, ULease, UsageError, diff --git a/sys/kern/src/task.rs b/sys/kern/src/task.rs index 8cf0ac0c3f..09caee26ed 100644 --- a/sys/kern/src/task.rs +++ b/sys/kern/src/task.rs @@ -4,7 +4,6 @@ //! Implementation of tasks. -use core::convert::TryFrom; use core::ops::Range; use abi::{ diff --git a/sys/userlib/src/kipc.rs b/sys/userlib/src/kipc.rs index 42e7c5d60f..b153ca2913 100644 --- a/sys/userlib/src/kipc.rs +++ b/sys/userlib/src/kipc.rs @@ -4,10 +4,10 @@ //! Operations implemented by IPC with the kernel task. -use crate::UnwrapLite; +use abi::{Kipcnum, TaskId}; use zerocopy::AsBytes; -use crate::*; +use crate::{sys_send, UnwrapLite}; pub fn read_task_status(task: usize) -> abi::TaskState { // Coerce `task` to a known size (Rust doesn't assume that usize == u32) diff --git a/sys/userlib/src/lib.rs b/sys/userlib/src/lib.rs index f7858bc48c..4c7011a18b 100644 --- a/sys/userlib/src/lib.rs +++ b/sys/userlib/src/lib.rs @@ -1461,7 +1461,7 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { // However, it is possible to produce an alias if the panic handler is // called reentrantly. This can only happen if the code in the panic handler // itself panics, which is what we're working very hard to prevent here. - let panic_buffer = unsafe { &mut PANIC_BUFFER }; + let panic_buffer = unsafe { &mut *core::ptr::addr_of_mut!(PANIC_BUFFER) }; // Whew! Time to write the darn message. // diff --git a/sys/userlib/src/units.rs b/sys/userlib/src/units.rs index cf832313eb..ed3aea16f9 100644 --- a/sys/userlib/src/units.rs +++ b/sys/userlib/src/units.rs @@ -6,7 +6,6 @@ //! Tuple structs for units that are useful in the real world //! -use core::convert::TryFrom; use zerocopy::{AsBytes, FromBytes}; /// Degrees Celsius diff --git a/task/hiffy/src/main.rs b/task/hiffy/src/main.rs index ff35cbe9b9..50eec44c7d 100644 --- a/task/hiffy/src/main.rs +++ b/task/hiffy/src/main.rs @@ -166,6 +166,9 @@ fn main() -> ! { sleep_ms = 1; sleeps = 0; + // TODO without a safety comment explaining why these are safe, it is + // not clear if this is sound, do _not_ "fix" this by slapping on an + // addr_of_mut! without further analysis! let text = unsafe { &HIFFY_TEXT }; let data = unsafe { &HIFFY_DATA }; let rstack = unsafe { &mut HIFFY_RSTACK[0..] }; diff --git a/task/hiffy/src/stm32h7.rs b/task/hiffy/src/stm32h7.rs index 76397da842..007454c8da 100644 --- a/task/hiffy/src/stm32h7.rs +++ b/task/hiffy/src/stm32h7.rs @@ -47,7 +47,9 @@ enum Trace { ringbuf!(Trace, 64, Trace::None); -pub struct Buffer(u8); +// Field is only used in the debugger, appears dead to the compiler. +pub struct Buffer(#[allow(dead_code)] u8); + // // The order in this enum must match the order in the functions array that // is passed to execute. diff --git a/task/host-sp-comms/src/bsp/gimlet_bcde.rs b/task/host-sp-comms/src/bsp/gimlet_bcde.rs index 92d07cfdae..c2c18718d9 100644 --- a/task/host-sp-comms/src/bsp/gimlet_bcde.rs +++ b/task/host-sp-comms/src/bsp/gimlet_bcde.rs @@ -82,8 +82,9 @@ impl ServerImpl { let packrat = &self.packrat; let mut data = InventoryData::VpdIdentity(Default::default()); self.tx_buf.try_encode_inventory(sequence, b"U615/ID", || { - let InventoryData::VpdIdentity(identity) = &mut data - else { unreachable!(); }; + let InventoryData::VpdIdentity(identity) = &mut data else { + unreachable!(); + }; *identity = packrat .get_identity() .map_err(|_| InventoryDataResult::DeviceAbsent)? @@ -184,18 +185,21 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { use pmbus::commands::bmr491::CommandCode; let InventoryData::Bmr491 { - mfr_id, - mfr_model, - mfr_revision, - mfr_location, - mfr_date, - mfr_serial, - mfr_firmware_data, - temp_sensor: _, - voltage_sensor: _, - current_sensor: _, - power_sensor: _, - } = &mut data else { unreachable!() }; + mfr_id, + mfr_model, + mfr_revision, + mfr_location, + mfr_date, + mfr_serial, + mfr_firmware_data, + temp_sensor: _, + voltage_sensor: _, + current_sensor: _, + power_sensor: _, + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -234,15 +238,18 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { use pmbus::commands::isl68224::CommandCode; let InventoryData::Isl68224 { - mfr_id, - mfr_model, - mfr_revision, - mfr_date, - ic_device_id, - ic_device_rev, - voltage_sensors: _, - current_sensors: _, - } = &mut data else { unreachable!() }; + mfr_id, + mfr_model, + mfr_revision, + mfr_date, + ic_device_id, + ic_device_rev, + voltage_sensors: _, + current_sensors: _, + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -286,17 +293,20 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { use pmbus::commands::raa229618::CommandCode; let InventoryData::Raa229618 { - mfr_id, - mfr_model, - mfr_revision, - mfr_date, - ic_device_id, - ic_device_rev, - temp_sensors: _, - power_sensors: _, - voltage_sensors: _, - current_sensors: _, - } = &mut data else { unreachable!() }; + mfr_id, + mfr_model, + mfr_revision, + mfr_date, + ic_device_id, + ic_device_rev, + temp_sensors: _, + power_sensors: _, + voltage_sensors: _, + current_sensors: _, + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -351,7 +361,10 @@ impl ServerImpl { temp_sensor: _, voltage_sensor: _, current_sensor: _, - } = &mut data else { unreachable!() }; + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -403,7 +416,10 @@ impl ServerImpl { temp_sensor: _, voltage_sensor: _, current_sensor: _, - } = &mut data else { unreachable!() }; + } = &mut data + else { + unreachable!() + }; dev.read_block(CommandCode::MFR_ID as u8, mfr_id)?; dev.read_block(CommandCode::MFR_MODEL as u8, mfr_model)?; dev.read_block( @@ -447,8 +463,11 @@ impl ServerImpl { eeprom1, eeprom2, eeprom3, - temp_sensor: _ - } = &mut data else { unreachable!(); }; + temp_sensor: _, + } = &mut data + else { + unreachable!(); + }; *id = dev.read_reg(0x0Fu8)?; *eeprom1 = dev.read_reg(0x05u8)?; *eeprom2 = dev.read_reg(0x06u8)?; @@ -474,7 +493,10 @@ impl ServerImpl { minor_rel, hotfix_rel, product_id, - } = &mut data else { unreachable!(); }; + } = &mut data + else { + unreachable!(); + }; // This chip includes a separate register that controls the // upper address byte, i.e. a paged memory implementation. // We'll use `write_read_reg` to avoid the possibility of @@ -509,8 +531,9 @@ impl ServerImpl { let ksz8463 = ksz8463::Ksz8463::new(ksz8463_dev); let mut data = InventoryData::Ksz8463 { cider: 0 }; self.tx_buf.try_encode_inventory(sequence, b"U401", || { - let InventoryData::Ksz8463 { cider } = &mut data - else { unreachable!(); }; + let InventoryData::Ksz8463 { cider } = &mut data else { + unreachable!(); + }; *cider = ksz8463 .read(ksz8463::Register::CIDER) .map_err(|_| InventoryDataResult::DeviceFailed)?; @@ -600,8 +623,9 @@ impl ServerImpl { self.tx_buf.try_encode_inventory(sequence, &name, || { // TODO: does packrat index match PCA designator? if packrat.get_spd_present(index as usize) { - let InventoryData::DimmSpd { id, .. } = &mut data - else { unreachable!(); }; + let InventoryData::DimmSpd { id, .. } = &mut data else { + unreachable!(); + }; packrat.get_full_spd_data(index as usize, id); Ok(&data) } else { @@ -626,8 +650,9 @@ impl ServerImpl { *data = InventoryData::At24csw08xSerial([0u8; 16]); let dev = At24Csw080::new(f(I2C.get_task_id())); self.tx_buf.try_encode_inventory(sequence, name, || { - let InventoryData::At24csw08xSerial(id) = data - else { unreachable!(); }; + let InventoryData::At24csw08xSerial(id) = data else { + unreachable!(); + }; for (i, b) in id.iter_mut().enumerate() { *b = dev.read_security_register_byte(i as u8).map_err(|e| { match e { @@ -656,8 +681,9 @@ impl ServerImpl { let dev = f(I2C.get_task_id()); let mut data = InventoryData::VpdIdentity(Default::default()); self.tx_buf.try_encode_inventory(sequence, name, || { - let InventoryData::VpdIdentity(identity) = &mut data - else { unreachable!(); }; + let InventoryData::VpdIdentity(identity) = &mut data else { + unreachable!(); + }; *identity = read_one_barcode(dev, &[(*b"BARC", 0)])?.into(); Ok(&data) }) @@ -689,8 +715,11 @@ impl ServerImpl { let InventoryData::FanIdentity { identity, vpd_identity, - fans: [fan0, fan1, fan2] - } = &mut data else { unreachable!(); }; + fans: [fan0, fan1, fan2], + } = &mut data + else { + unreachable!(); + }; *identity = read_one_barcode(dev, &[(*b"BARC", 0)])?.into(); *vpd_identity = read_one_barcode(dev, &[(*b"SASY", 0), (*b"BARC", 0)])?.into(); diff --git a/task/net/src/bsp/gimlet_bcdef.rs b/task/net/src/bsp/gimlet_bcdef.rs index 89909f0d8c..6859b3e9ba 100644 --- a/task/net/src/bsp/gimlet_bcdef.rs +++ b/task/net/src/bsp/gimlet_bcdef.rs @@ -90,7 +90,6 @@ impl crate::bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(10).and_pin(12)), slow_power_en: false, power_good: &[], // TODO - pll_lock: None, // TODO? ksz8463: Ksz8463::new(ksz8463_dev), diff --git a/task/net/src/bsp/gimletlet_mgmt.rs b/task/net/src/bsp/gimletlet_mgmt.rs index d2457804ea..a6c6829ed5 100644 --- a/task/net/src/bsp/gimletlet_mgmt.rs +++ b/task/net/src/bsp/gimletlet_mgmt.rs @@ -154,7 +154,6 @@ impl bsp_support::Bsp for BspImpl { power_en: None, slow_power_en: false, power_good: &[], - pll_lock: None, ksz8463: Ksz8463::new(ksz8463_dev), ksz8463_nrst: Port::A.pin(9), diff --git a/task/net/src/bsp/psc_a.rs b/task/net/src/bsp/psc_a.rs index 7d05d3d1c8..e020fcbb71 100644 --- a/task/net/src/bsp/psc_a.rs +++ b/task/net/src/bsp/psc_a.rs @@ -83,7 +83,6 @@ impl bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(10).and_pin(12)), slow_power_en: true, power_good: &[], // TODO - pll_lock: None, ksz8463: Ksz8463::new(ksz8463_dev), ksz8463_nrst: Port::C.pin(2), diff --git a/task/net/src/bsp/psc_bc.rs b/task/net/src/bsp/psc_bc.rs index 0c003c31e9..aa91a4f995 100644 --- a/task/net/src/bsp/psc_bc.rs +++ b/task/net/src/bsp/psc_bc.rs @@ -87,7 +87,6 @@ impl bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(10)), slow_power_en: false, power_good: &PG_PINS, - pll_lock: None, ksz8463: Ksz8463::new(ksz8463_dev), ksz8463_nrst: Port::C.pin(2), // SP_TO_MGMT_SW_RESET_L diff --git a/task/net/src/bsp/sidecar_bcd.rs b/task/net/src/bsp/sidecar_bcd.rs index 39ffb58330..6d7be18133 100644 --- a/task/net/src/bsp/sidecar_bcd.rs +++ b/task/net/src/bsp/sidecar_bcd.rs @@ -75,7 +75,6 @@ impl bsp_support::Bsp for BspImpl { power_en: Some(Port::I.pin(11)), slow_power_en: false, power_good: &[], // TODO - pll_lock: None, // TODO? ksz8463: Ksz8463::new(ksz8463_dev), // SP_TO_EPE_RESET_L diff --git a/task/net/src/mgmt.rs b/task/net/src/mgmt.rs index c3a3724ac4..e8c2219427 100644 --- a/task/net/src/mgmt.rs +++ b/task/net/src/mgmt.rs @@ -58,9 +58,6 @@ pub struct Config { /// Goes high once power is good pub power_good: &'static [sys_api::PinSet], - /// Goes high once the PLLs are locked - pub pll_lock: Option, - pub ksz8463: Ksz8463, pub ksz8463_nrst: sys_api::PinSet, pub ksz8463_rst_type: Ksz8463ResetSpeed, diff --git a/task/power/src/bsp/gimlet_bcdef.rs b/task/power/src/bsp/gimlet_bcdef.rs index e6c6071900..c737eb0d9f 100644 --- a/task/power/src/bsp/gimlet_bcdef.rs +++ b/task/power/src/bsp/gimlet_bcdef.rs @@ -3,8 +3,8 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::{ - i2c_config, i2c_config::sensors, Device, DeviceType, Ohms, - PowerControllerConfig, PowerState, SensorId, + i2c_config, i2c_config::sensors, Device, DeviceType, PowerControllerConfig, + PowerState, SensorId, }; use drv_i2c_devices::max5970::*; diff --git a/task/sensor-api/src/lib.rs b/task/sensor-api/src/lib.rs index 837441f00f..9f9917adc2 100644 --- a/task/sensor-api/src/lib.rs +++ b/task/sensor-api/src/lib.rs @@ -8,13 +8,10 @@ use userlib::*; -use core::convert::TryFrom; - use derive_idol_err::IdolError; use drv_i2c_api::ResponseCode; use hubpack::SerializedSize; use num_derive::FromPrimitive; -use num_traits::FromPrimitive; use serde::{Deserialize, Serialize}; /// A validated sensor ID. diff --git a/task/thermal/src/bsp/gimlet_bcdef.rs b/task/thermal/src/bsp/gimlet_bcdef.rs index 94afb990c6..3f3d11ca14 100644 --- a/task/thermal/src/bsp/gimlet_bcdef.rs +++ b/task/thermal/src/bsp/gimlet_bcdef.rs @@ -11,7 +11,6 @@ use crate::{ }, i2c_config::{devices, sensors}, }; -use core::convert::TryInto; pub use drv_gimlet_seq_api::SeqError; use drv_gimlet_seq_api::{PowerState, Sequencer}; use drv_i2c_devices::max31790::Max31790; diff --git a/task/thermal/src/bsp/sidecar_bcd.rs b/task/thermal/src/bsp/sidecar_bcd.rs index 4b51d4ea04..38809c0d74 100644 --- a/task/thermal/src/bsp/sidecar_bcd.rs +++ b/task/thermal/src/bsp/sidecar_bcd.rs @@ -8,7 +8,6 @@ use crate::control::{ ChannelType, Device, FanControl, Fans, InputChannel, PidConfig, TemperatureSensor, }; -use core::convert::TryInto; use drv_i2c_devices::max31790::Max31790; use drv_i2c_devices::tmp451::*; pub use drv_sidecar_seq_api::SeqError; diff --git a/task/thermal/src/control.rs b/task/thermal/src/control.rs index 414de8829d..d91c214a61 100644 --- a/task/thermal/src/control.rs +++ b/task/thermal/src/control.rs @@ -242,6 +242,7 @@ pub(crate) struct DynamicInputChannel { //////////////////////////////////////////////////////////////////////////////// #[derive(Copy, Clone)] +#[allow(dead_code)] // used only by the debugger pub struct TimestampedSensorError { pub timestamp: u64, pub id: SensorId, diff --git a/task/thermal/src/main.rs b/task/thermal/src/main.rs index e112da87d9..ed6fe2e10d 100644 --- a/task/thermal/src/main.rs +++ b/task/thermal/src/main.rs @@ -37,7 +37,6 @@ use crate::{ bsp::{Bsp, PowerBitmask, SeqError}, control::ThermalControl, }; -use core::convert::TryFrom; use drv_i2c_api::ResponseCode; use drv_i2c_devices::max31790::I2cWatchdog; use idol_runtime::{NotificationHandler, RequestError}; From 14f4170cb016926f57c70ccddd247a7050b53475 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 10:56:36 -0700 Subject: [PATCH 22/26] Temporarily* disable clippy *famous last words --- .github/workflows/build-one.yml | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build-one.yml b/.github/workflows/build-one.yml index 8f8b22b279..895de6393f 100644 --- a/.github/workflows/build-one.yml +++ b/.github/workflows/build-one.yml @@ -79,10 +79,14 @@ jobs: target/release/humility -a target/${{ inputs.app_name }}/dist/build-${{ inputs.app_name }}-image-$image.zip manifest; \ done - - name: Clippy - if: inputs.os == 'ubuntu-latest' - run: | - cargo xtask clippy ${{ inputs.app_toml}} -- --deny warnings + # TODO: Clippy temporarily disabled 2024-04 while people clean up + # the new Clippy warnings resulting from the last toolchain + # upgrade. If you're reading this, try turning it back on and see + # if we're good yet. + #- name: Clippy + # if: inputs.os == 'ubuntu-latest' + # run: | + # cargo xtask clippy ${{ inputs.app_toml}} -- --deny warnings # upload the output of our build - name: Upload build archive From 366f42504811667804061adbedd4dc0a070827b8 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 14:00:26 -0700 Subject: [PATCH 23/26] thermal: remove use of f32::clamp for now This was adding 20 kiB (!) to the task because (1) it wasn't getting inlined and (2) its non-inlined implementation contains a panic that pulls in f32 formatting code, which has recently gotten really expensive. --- app/gimlet/base.toml | 2 +- app/sidecar/base.toml | 2 +- task/thermal/src/control.rs | 7 +++++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index b987f314ae..df66586908 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -123,7 +123,7 @@ features = ["gimlet"] name = "task-thermal" features = ["gimlet"] priority = 5 -max-sizes = {flash = 65536, ram = 8192 } +max-sizes = {flash = 32768, ram = 8192 } stacksize = 6000 start = true task-slots = ["i2c_driver", "sensor", "gimlet_seq", "jefe"] diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index 2b8c77afcc..98cb98c8ba 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -252,7 +252,7 @@ notifications = ["timer"] name = "task-thermal" features = ["sidecar"] priority = 5 -max-sizes = {flash = 65536, ram = 16384 } +max-sizes = {flash = 32768, ram = 16384 } stacksize = 8096 start = true task-slots = ["i2c_driver", "sensor", "sequencer"] diff --git a/task/thermal/src/control.rs b/task/thermal/src/control.rs index d91c214a61..9afa4a2ea6 100644 --- a/task/thermal/src/control.rs +++ b/task/thermal/src/control.rs @@ -419,11 +419,14 @@ impl OneSidedPidState { } else { (-out_pd, output_limit - out_pd) }; - self.integral = self.integral.clamp(integral_min, integral_max); + // f32::clamp is not inlining well as of 2024-04 so we do it by hand + // here and below. + self.integral = self.integral.max(integral_min).min(integral_max); // Clamp output values to valid range. let out = out_pd + self.integral; - out.clamp(0.0, output_limit) + // same issue with f32::clamp (above) + out.max(0.0).min(output_limit) } } From a326ff5d7377b29f628952e471d00d4bcea1672a Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 12:06:02 -0700 Subject: [PATCH 24/26] build/i2c: generate range matches We've noticed the compiler tends to generate better code for integer range matches of the form match x { 0..=7 => a, 7..=10 => b, _ => c, } as compared to match x { 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 => a, // and so on } Also, Clippy in recent nightly has started objecting to the "cascade of digits" form. Rather than suppressing that warning, I thought I'd convert the biggest matches in our codegen output to use ranges. This commit uses rangemap to automatically consolidate adjacent ranges of integers into a convenient match statement. I've also factored out the code from the two places it was used. --- Cargo.lock | 1 + build/i2c/Cargo.toml | 1 + build/i2c/src/lib.rs | 63 ++++++++++++++++++++++++-------------------- 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 82f1253411..6898539039 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -243,6 +243,7 @@ dependencies = [ "convert_case", "indexmap 1.9.1", "multimap", + "rangemap", "serde", ] diff --git a/build/i2c/Cargo.toml b/build/i2c/Cargo.toml index 941b80a746..7604d4b1ce 100644 --- a/build/i2c/Cargo.toml +++ b/build/i2c/Cargo.toml @@ -12,6 +12,7 @@ convert_case = { workspace = true } indexmap = { workspace = true } multimap = { workspace = true } serde = { workspace = true } +rangemap.workspace = true [features] h743 = [] diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index ea3a2303ca..9442301680 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -6,6 +6,7 @@ use anyhow::{bail, Context, Result}; use convert_case::{Case, Casing}; use indexmap::IndexMap; use multimap::MultiMap; +use rangemap::RangeSet; use serde::Deserialize; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Write; @@ -1001,20 +1002,9 @@ impl ConfigGenerator { let mut all: Vec<_> = by_controller.iter_all().collect(); all.sort(); - for (controller, indices) in all { - let mut s: Vec = - indices.iter().map(|f| format!("{}", f)).collect::<_>(); - - s.sort(); - - write!( - &mut self.output, - r##" - {} => Some(Controller::I2C{}),"##, - s.join("\n | "), - controller, - )?; - } + match_arms(&mut self.output, all, |c| { + format!("Some(Controller::I2C{c})") + })?; write!( &mut self.output, @@ -1037,20 +1027,7 @@ impl ConfigGenerator { let mut all: Vec<_> = by_port.iter_all().collect(); all.sort(); - for (port, indices) in all { - let mut s: Vec = - indices.iter().map(|f| format!("{}", f)).collect::<_>(); - - s.sort(); - - write!( - &mut self.output, - r##" - {} => Some(PortIndex({})),"##, - s.join("\n | "), - port, - )?; - } + match_arms(&mut self.output, all, |p| format!("Some(PortIndex({p}))"))?; write!( &mut self.output, @@ -1755,3 +1732,33 @@ pub fn device_descriptions() -> impl Iterator { }, ) } + +fn match_arms<'a, C>( + mut out: impl Write, + source: impl IntoIterator)>, + fmt: impl Fn(&C) -> String, +) -> Result<()> +where + C: 'a, +{ + for (controller, indices) in source { + let indices = indices + .iter() + .map(|&i| i..i + 1) + .collect::>(); + let s = indices + .iter() + .map(|range| format!("{}..={}", range.start, range.end - 1)) + .collect::>() + .join("\n | "); + + let result = fmt(controller); + + write!( + &mut out, + r##" + {s} => {result},"##, + )?; + } + Ok(()) +} From d195bcef77dba0c3206727ae31c86761e33c087b Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 15:22:03 -0700 Subject: [PATCH 25/26] control-plane-agent: finish ComponentUpdater This change causes SpUpdate to impl ComponentUpdater, instead of sporting functions that look a lot _like_ ComponentUpdater but aren't actually. This fixes a warning in the newest toolchain, which was concerned that operations in the non-public trait were going unused (which they were!). In order to adapt ComponentUpdater to be able to describe all three cases, I have added two associated types to it. UpdatePrepare parameterizes the difference between RoT/HostFlash updates (which want ComponentUpdatePrepare) and SP updates (which want a different SpUpdatePrepare type). SubComponent models the fact that only the SP seems to want a specifier for sub-components inside itself; the others set this to (). --- task/control-plane-agent/src/mgs_gimlet.rs | 8 ++--- task/control-plane-agent/src/mgs_psc.rs | 6 ++-- task/control-plane-agent/src/mgs_sidecar.rs | 6 ++-- .../src/update/host_flash.rs | 4 +++ task/control-plane-agent/src/update/mod.rs | 16 +++++++--- task/control-plane-agent/src/update/rot.rs | 4 +++ task/control-plane-agent/src/update/sp.rs | 32 +++++++++++-------- 7 files changed, 49 insertions(+), 27 deletions(-) diff --git a/task/control-plane-agent/src/mgs_gimlet.rs b/task/control-plane-agent/src/mgs_gimlet.rs index a613d127b7..ae03c797af 100644 --- a/task/control-plane-agent/src/mgs_gimlet.rs +++ b/task/control-plane-agent/src/mgs_gimlet.rs @@ -623,10 +623,10 @@ impl SpHandler for MgsHandler { .ingest_chunk(&chunk.component, &chunk.id, chunk.offset, data), SpComponent::HOST_CPU_BOOT_FLASH => self .host_flash_update - .ingest_chunk(&chunk.id, chunk.offset, data), - SpComponent::ROT | SpComponent::STAGE0 => { - self.rot_update.ingest_chunk(&chunk.id, chunk.offset, data) - } + .ingest_chunk(&(), &chunk.id, chunk.offset, data), + SpComponent::ROT | SpComponent::STAGE0 => self + .rot_update + .ingest_chunk(&(), &chunk.id, chunk.offset, data), _ => Err(SpError::RequestUnsupportedForComponent), } } diff --git a/task/control-plane-agent/src/mgs_psc.rs b/task/control-plane-agent/src/mgs_psc.rs index f74d835ab2..ffb50bcf26 100644 --- a/task/control-plane-agent/src/mgs_psc.rs +++ b/task/control-plane-agent/src/mgs_psc.rs @@ -332,9 +332,9 @@ impl SpHandler for MgsHandler { SpComponent::SP_ITSELF | SpComponent::SP_AUX_FLASH => self .sp_update .ingest_chunk(&chunk.component, &chunk.id, chunk.offset, data), - SpComponent::ROT | SpComponent::STAGE0 => { - self.rot_update.ingest_chunk(&chunk.id, chunk.offset, data) - } + SpComponent::ROT | SpComponent::STAGE0 => self + .rot_update + .ingest_chunk(&(), &chunk.id, chunk.offset, data), _ => Err(SpError::RequestUnsupportedForComponent), } } diff --git a/task/control-plane-agent/src/mgs_sidecar.rs b/task/control-plane-agent/src/mgs_sidecar.rs index ea3b011751..edbdbdfd94 100644 --- a/task/control-plane-agent/src/mgs_sidecar.rs +++ b/task/control-plane-agent/src/mgs_sidecar.rs @@ -385,9 +385,9 @@ impl SpHandler for MgsHandler { SpComponent::SP_ITSELF | SpComponent::SP_AUX_FLASH => self .sp_update .ingest_chunk(&chunk.component, &chunk.id, chunk.offset, data), - SpComponent::ROT | SpComponent::STAGE0 => { - self.rot_update.ingest_chunk(&chunk.id, chunk.offset, data) - } + SpComponent::ROT | SpComponent::STAGE0 => self + .rot_update + .ingest_chunk(&(), &chunk.id, chunk.offset, data), _ => Err(SpError::RequestUnsupportedForComponent), } } diff --git a/task/control-plane-agent/src/update/host_flash.rs b/task/control-plane-agent/src/update/host_flash.rs index 3485d26de0..3f011a0442 100644 --- a/task/control-plane-agent/src/update/host_flash.rs +++ b/task/control-plane-agent/src/update/host_flash.rs @@ -84,6 +84,9 @@ static_assertions::const_assert!( impl ComponentUpdater for HostFlashUpdate { const BLOCK_SIZE: usize = PAGE_SIZE_BYTES; + type UpdatePrepare = ComponentUpdatePrepare; + type SubComponent = (); + fn prepare( &mut self, buffer: &'static UpdateBuffer, @@ -243,6 +246,7 @@ impl ComponentUpdater for HostFlashUpdate { fn ingest_chunk( &mut self, + _sub: &(), id: &UpdateId, offset: u32, mut data: &[u8], diff --git a/task/control-plane-agent/src/update/mod.rs b/task/control-plane-agent/src/update/mod.rs index a24abbbf2d..650db2551e 100644 --- a/task/control-plane-agent/src/update/mod.rs +++ b/task/control-plane-agent/src/update/mod.rs @@ -3,9 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use crate::mgs_handler::UpdateBuffer; -use gateway_messages::{ - ComponentUpdatePrepare, SpError, UpdateId, UpdateStatus, -}; +use gateway_messages::{SpError, UpdateId, UpdateStatus}; #[cfg(feature = "gimlet")] pub(crate) mod host_flash; @@ -30,6 +28,15 @@ pub(crate) trait ComponentUpdater { /// mechanism wants as a single chunk. const BLOCK_SIZE: usize; + /// Record provided to the `prepare` operation. Generally + /// `ComponentUpdatePrepare`, and would default to that if associated type + /// defaults were stable at the time of this writing (2024-04), which they + /// are not. + type UpdatePrepare; + + /// Type used to specify sub-components within a component. + type SubComponent; + /// Attempt to start preparing for an update, using `buffer` as the backing /// store for incoming data. /// @@ -38,7 +45,7 @@ pub(crate) trait ComponentUpdater { fn prepare( &mut self, buffer: &'static UpdateBuffer, - update: ComponentUpdatePrepare, + update: Self::UpdatePrepare, ) -> Result<(), SpError>; /// Returns true if this task needs `step_preparation()` called. @@ -53,6 +60,7 @@ pub(crate) trait ComponentUpdater { /// Attempt to ingest a single update chunk from MGS. fn ingest_chunk( &mut self, + sub: &Self::SubComponent, id: &UpdateId, offset: u32, data: &[u8], diff --git a/task/control-plane-agent/src/update/rot.rs b/task/control-plane-agent/src/update/rot.rs index bc1ad91de3..928301f31d 100644 --- a/task/control-plane-agent/src/update/rot.rs +++ b/task/control-plane-agent/src/update/rot.rs @@ -51,6 +51,9 @@ enum State { impl ComponentUpdater for RotUpdate { const BLOCK_SIZE: usize = BLOCK_SIZE_BYTES; + type UpdatePrepare = ComponentUpdatePrepare; + type SubComponent = (); + fn prepare( &mut self, buffer: &'static UpdateBuffer, @@ -131,6 +134,7 @@ impl ComponentUpdater for RotUpdate { fn ingest_chunk( &mut self, + _sub: &(), id: &UpdateId, offset: u32, mut data: &[u8], diff --git a/task/control-plane-agent/src/update/sp.rs b/task/control-plane-agent/src/update/sp.rs index 570a02e934..f83a7551ba 100644 --- a/task/control-plane-agent/src/update/sp.rs +++ b/task/control-plane-agent/src/update/sp.rs @@ -56,6 +56,7 @@ use crate::mgs_common::UPDATE_SERVER; use crate::mgs_handler::{BorrowedUpdateBuffer, UpdateBuffer}; +use crate::update::ComponentUpdater; use cfg_if::cfg_if; use core::ops::{Deref, DerefMut}; use drv_caboose::CabooseReader; @@ -98,12 +99,6 @@ pub(crate) struct SpUpdate { } impl SpUpdate { - #[cfg(feature = "auxflash")] - pub(crate) const BLOCK_SIZE: usize = - crate::usize_max(BLOCK_SIZE_BYTES, drv_auxflash_api::PAGE_SIZE_BYTES); - #[cfg(not(feature = "auxflash"))] - pub(crate) const BLOCK_SIZE: usize = BLOCK_SIZE_BYTES; - pub(crate) fn new() -> Self { Self { sp_task: Update::from(UPDATE_SERVER.get_task_id()), @@ -111,11 +106,22 @@ impl SpUpdate { current: None, } } +} - pub(crate) fn prepare( +impl ComponentUpdater for SpUpdate { + #[cfg(feature = "auxflash")] + const BLOCK_SIZE: usize = + crate::usize_max(BLOCK_SIZE_BYTES, drv_auxflash_api::PAGE_SIZE_BYTES); + #[cfg(not(feature = "auxflash"))] + const BLOCK_SIZE: usize = BLOCK_SIZE_BYTES; + + type UpdatePrepare = SpUpdatePrepare; + type SubComponent = SpComponent; + + fn prepare( &mut self, buffer: &'static UpdateBuffer, - update: SpUpdatePrepare, + update: Self::UpdatePrepare, ) -> Result<(), SpError> { // Do we have an update already in progress? match self.current.as_ref().map(|c| c.state()) { @@ -178,14 +184,14 @@ impl SpUpdate { Ok(()) } - pub(crate) fn is_preparing(&self) -> bool { + fn is_preparing(&self) -> bool { match self.current.as_ref().map(|c| c.state()) { Some(State::AuxFlash(s)) => s.is_preparing(), _ => false, } } - pub(crate) fn step_preparation(&mut self) { + fn step_preparation(&mut self) { // Do we have an update? let current = match self.current.as_mut() { Some(current) => current, @@ -231,7 +237,7 @@ impl SpUpdate { }); } - pub(crate) fn status(&self) -> UpdateStatus { + fn status(&self) -> UpdateStatus { let current = match self.current.as_ref() { Some(current) => current, None => return UpdateStatus::None, @@ -264,7 +270,7 @@ impl SpUpdate { } } - pub(crate) fn ingest_chunk( + fn ingest_chunk( &mut self, component: &SpComponent, id: &UpdateId, @@ -377,7 +383,7 @@ impl SpUpdate { }) } - pub(crate) fn abort(&mut self, id: &UpdateId) -> Result<(), SpError> { + fn abort(&mut self, id: &UpdateId) -> Result<(), SpError> { // Do we have an update in progress? If not, nothing to do. let current = match self.current.as_mut() { Some(current) => current, From c60adc1006445a8cf8dda098a4095d47a7884dba Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 18:02:11 -0700 Subject: [PATCH 26/26] transceivers-server: remove illusory loop Clippy pointed out that this loop can't, well, loop. At all. I'm kind of surprised earlier versions missed this. --- drv/transceivers-server/src/main.rs | 219 ++++++++++++++-------------- 1 file changed, 106 insertions(+), 113 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 036a5200a5..e40ee1c241 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -596,129 +596,122 @@ impl NotificationHandler for ServerImpl { #[export_name = "main"] fn main() -> ! { + // This is a temporary workaround that makes sure the FPGAs are up + // before we start doing things with them. A more sophisticated + // notification system will be put in place. + let seq = Sequencer::from(SEQ.get_task_id()); + + let transceivers = Transceivers::new(FRONT_IO.get_task_id()); + let leds = Leds::new( + &i2c_config::devices::pca9956b_front_leds_left(I2C.get_task_id()), + &i2c_config::devices::pca9956b_front_leds_right(I2C.get_task_id()), + ); + + let net = task_net_api::Net::from(NET.get_task_id()); + let thermal_api = Thermal::from(THERMAL.get_task_id()); + let sensor_api = Sensor::from(SENSOR.get_task_id()); + let (tx_data_buf, rx_data_buf) = claim_statics(); + let mut server = ServerImpl { + transceivers, + leds, + net, + modules_present: LogicalPortMask(0), + front_io_board_present: FrontIOStatus::NotReady, + led_error: Default::default(), + leds_initialized: false, + led_states: LedStates([LedState::Off; NUM_PORTS as usize]), + blink_on: false, + system_led_state: LedState::Off, + disabled: LogicalPortMask(0), + consecutive_nacks: [0; NUM_PORTS as usize], + thermal_api, + sensor_api, + thermal_models: [None; NUM_PORTS as usize], + }; + + // There are two timers, one for each communication bus: + #[derive(Copy, Clone, Enum)] + #[allow(clippy::upper_case_acronyms)] + enum Timers { + I2C, + SPI, + Blink, + } + let mut multitimer = Multitimer::::new(notifications::TIMER_BIT); + // Immediately fire each timer, then begin to service regularly + let now = sys_get_timer().now; + multitimer.set_timer( + Timers::I2C, + now, + Some(Repeat::AfterDeadline(I2C_INTERVAL)), + ); + multitimer.set_timer( + Timers::SPI, + now, + Some(Repeat::AfterDeadline(SPI_INTERVAL)), + ); + multitimer.set_timer( + Timers::Blink, + now, + Some(Repeat::AfterDeadline(BLINK_INTERVAL)), + ); + + let mut buffer = [0; idl::INCOMING_SIZE]; loop { - // This is a temporary workaround that makes sure the FPGAs are up - // before we start doing things with them. A more sophisticated - // notification system will be put in place. - let seq = Sequencer::from(SEQ.get_task_id()); - - let transceivers = Transceivers::new(FRONT_IO.get_task_id()); - let leds = Leds::new( - &i2c_config::devices::pca9956b_front_leds_left(I2C.get_task_id()), - &i2c_config::devices::pca9956b_front_leds_right(I2C.get_task_id()), - ); - - let net = task_net_api::Net::from(NET.get_task_id()); - let thermal_api = Thermal::from(THERMAL.get_task_id()); - let sensor_api = Sensor::from(SENSOR.get_task_id()); - let (tx_data_buf, rx_data_buf) = claim_statics(); - let mut server = ServerImpl { - transceivers, - leds, - net, - modules_present: LogicalPortMask(0), - front_io_board_present: FrontIOStatus::NotReady, - led_error: Default::default(), - leds_initialized: false, - led_states: LedStates([LedState::Off; NUM_PORTS as usize]), - blink_on: false, - system_led_state: LedState::Off, - disabled: LogicalPortMask(0), - consecutive_nacks: [0; NUM_PORTS as usize], - thermal_api, - sensor_api, - thermal_models: [None; NUM_PORTS as usize], - }; + if server.front_io_board_present == FrontIOStatus::NotReady { + server.front_io_board_present = match seq.front_io_board_ready() { + Ok(true) => { + ringbuf_entry!(Trace::FrontIOBoardReady(true)); + FrontIOStatus::Ready + } + Err(SeqError::NoFrontIOBoard) => { + ringbuf_entry!(Trace::FrontIOSeqErr( + SeqError::NoFrontIOBoard + )); + FrontIOStatus::NotPresent + } + _ => { + ringbuf_entry!(Trace::FrontIOBoardReady(false)); + FrontIOStatus::NotReady + } + }; - // There are two timers, one for each communication bus: - #[derive(Copy, Clone, Enum)] - #[allow(clippy::upper_case_acronyms)] - enum Timers { - I2C, - SPI, - Blink, - } - let mut multitimer = - Multitimer::::new(notifications::TIMER_BIT); - // Immediately fire each timer, then begin to service regularly - let now = sys_get_timer().now; - multitimer.set_timer( - Timers::I2C, - now, - Some(Repeat::AfterDeadline(I2C_INTERVAL)), - ); - multitimer.set_timer( - Timers::SPI, - now, - Some(Repeat::AfterDeadline(SPI_INTERVAL)), - ); - multitimer.set_timer( - Timers::Blink, - now, - Some(Repeat::AfterDeadline(BLINK_INTERVAL)), - ); - - let mut buffer = [0; idl::INCOMING_SIZE]; - loop { - if server.front_io_board_present == FrontIOStatus::NotReady { - server.front_io_board_present = match seq.front_io_board_ready() - { - Ok(true) => { - ringbuf_entry!(Trace::FrontIOBoardReady(true)); - FrontIOStatus::Ready - } - Err(SeqError::NoFrontIOBoard) => { - ringbuf_entry!(Trace::FrontIOSeqErr( - SeqError::NoFrontIOBoard - )); - FrontIOStatus::NotPresent - } - _ => { - ringbuf_entry!(Trace::FrontIOBoardReady(false)); - FrontIOStatus::NotReady + // If a board is present, attempt to initialize its + // LED drivers + if server.front_io_board_present == FrontIOStatus::Ready { + ringbuf_entry!(Trace::LEDInit); + match server.transceivers.enable_led_controllers() { + Ok(_) => server.led_init(), + Err(e) => { + ringbuf_entry!(Trace::LEDEnableError(e)) } }; - - // If a board is present, attempt to initialize its - // LED drivers - if server.front_io_board_present == FrontIOStatus::Ready { - ringbuf_entry!(Trace::LEDInit); - match server.transceivers.enable_led_controllers() { - Ok(_) => server.led_init(), - Err(e) => { - ringbuf_entry!(Trace::LEDEnableError(e)) - } - }; - } } + } - multitimer.poll_now(); - for t in multitimer.iter_fired() { - match t { - Timers::I2C => { - // Handle the Front IO status checking as part of this - // loop because the frequency is what we had before and - // the server itself has no knowledge of the sequencer. - server.handle_i2c_loop(); - } - Timers::SPI => { - if server.front_io_board_present == FrontIOStatus::Ready - { - server.handle_spi_loop(); - } - } - Timers::Blink => { - server.blink_on = !server.blink_on; + multitimer.poll_now(); + for t in multitimer.iter_fired() { + match t { + Timers::I2C => { + // Handle the Front IO status checking as part of this + // loop because the frequency is what we had before and + // the server itself has no knowledge of the sequencer. + server.handle_i2c_loop(); + } + Timers::SPI => { + if server.front_io_board_present == FrontIOStatus::Ready { + server.handle_spi_loop(); } } + Timers::Blink => { + server.blink_on = !server.blink_on; + } } - - server.check_net( - tx_data_buf.as_mut_slice(), - rx_data_buf.as_mut_slice(), - ); - idol_runtime::dispatch(&mut buffer, &mut server); } + + server + .check_net(tx_data_buf.as_mut_slice(), rx_data_buf.as_mut_slice()); + idol_runtime::dispatch(&mut buffer, &mut server); } } ////////////////////////////////////////////////////////////////////////////////