From ce20b9b7b561035e853f1c22f7d2f0d62715175f Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 10:48:53 -0700 Subject: [PATCH 1/7] fixedmap: make runtime check smaller FixedMap contains a runtime capacity check that has, as far as I can tell, never fired. This means it doesn't need an explanatory error message -- file/line should be sufficient. Removing the message saves a good chunk of text. --- lib/fixedmap/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fixedmap/src/lib.rs b/lib/fixedmap/src/lib.rs index c1e4402bc..497302958 100644 --- a/lib/fixedmap/src/lib.rs +++ b/lib/fixedmap/src/lib.rs @@ -75,7 +75,7 @@ impl FixedMap { } } - panic!("FixedMap overflow"); + panic!(); } /// From bd20e114b66465b2820f26b5ad8f1a042b7457e9 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 10:50:12 -0700 Subject: [PATCH 2/7] hl::recv: handle message truncation The hl::recv facility is _almost_ dead, but the single user I can find (the I2C server) is quite popular, so we have to maintain it. Sigh. It's entirely possible for rm.message_len to be larger than the buffer: if the client sends an overly long message, the kernel will report that. So, this code has been a client-controlled server panic for ages. This switches it to check the message length when slicing, and transfer the panic to the client, where it belongs. This happens to remove a hundred bytes or so of text, by removing the setup and call to the slice index bounds fail routine. --- sys/userlib/src/hl.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/sys/userlib/src/hl.rs b/sys/userlib/src/hl.rs index bee304682..e6a7aeaa1 100644 --- a/sys/userlib/src/hl.rs +++ b/sys/userlib/src/hl.rs @@ -13,8 +13,8 @@ use zerocopy::{AsBytes, FromBytes, LayoutVerified}; use crate::{ sys_borrow_info, sys_borrow_read, sys_borrow_write, sys_get_timer, - sys_recv, sys_recv_closed, sys_recv_open, sys_reply, sys_set_timer, - BorrowInfo, ClosedRecvError, FromPrimitive, + sys_recv, sys_recv_closed, sys_recv_open, sys_reply, sys_reply_fault, + sys_set_timer, BorrowInfo, ClosedRecvError, FromPrimitive, }; const INTERNAL_TIMER_NOTIFICATION: u32 = 1 << 31; @@ -94,14 +94,18 @@ pub fn recv<'a, O, E, S>( if rm.sender == TaskId::KERNEL { notify(state, rm.operation); } else if let Some(op) = O::from_u32(rm.operation) { - let m = Message { - buffer: &buffer[..rm.message_len], - sender: rm.sender, - response_capacity: rm.response_capacity, - lease_count: rm.lease_count, - }; - if let Err(e) = msg(state, op, m) { - sys_reply(sender, e.into(), &[]); + if let Some(buffer) = buffer.get(..rm.message_len) { + let m = Message { + buffer, + sender: rm.sender, + response_capacity: rm.response_capacity, + lease_count: rm.lease_count, + }; + if let Err(e) = msg(state, op, m) { + sys_reply(sender, e.into(), &[]); + } + } else { + sys_reply_fault(sender, abi::ReplyFaultReason::BadMessageSize); } } else { sys_reply(sender, 1, &[]); From 4156ce3a2615728c3ead1f680f0a0753ebd12477 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 10:52:06 -0700 Subject: [PATCH 3/7] hl::sleep_for: use explicit checked arithmetic Previously, sleep_for was using normal arithmetic to compute a deadline from the given number of ticks. We don't expect ticks to overflow in practice (it's 64-bit) but normal arithmetic checks for overflow. This means sleeping with a very very large interval would produce a panic. A panic from arithmetic overflow is somewhat more expensive than a no-message explicit panic, or an unwrap_lite. This commit changes the arithmetic to do checks explicitly and unwrap_lite on failure, saving a chunk of text. --- sys/userlib/src/hl.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sys/userlib/src/hl.rs b/sys/userlib/src/hl.rs index e6a7aeaa1..00e7be98f 100644 --- a/sys/userlib/src/hl.rs +++ b/sys/userlib/src/hl.rs @@ -9,6 +9,7 @@ use abi::TaskId; use core::marker::PhantomData; +use unwrap_lite::UnwrapLite; use zerocopy::{AsBytes, FromBytes, LayoutVerified}; use crate::{ @@ -464,5 +465,10 @@ pub fn sleep_for(ticks: u64) { // `sleep_for(x)` will sleep for at least `x` full ticks. Note that the task // calling `sleep_for` may get woken arbitrarily later if preempted by // higher priority tasks, so at-least is generally the best we can do. - sleep_until(sys_get_timer().now + ticks + 1) + let deadline = sys_get_timer() + .now + .checked_add(ticks) + .and_then(|t| t.checked_add(1)) + .unwrap_lite(); + sleep_until(deadline) } From 79757fc1788ec93bc18ac7ed1d3816ff58a0fd0e Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 10:57:22 -0700 Subject: [PATCH 4/7] userlib::task_slot: remove (one) runtime check task_slot checks its own build system behavior at runtime at a nontrivial cost, particularly in tasks with few panics. I'd argue this is unnecessary because 1. This has, as far as I can tell, never failed. 2. Should it fail, the kernel will fault us with a "task index out of range" fault, so it's not like it'll be silently ignored. Fixes #1538. --- sys/userlib/src/task_slot.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/sys/userlib/src/task_slot.rs b/sys/userlib/src/task_slot.rs index 9ecbaede8..4c30d6f48 100644 --- a/sys/userlib/src/task_slot.rs +++ b/sys/userlib/src/task_slot.rs @@ -20,16 +20,13 @@ pub struct TaskSlot(VolatileConst); impl TaskSlot { /// A TaskSlot that has not been resolved by a later processing step. /// - /// Calling get_task_id() on an unbound TaskSlot will panic. + /// Calling get_task_id() on an unbound TaskSlot will cause a fault from the + /// kernel. pub const UNBOUND: Self = Self(VolatileConst::new(TaskId::UNBOUND.0)); pub fn get_task_id(&self) -> TaskId { let task_index = self.get_task_index(); - if task_index == TaskId::UNBOUND.0 { - panic!("Attempted to get task id of unbound TaskSlot"); - } - let prototype = TaskId::for_index_and_gen(task_index.into(), Generation::default()); crate::sys_refresh_task_id(prototype) From af0748d8acca90c7cf42cf073dbe509f65af626d Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 10:59:10 -0700 Subject: [PATCH 5/7] stm32xx-i2c: remove arithmetic overflow in retry I'm pretty sure the "laps" counter variable couldn't overflow, but this was not obvious to the compiler, which was generating checks and panics. Restructuring the loop to use explicit iteration bounds fixes this. This knocks about 100 bytes off the I2C server. --- drv/stm32xx-i2c/src/lib.rs | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/drv/stm32xx-i2c/src/lib.rs b/drv/stm32xx-i2c/src/lib.rs index 2a821d7ea..66951b93e 100644 --- a/drv/stm32xx-i2c/src/lib.rs +++ b/drv/stm32xx-i2c/src/lib.rs @@ -441,9 +441,8 @@ impl I2cController<'_> { // failure mode for a condition expected to be unusual.) // const BUSY_SLEEP_THRESHOLD: u32 = 300; - let mut laps = 0; - loop { + for lap in 0..=BUSY_SLEEP_THRESHOLD + 1 { let isr = i2c.isr.read(); ringbuf_entry!(Trace::WaitISR(isr.bits())); @@ -454,20 +453,17 @@ impl I2cController<'_> { // up to the controller, the timeout flag is set, we clear it and // ignore it -- we know that it's spurious. // - if laps == 0 && isr.timeout().is_timeout() { + if lap == 0 && isr.timeout().is_timeout() { i2c.icr.write(|w| w.timoutcf().set_bit()); } if !isr.busy().is_busy() { - break; + return Ok(()); } self.check_errors(&isr)?; - laps += 1; - - #[allow(clippy::comparison_chain)] // clippy misfire - if laps == BUSY_SLEEP_THRESHOLD { + if lap == BUSY_SLEEP_THRESHOLD { // // If we have taken BUSY_SLEEP_THRESHOLD laps, we are going to // sleep for two ticks -- which should be far greater than the @@ -475,21 +471,19 @@ impl I2cController<'_> { // ringbuf_entry!(Trace::BusySleep); hl::sleep_for(2); - } else if laps > BUSY_SLEEP_THRESHOLD { - // - // We have already taken BUSY_SLEEP_THRESHOLD laps AND a two - // tick sleep -- and the busy bit is still set. At this point, - // we need to return an error indicating that we need to reset - // the controller. We return a disjoint error code here to - // be able to know that we hit this condition rather than our - // more expected conditions on bus lockup (namely, a timeout - // or arbitration lost). - // - return Err(drv_i2c_api::ResponseCode::ControllerBusy); } + // On lap == BUSY_SLEEP_THRESHOLD + 1 we'll fall out. } - Ok(()) + // + // We have already taken BUSY_SLEEP_THRESHOLD laps AND a two tick sleep + // -- and the busy bit is still set. At this point, we need to return + // an error indicating that we need to reset the controller. We return + // a disjoint error code here to be able to know that we hit this + // condition rather than our more expected conditions on bus lockup + // (namely, a timeout or arbitration lost). + // + Err(drv_i2c_api::ResponseCode::ControllerBusy) } /// Perform a write to and then a read from the specified device. Either From 45d09f0b984943cd893492b258732b53df105ee7 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 11:01:49 -0700 Subject: [PATCH 6/7] stm32xx-i2c-server: remove overflow in pin wiggling I'm pretty sure the wiggles variable (a u8) could not overflow here, but the compiler is less convinced and was generating checks. Switched it to explicitly wrapping arithmetic for some text savings. --- drv/stm32xx-i2c-server/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index 437b522c7..3dba253d0 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -585,7 +585,7 @@ fn configure_port( /// [0] Analog Devices. AN-686: Implementing an I2C Reset. 2003. /// fn wiggle_scl(sys: &Sys, scl: PinSet, sda: PinSet) { - let mut wiggles = 0; + let mut wiggles = 0_u8; sys.gpio_set(scl); sys.gpio_configure_output( @@ -618,7 +618,7 @@ fn wiggle_scl(sys: &Sys, scl: PinSet, sda: PinSet) { // sys.gpio_reset(scl); sys.gpio_set(scl); - wiggles += 1; + wiggles = wiggles.wrapping_add(1); } else { // // SDA is high. We're going to flip it back to an output, pull the From eabedcd675063b65717c626dc169048b4bacff00 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Sun, 17 Sep 2023 11:03:57 -0700 Subject: [PATCH 7/7] STM32G030/0xCon Demo Board I2C support. This is the shotgun change required to teach all the various bits that g030 exists, plus adding it to the oxcon2023g0 demo app. The main functional change here: panic messages can now be disabled in the i2c server for specific targets. It'd be nice to have explicit control over this, instead of having it depend on the target, but the way things are currently structured that's not practical. With panic messages off we can fit the server in less than 4 kiB, making it practical on STM32G030. --- app/oxcon2023g0/app.toml | 27 +++++++++++++++++++++++++-- build/i2c/Cargo.toml | 1 + build/i2c/src/lib.rs | 4 +++- drv/stm32xx-i2c-server/Cargo.toml | 11 ++++++----- drv/stm32xx-i2c/Cargo.toml | 1 + drv/stm32xx-i2c/src/lib.rs | 21 +++++++++++++++++---- 6 files changed, 53 insertions(+), 12 deletions(-) diff --git a/app/oxcon2023g0/app.toml b/app/oxcon2023g0/app.toml index 53e338e42..78bfbb0ab 100644 --- a/app/oxcon2023g0/app.toml +++ b/app/oxcon2023g0/app.toml @@ -6,7 +6,7 @@ board = "oxcon2023g0" [kernel] name = "oxcon2023g0" -requires = {flash = 10752, ram = 1200} +requires = {flash = 11008, ram = 1296} stacksize = 640 [tasks.jefe] @@ -24,7 +24,7 @@ priority = 1 max-sizes = {flash = 2048, ram = 256} uses = ["rcc", "gpio", "system_flash"] start = true -features = ["g031"] +features = ["g030"] stacksize = 256 task-slots = ["jefe"] @@ -48,9 +48,32 @@ task-slots = ["sys"] stacksize = 912 features = ["stm32g0", "gpio", "micro"] +[tasks.i2c_driver] +name = "drv-stm32xx-i2c-server" +features = ["g030"] +priority = 2 +max-sizes = {flash = 16384, ram = 1024} +uses = ["i2c1"] +start = true +task-slots = ["sys"] +stacksize = 896 +notifications = ["i2c1-irq"] + +[tasks.i2c_driver.interrupts] +"i2c1.irq" = "i2c1-irq" + [tasks.idle] name = "task-idle" priority = 5 max-sizes = {flash = 128, ram = 64} stacksize = 64 start = true + +[config] +[[config.i2c.controllers]] +controller = 1 + +[config.i2c.controllers.ports.A] +scl.pin = 11 +sda.pin = 10 +af = 6 diff --git a/build/i2c/Cargo.toml b/build/i2c/Cargo.toml index d6c94cd06..941b80a74 100644 --- a/build/i2c/Cargo.toml +++ b/build/i2c/Cargo.toml @@ -18,3 +18,4 @@ h743 = [] h753 = [] h7b3 = [] g031 = [] +g030 = [] diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index e5225bb82..ed601b244 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -650,7 +650,9 @@ impl ConfigGenerator { use stm32h7::stm32h7b3 as device; #[cfg(feature = "g031")] - use stm32g0::stm32g031 as device;"## + use stm32g0::stm32g031 as device; + #[cfg(feature = "g030")] + use stm32g0::stm32g030 as device;"## )?; } diff --git a/drv/stm32xx-i2c-server/Cargo.toml b/drv/stm32xx-i2c-server/Cargo.toml index 91b703899..48dbb2892 100644 --- a/drv/stm32xx-i2c-server/Cargo.toml +++ b/drv/stm32xx-i2c-server/Cargo.toml @@ -15,7 +15,7 @@ drv-stm32xx-i2c = { path = "../stm32xx-i2c" } drv-stm32xx-sys-api = { path = "../stm32xx-sys-api" } fixedmap = { path = "../../lib/fixedmap" } ringbuf = { path = "../../lib/ringbuf" } -userlib = { path = "../../sys/userlib", features = ["panic-messages"] } +userlib = { path = "../../sys/userlib" } [build-dependencies] anyhow = { workspace = true } @@ -25,10 +25,11 @@ build-util = { path = "../../build/util" } build-i2c = { path = "../../build/i2c" } [features] -h743 = ["stm32h7/stm32h743", "drv-stm32xx-i2c/h743", "drv-stm32xx-sys-api/h743", "build-i2c/h743"] -h753 = ["stm32h7/stm32h753", "drv-stm32xx-i2c/h753", "drv-stm32xx-sys-api/h753", "build-i2c/h753"] -g031 = ["stm32g0/stm32g031", "drv-stm32xx-i2c/g031", "drv-stm32xx-sys-api/g031", -"build-i2c/g031", "ringbuf/disabled"] +h743 = ["stm32h7/stm32h743", "drv-stm32xx-i2c/h743", "drv-stm32xx-sys-api/h743", "build-i2c/h743", "panic-messages"] +h753 = ["stm32h7/stm32h753", "drv-stm32xx-i2c/h753", "drv-stm32xx-sys-api/h753", "build-i2c/h753", "panic-messages"] +g031 = ["stm32g0/stm32g031", "drv-stm32xx-i2c/g031", "drv-stm32xx-sys-api/g031", "build-i2c/g031", "ringbuf/disabled", "panic-messages"] +g030 = ["stm32g0/stm32g030", "drv-stm32xx-i2c/g030", "drv-stm32xx-sys-api/g030", "build-i2c/g030", "ringbuf/disabled"] +panic-messages = ["userlib/panic-messages"] # This section is here to discourage RLS/rust-analyzer from doing test builds, # since test builds don't work for cross compilation. diff --git a/drv/stm32xx-i2c/Cargo.toml b/drv/stm32xx-i2c/Cargo.toml index 022bb4a1f..a41788da9 100644 --- a/drv/stm32xx-i2c/Cargo.toml +++ b/drv/stm32xx-i2c/Cargo.toml @@ -20,6 +20,7 @@ userlib = { path = "../../sys/userlib" } h743 = ["stm32h7/stm32h743", "drv-stm32xx-sys-api/h743"] h753 = ["stm32h7/stm32h753", "drv-stm32xx-sys-api/h753"] g031 = ["stm32g0/stm32g031", "drv-stm32xx-sys-api/g031"] +g030 = ["stm32g0/stm32g030", "drv-stm32xx-sys-api/g030"] amd_erratum_1394 = [] # This section is here to discourage RLS/rust-analyzer from doing test builds, diff --git a/drv/stm32xx-i2c/src/lib.rs b/drv/stm32xx-i2c/src/lib.rs index 66951b93e..6fae96bf0 100644 --- a/drv/stm32xx-i2c/src/lib.rs +++ b/drv/stm32xx-i2c/src/lib.rs @@ -25,10 +25,23 @@ pub type Isr = device::i2c3::isr::R; #[cfg(feature = "g031")] use stm32g0::stm32g031 as device; -#[cfg(any(feature = "h743", feature = "h753", feature = "g031"))] +#[cfg(feature = "g030")] +use stm32g0::stm32g030 as device; + +#[cfg(any( + feature = "h743", + feature = "h753", + feature = "g031", + feature = "g030" +))] pub type RegisterBlock = device::i2c1::RegisterBlock; -#[cfg(any(feature = "h743", feature = "h753", feature = "g031"))] +#[cfg(any( + feature = "h743", + feature = "h753", + feature = "g031", + feature = "g030" +))] pub type Isr = device::i2c1::isr::R; pub mod ltc4306; @@ -291,7 +304,7 @@ impl I2cController<'_> { .scldel().bits(scldel) .sdadel().bits(0) }); - } else if #[cfg(feature = "g031")] { + } else if #[cfg(any(feature = "g031", feature = "g030"))] { // On the G0, our APB peripheral clock is 16MHz, yielding: // // - A PRESC of 0, yielding a t_presc of 62 ns @@ -339,7 +352,7 @@ impl I2cController<'_> { .timeouta().bits(3417) // Timeout value .tidle().clear_bit() // Want SCL, not IDLE }); - } else if #[cfg(feature = "g031")] { + } else if #[cfg(any(feature = "g030", feature = "g031"))] { i2c.timeoutr.write(|w| { w .timouten().set_bit() // Enable SCL timeout .timeouta().bits(196) // Timeout value