From 68f2c9178c41b0935c0cd35bd36b554b434e618d Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 15:40:56 -0700 Subject: [PATCH 01/20] lib/dice: remove dead traits These traits are newly detected as dead by the new toolchain, so after discussion with @flihp I'm removing them -- we can always get them out of git if we need them. --- lib/dice/src/cert.rs | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/lib/dice/src/cert.rs b/lib/dice/src/cert.rs index b69b69651..e50586915 100644 --- a/lib/dice/src/cert.rs +++ b/lib/dice/src/cert.rs @@ -65,24 +65,6 @@ pub trait Cert: AsBytes { } } -pub trait IssuerCnCert: Cert { - const ISSUER_CN_RANGE: Range; - - fn get_issuer_cn(&self) -> PlatformId { - PlatformId::try_from(self.get_range::<&[u8]>(Self::ISSUER_CN_RANGE)) - .unwrap_lite() - } -} - -pub trait SubjectCnCert: Cert { - const SUBJECT_CN_RANGE: Range; - - fn get_subject_cn(&self) -> PlatformId { - PlatformId::try_from(self.get_range::<&[u8]>(Self::SUBJECT_CN_RANGE)) - .unwrap_lite() - } -} - /// Trait for Certs with the TCG DICE TcbInfo structure w/ the FWID member. pub trait FwidCert: Cert { const FWID_RANGE: Range; From d663549d2751de97374ed5463287393ab0a8d4a6 Mon Sep 17 00:00:00 2001 From: Bryan Cantrill Date: Tue, 9 Apr 2024 23:32:32 -0700 Subject: [PATCH 02/20] Gimlet hangs on cold power-on after toolchain update (#1742) Fixes #1741 by: 1. Bumping the stack size for the net task 2. Changing i2c_driver to do its existing SCL wiggle dance on a controller reset 3. Fixing the SPD retry login in gimlet_seq Unrelatedly, this also grows the stack of the power task to provide sufficient stack space to read the IBC blackbox. --- app/demo-stm32h7-nucleo/app-h743.toml | 2 +- app/demo-stm32h7-nucleo/app-h753.toml | 2 +- app/gimlet/base.toml | 4 +- app/gimletlet/app.toml | 2 +- app/psc/base.toml | 2 +- app/sidecar/base.toml | 4 +- drv/gimlet-seq-server/src/main.rs | 66 ++++++++++++++++++++------ drv/stm32xx-i2c-server/src/main.rs | 67 +++++++++++++++++++++++---- task/jefe/src/dump.rs | 10 ++++ 9 files changed, 129 insertions(+), 30 deletions(-) diff --git a/app/demo-stm32h7-nucleo/app-h743.toml b/app/demo-stm32h7-nucleo/app-h743.toml index c8b9c9c70..ae9727589 100644 --- a/app/demo-stm32h7-nucleo/app-h743.toml +++ b/app/demo-stm32h7-nucleo/app-h743.toml @@ -80,7 +80,7 @@ task-slots = ["sys"] [tasks.net] name = "task-net" -stacksize = 3000 +stacksize = 4000 priority = 2 max-sizes = {flash = 65536, ram = 8192, sram1 = 32768} features = ["h743"] diff --git a/app/demo-stm32h7-nucleo/app-h753.toml b/app/demo-stm32h7-nucleo/app-h753.toml index efa28c158..0909520e4 100644 --- a/app/demo-stm32h7-nucleo/app-h753.toml +++ b/app/demo-stm32h7-nucleo/app-h753.toml @@ -89,7 +89,7 @@ task-slots = ["sys"] [tasks.net] name = "task-net" -stacksize = 3000 +stacksize = 4000 priority = 2 max-sizes = {flash = 131072, ram = 16384, sram1 = 32768} features = ["h753"] diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index e9503eb01..05a63a658 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -37,7 +37,7 @@ request_reset = ["hiffy", "control_plane_agent"] [tasks.net] name = "task-net" -stacksize = 6040 +stacksize = 8000 priority = 5 features = ["mgmt", "h753", "gimlet", "vlan", "vpd-mac"] max-sizes = {flash = 131072, ram = 65536, sram1 = 16384} @@ -139,7 +139,7 @@ name = "task-power" features = ["gimlet"] priority = 6 max-sizes = {flash = 65536, ram = 16384 } -stacksize = 2800 +stacksize = 3800 start = true task-slots = ["i2c_driver", "sensor", "gimlet_seq"] notifications = ["timer", "external_badness"] diff --git a/app/gimletlet/app.toml b/app/gimletlet/app.toml index a8bce0abe..5f89e7522 100644 --- a/app/gimletlet/app.toml +++ b/app/gimletlet/app.toml @@ -145,7 +145,7 @@ notifications = ["hash-irq"] [tasks.net] name = "task-net" -stacksize = 6040 +stacksize = 8000 priority = 3 features = ["h753", "vlan", "gimletlet-nic", "use-spi-core", "spi4"] max-sizes = {flash = 131072, ram = 65536, sram1 = 16384} diff --git a/app/psc/base.toml b/app/psc/base.toml index 5443d3ff2..348b875ca 100644 --- a/app/psc/base.toml +++ b/app/psc/base.toml @@ -117,7 +117,7 @@ task-slots = ["i2c_driver"] [tasks.net] name = "task-net" -stacksize = 6040 +stacksize = 8000 priority = 4 features = ["mgmt", "h753", "psc", "vlan", "vpd-mac", "use-spi-core", "spi2"] max-sizes = {flash = 131072, ram = 65536, sram1 = 16384} diff --git a/app/sidecar/base.toml b/app/sidecar/base.toml index e18eb246d..f5e25fac5 100644 --- a/app/sidecar/base.toml +++ b/app/sidecar/base.toml @@ -78,7 +78,7 @@ task-slots = ["sys"] [tasks.net] name = "task-net" -stacksize = 6040 +stacksize = 8000 priority = 5 features = ["mgmt", "h753", "sidecar", "vlan", "vpd-mac", "use-spi-core", "spi3"] max-sizes = {flash = 131072, ram = 65536, sram1 = 16384} @@ -278,7 +278,7 @@ name = "task-power" features = ["sidecar"] priority = 6 max-sizes = {flash = 32768, ram = 8192 } -stacksize = 2800 +stacksize = 3800 start = true task-slots = ["i2c_driver", "sensor", "sequencer"] notifications = ["timer"] diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs index c0ba2d543..38116d6f8 100644 --- a/drv/gimlet-seq-server/src/main.rs +++ b/drv/gimlet-seq-server/src/main.rs @@ -46,6 +46,16 @@ include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); )] mod payload; +#[derive(Copy, Clone, PartialEq, Count)] +enum I2cTxn { + SpdLoad(u8, u8), + SpdLoadTop(u8, u8), + VCoreOn, + VCoreOff, + SocOn, + SocOff, +} + #[derive(Copy, Clone, PartialEq, Count)] enum Trace { Ice40Rails(bool, bool), @@ -111,11 +121,17 @@ enum Trace { SpdBankAbsent(u8), SpdAbsent(u8, u8, u8), SpdDimmsFound(usize), - I2cFault { - retries_remaining: u8, + I2cError { + txn: I2cTxn, #[count(children)] code: i2c::ResponseCode, }, + I2cFault(I2cTxn), + I2cRetry { + #[count(children)] + txn: I2cTxn, + retries_remaining: u8, + }, StartFailed(#[count(children)] SeqError), #[count(skip)] None, @@ -592,6 +608,7 @@ impl NotificationHandler for ServerImpl { } fn retry_i2c_txn( + which: I2cTxn, mut txn: impl FnMut() -> Result, ) -> Result where @@ -604,15 +621,18 @@ where Ok(x) => return Ok(x), Err(e) => { let code = e.into(); - ringbuf_entry!(Trace::I2cFault { - retries_remaining, - code, - }); + ringbuf_entry!(Trace::I2cError { txn: which, code }); if retries_remaining == 0 { + ringbuf_entry!(Trace::I2cFault(which)); return Err(code); } + ringbuf_entry!(Trace::I2cRetry { + txn: which, + retries_remaining + }); + retries_remaining -= 1; } } @@ -1119,7 +1139,20 @@ fn read_spd_data_and_load_packrat( // We'll store that byte and then read 255 more. tmp[0] = first; - retry_i2c_txn(|| spd.read_into(&mut tmp[1..]))?; + let mut retried = false; + + retry_i2c_txn(I2cTxn::SpdLoad(nbank, i), || { + if retried { + // + // If our read needs to be retried, we need to also reset + // ourselves back to the 0th byte. + // + _ = spd.read_reg::(0)?; + } + + retried = true; + spd.read_into(&mut tmp[1..]) + })?; packrat.set_spd_eeprom(ndx, false, 0, &tmp); } @@ -1146,9 +1179,16 @@ fn read_spd_data_and_load_packrat( let spd = I2cDevice::new(i2c_task, controller, port, mux, mem); let chunk = 128; - retry_i2c_txn(|| spd.read_reg_into::(0, &mut tmp[..chunk]))?; - retry_i2c_txn(|| spd.read_into(&mut tmp[chunk..]))?; + retry_i2c_txn(I2cTxn::SpdLoadTop(nbank, i), || { + // + // Both of these reads need to be in a single transaction from + // the perspective of the retry logic: if either fails, we + // must redo both. + // + spd.read_reg_into::(0, &mut tmp[..chunk])?; + spd.read_into(&mut tmp[chunk..]) + })?; packrat.set_spd_eeprom(ndx, true, 0, &tmp); } @@ -1302,8 +1342,8 @@ cfg_if::cfg_if! { let (device, rail) = i2c_config::pmbus::vddcr_soc(i2c); let mut vddcr_soc = Raa229618::new(&device, rail); - retry_i2c_txn(|| vdd_vcore.turn_off())?; - retry_i2c_txn(|| vddcr_soc.turn_off())?; + retry_i2c_txn(I2cTxn::VCoreOff, || vdd_vcore.turn_off())?; + retry_i2c_txn(I2cTxn::SocOff, || vddcr_soc.turn_off())?; Ok(()) } @@ -1317,8 +1357,8 @@ cfg_if::cfg_if! { let (device, rail) = i2c_config::pmbus::vddcr_soc(i2c); let mut vddcr_soc = Raa229618::new(&device, rail); - retry_i2c_txn(|| vdd_vcore.turn_on())?; - retry_i2c_txn(|| vddcr_soc.turn_on())?; + retry_i2c_txn(I2cTxn::VCoreOn, || vdd_vcore.turn_on())?; + retry_i2c_txn(I2cTxn::SocOn, || vddcr_soc.turn_on())?; Ok(()) } diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index a38be81a7..5073c7750 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -296,6 +296,48 @@ fn reset_if_needed( } } +/// +/// A variant of [`reset_if_needed`] that will also wiggle the SCL lines +/// via [`wiggle_scl`]. +/// +fn reset_and_wiggle_if_needed( + code: ResponseCode, + controller: &I2cController<'_>, + port: PortIndex, + muxes: &[I2cMux<'_>], + muxmap: &mut MuxMap, + pins: &[I2cPins], +) { + if reset_needed(code) { + let sys = SYS.get_task_id(); + let sys = Sys::from(sys); + + for pin in pins + .iter() + .filter(|p| p.controller == controller.controller) + .filter(|p| p.port == port) + { + wiggle_scl(&sys, pin.scl, pin.sda); + + // + // [`wiggle_scl`] puts our pins in output (and input) mode; set + // them back to be configured for I2C before we reset. + // + for gpio_pin in &[pin.scl, pin.sda] { + sys.gpio_configure_alternate( + *gpio_pin, + OutputType::OpenDrain, + Speed::Low, + Pull::None, + pin.function, + ); + } + } + + reset(controller, port, muxes, muxmap); + } +} + include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); type PortMap = FixedMap; @@ -487,12 +529,13 @@ fn main() -> ! { } } - reset_if_needed( + reset_and_wiggle_if_needed( code, controller, port, &muxes, &mut muxmap, + &pins, ); return Err(code); } @@ -588,17 +631,23 @@ fn configure_port( } /// -/// When the system is reset without power loss, I2C can be in an arbitrary -/// state with respect to the bus -- and we can therefore come to life with a -/// transaction already in flight. It is very important that we abort any -/// such transaction: failure to do so will result in our first I2C -/// transaction being corrupted. (And especially because our first I2C -/// transactions may well be to disable segments on a mux, this can result in -/// nearly arbitrary mayhem down the road!) To do this, we engage in the +/// When the system is either reset without power loss (e.g., due to an SP +/// upgrade) or I2C is preempted longer than the 25ms I2C timeout (e.g., due +/// to a large process panicking and being dumped by jefe), I2C can be in an +/// arbitrary state with respect to the bus -- and we can therefore come to +/// life with a transaction already in flight. It is very important that we +/// abort any such transaction: failure to do so will result in our first I2C +/// transaction being corrupted. (And because our first I2C transaction on SP +/// boot may well be to disable segments on a mux, this can result in nearly +/// arbitrary mayhem down the road!) To do this, we engage in the /// time-honored[0] tradition of "clocking through the problem": wiggling SCL /// until we see SDA high, and then pulling SDA low and releasing SCL to /// indicate a STOP condition. (Note that we need to do this up to 9 times to -/// assure that we have clocked through the entire transaction.) +/// assure that we have clocked through the entire transaction.) Our assumption +/// is that if SCL is being stretched by an errant target, it has been already +/// stretched beyond our timeout (25ms); if this is the case, us trying to +/// wiggle SCL here won't actually wiggle SCL -- but unless such a device is +/// isolated to a segment on a mux that we can reset, nothing will in fact help. /// /// [0] Analog Devices. AN-686: Implementing an I2C Reset. 2003. /// diff --git a/task/jefe/src/dump.rs b/task/jefe/src/dump.rs index e39d40183..7ea79c03d 100644 --- a/task/jefe/src/dump.rs +++ b/task/jefe/src/dump.rs @@ -55,6 +55,10 @@ enum Trace { }, DumpRead(usize), DumpDone(Result<(), humpty::DumpError<()>>), + DumpTime { + start: u64, + end: u64, + }, } ringbuf!(Trace, 8, Trace::None); @@ -178,6 +182,7 @@ fn dump_task_setup( /// Once a task dump is set up, this function executes it fn dump_task_run(base: u32, task: usize) -> Result<(), DumpAgentError> { ringbuf_entry!(Trace::DumpStart { base }); + let start = sys_get_timer().now; // // The humpty dance is your chance... to do the dump! @@ -225,6 +230,11 @@ fn dump_task_run(base: u32, task: usize) -> Result<(), DumpAgentError> { ); ringbuf_entry!(Trace::DumpDone(r)); + ringbuf_entry!(Trace::DumpTime { + start, + end: sys_get_timer().now + }); + r.map_err(|_| DumpAgentError::DumpFailed)?; Ok(()) } From a35ea553157462635ef35c6f86f24675063eac97 Mon Sep 17 00:00:00 2001 From: Bryan Cantrill Date: Wed, 10 Apr 2024 12:07:40 -0700 Subject: [PATCH 03/20] add RAA229618 VIN monitoring (#1730) In our ongoing attempts to characterize sags in the V12_SYS_A2 rail, we enlist one of the RAA229618 VRs to record observed READ_VIN values whenever the rail drops below 11.75V. --- app/gimlet/base.toml | 10 +- drv/gimlet-seq-server/src/main.rs | 27 ++++- drv/gimlet-seq-server/src/vcore.rs | 156 +++++++++++++++++++++++++++++ drv/i2c-devices/src/lib.rs | 12 +++ drv/i2c-devices/src/raa229618.rs | 15 +++ 5 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 drv/gimlet-seq-server/src/vcore.rs diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 05a63a658..bcba76888 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -66,6 +66,12 @@ notifications = ["exti-wildcard-irq"] "exti.exti9_5" = "exti-wildcard-irq" "exti.exti15_10" = "exti-wildcard-irq" +# PWR_CONT1_VCORE_TO_SP_ALERT_L +[tasks.sys.config.gpio-irqs.vcore_to_sp_alert_l] +port = "I" +pin = 14 +owner = {name = "gimlet_seq", notification = "vcore"} + [tasks.sys.config.gpio-irqs.rot_irq] port = "E" pin = 3 @@ -157,11 +163,11 @@ task-slots = ["sys", "hf", "i2c_driver", "hash_driver", "update_server", "sprot" name = "drv-gimlet-seq-server" features = ["h753"] priority = 4 -max-sizes = {flash = 131072, ram = 8192 } +max-sizes = {flash = 131072, ram = 16384 } stacksize = 1600 start = true task-slots = ["sys", "i2c_driver", {spi_driver = "spi2_driver"}, "hf", "jefe", "packrat"] -notifications = ["timer"] +notifications = ["timer", "vcore"] copy-to-archive = ["register_defs"] [tasks.gimlet_seq.config] diff --git a/drv/gimlet-seq-server/src/main.rs b/drv/gimlet-seq-server/src/main.rs index 38116d6f8..8ba1393aa 100644 --- a/drv/gimlet-seq-server/src/main.rs +++ b/drv/gimlet-seq-server/src/main.rs @@ -8,6 +8,7 @@ #![no_main] mod seq_spi; +mod vcore; use counters::*; use ringbuf::*; @@ -52,6 +53,7 @@ enum I2cTxn { SpdLoadTop(u8, u8), VCoreOn, VCoreOff, + VCoreUndervoltageInitialize, SocOn, SocOff, } @@ -181,6 +183,7 @@ struct ServerImpl { seq: seq_spi::SequencerFpga, jefe: Jefe, hf: hf_api::HostFlash, + vcore: vcore::VCore, deadline: u64, } @@ -462,6 +465,8 @@ impl ServerImpl { // Turn on the chassis LED once we reach A2 sys.gpio_set(CHASSIS_LED); + let (device, rail) = i2c_config::pmbus::vdd_vcore(I2C.get_task_id()); + let mut server = Self { state: PowerState::A2, sys: sys.clone(), @@ -469,6 +474,7 @@ impl ServerImpl { jefe, hf, deadline: 0, + vcore: vcore::VCore::new(sys, &device, rail), }; // Power on, unless suppressed by the `stay-in-a2` feature @@ -500,10 +506,18 @@ impl ServerImpl { impl NotificationHandler for ServerImpl { fn current_notification_mask(&self) -> u32 { - notifications::TIMER_MASK + notifications::TIMER_MASK | self.vcore.mask() } - fn handle_notification(&mut self, _bits: u32) { + fn handle_notification(&mut self, bits: u32) { + if (bits & self.vcore.mask()) != 0 { + self.vcore.handle_notification(); + } + + if (bits & notifications::TIMER_MASK) == 0 { + return; + } + let ifr = self.seq.read_byte(Addr::IFR).unwrap_lite(); ringbuf_entry!(Trace::Status { ier: self.seq.read_byte(Addr::IER).unwrap_lite(), @@ -685,6 +699,15 @@ impl ServerImpl { return Err(SeqError::MuxToHostCPUFailed); } + // + // If we fail to initialize our UV warning despite retries, we + // will drive on: the failures will be logged, and this isn't + // strictly required to sequence. + // + _ = retry_i2c_txn(I2cTxn::VCoreUndervoltageInitialize, || { + self.vcore.initialize_uv_warning() + }); + let start = sys_get_timer().now; let deadline = start + A0_TIMEOUT_MILLIS; diff --git a/drv/gimlet-seq-server/src/vcore.rs b/drv/gimlet-seq-server/src/vcore.rs new file mode 100644 index 000000000..dc9bc0131 --- /dev/null +++ b/drv/gimlet-seq-server/src/vcore.rs @@ -0,0 +1,156 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +/// +/// We have seen adventures on the V12_SYS_A2 rail in that it will sag from +/// 12V to ~8V over a period of about ~4ms, and then rise back 12V over ~7ms. +/// This happens only on very few machines, and even then happens very rarely +/// (happening once over hours or days), but the consequences are acute: the +/// dip in power results in U.2 drives resetting, and ultimately, the system +/// resetting itself. To better characterize any such dips, we want to use +/// one of the rails on of the RAA229618s (specifically, VDD_VCORE) as a +/// witness to any V12_SYS_A2 rail fluctuation via its VIN: we set its VIN +/// undervoltage warning limit to a value that is lower than any we expect in +/// an operable system (but higher than the sags we have observed), and then +/// configure its fault output (PWR_CONT1_VCORE_TO_SP_ALERT_L, connected to +/// PI14) to generate an interrupt on a falling edge. Upon the interrupt, we +/// will get notification here, and we will record values of VIN as quickly as +/// we can. Each READ_VIN requires 8 bytes, over 3 I2C transactions: +/// +/// [Write + PAGE + rail] [Write + READ_VIN] [Read + MSB + LSB] +/// +/// At our midbus speed of 100kHz, this is ~900µs per READ_VIN. We gather 50 +/// of these READ_VIN measurements, along with timestamps before and after the +/// operations, and put them all in a ring buffer. Note that we don't clear +/// faults after this condition; we will wait until the machine next makes an +/// A2 to A0 transition to clear faults. +/// +use drv_i2c_api::{I2cDevice, ResponseCode}; +use drv_i2c_devices::raa229618::Raa229618; +use drv_stm32xx_sys_api as sys_api; +use ringbuf::*; +use sys_api::{gpio_irq_pins::VCORE_TO_SP_ALERT_L, IrqControl}; +use userlib::*; + +pub struct VCore { + device: Raa229618, + sys: sys_api::Sys, +} + +#[derive(Copy, Clone, PartialEq)] +enum Trace { + Initializing, + Initialized, + LimitLoaded, + FaultsCleared, + Notified, + Fault, + Reading { timestamp: u64, volts: units::Volts }, + Error(ResponseCode), + None, +} + +ringbuf!(Trace, 120, Trace::None); + +/// +/// We are going to set our input undervoltage warn limit to be 11.75 volts. +/// Note that we will not fault if VIN goes below this (that is, we will not +/// lose POWER_GOOD), but the part will indicate an input fault and pull +/// PWR_CONT1_VCORE_TO_SP_ALERT_L low. +/// +const VCORE_UV_WARN_LIMIT: units::Volts = units::Volts(11.75); + +/// +/// We want to collect enough samples (at ~900µs per sample) to adequately +/// cover any anticipated dip. We have seen these have an ~11ms total width +/// in the wild, so we give ourselves plenty of margin here and get ~45ms +/// of data. +/// +const VCORE_NSAMPLES: usize = 50; + +cfg_if::cfg_if! { + if #[cfg(not(any( + target_board = "gimlet-b", + target_board = "gimlet-c", + target_board = "gimlet-d", + target_board = "gimlet-e", + target_board = "gimlet-f", + )))] { + compile_error!("RAA229618 VIN monitoring unsupported for this board"); + } +} + +impl VCore { + pub fn new(sys: &sys_api::Sys, device: &I2cDevice, rail: u8) -> Self { + Self { + device: Raa229618::new(device, rail), + sys: sys.clone(), + } + } + + pub fn mask(&self) -> u32 { + crate::notifications::VCORE_MASK + } + + pub fn initialize_uv_warning(&self) -> Result<(), ResponseCode> { + let sys = &self.sys; + + ringbuf_entry!(Trace::Initializing); + + // Set our warn limit + self.device.set_vin_uv_warn_limit(VCORE_UV_WARN_LIMIT)?; + ringbuf_entry!(Trace::LimitLoaded); + + // Clear our faults + self.device.clear_faults()?; + ringbuf_entry!(Trace::FaultsCleared); + + // Set our alert line to be an input + sys.gpio_configure_input(VCORE_TO_SP_ALERT_L, sys_api::Pull::None); + sys.gpio_irq_configure(self.mask(), sys_api::Edge::Falling); + + // Enable the interrupt! + let _ = self.sys.gpio_irq_control(self.mask(), IrqControl::Enable); + + ringbuf_entry!(Trace::Initialized); + + Ok(()) + } + + pub fn handle_notification(&self) { + let faulted = self.sys.gpio_read(VCORE_TO_SP_ALERT_L) == 0; + + ringbuf_entry!(Trace::Notified); + + if faulted { + ringbuf_entry!(Trace::Fault); + + for _ in 0..VCORE_NSAMPLES { + match self.device.read_vin() { + Ok(val) => { + // + // Record our reading, along with a timestamp. On the + // one hand, it's a little exceesive to record a + // timestamp on every reading: it's in milliseconds, + // and because it takes ~900µs per reading, we expect + // the timestamp to (basically) be incremented by 1 with + // every reading (with a duplicate timestamp occuring + // every ~7-9 entries). But on the other, it's not + // impossible to be preempted, and it's valuable to have + // as tight a coupling as possible between observed + // reading and observed time. + // + ringbuf_entry!(Trace::Reading { + timestamp: sys_get_timer().now, + volts: val, + }); + } + Err(code) => ringbuf_entry!(Trace::Error(code.into())), + } + } + } + + let _ = self.sys.gpio_irq_control(self.mask(), IrqControl::Enable); + } +} diff --git a/drv/i2c-devices/src/lib.rs b/drv/i2c-devices/src/lib.rs index fcd88294e..4ddafcb3f 100644 --- a/drv/i2c-devices/src/lib.rs +++ b/drv/i2c-devices/src/lib.rs @@ -117,6 +117,18 @@ macro_rules! pmbus_rail_phase_read { } macro_rules! pmbus_write { + ($device:expr, $cmd:ident) => {{ + let payload = [CommandCode::$cmd as u8]; + + match $device.write(&payload) { + Err(code) => Err(Error::BadWrite { + cmd: CommandCode::$cmd as u8, + code, + }), + Ok(_) => Ok(()), + } + }}; + ($device:expr, $cmd:ident, $data:expr) => {{ let mut payload = [0u8; $cmd::CommandData::len() + 1]; payload[0] = $cmd::CommandData::code(); diff --git a/drv/i2c-devices/src/raa229618.rs b/drv/i2c-devices/src/raa229618.rs index 350432076..c4a47fa32 100644 --- a/drv/i2c-devices/src/raa229618.rs +++ b/drv/i2c-devices/src/raa229618.rs @@ -115,6 +115,21 @@ impl Raa229618 { } } + pub fn clear_faults(&self) -> Result<(), Error> { + pmbus_write!(self.device, CLEAR_FAULTS) + } + + pub fn set_vin_uv_warn_limit(&self, value: Volts) -> Result<(), Error> { + let mut vin = VIN_UV_WARN_LIMIT::CommandData(0); + vin.set(pmbus::units::Volts(value.0))?; + pmbus_rail_write!(self.device, self.rail, VIN_UV_WARN_LIMIT, vin) + } + + pub fn read_vin(&self) -> Result { + let vin = pmbus_rail_read!(self.device, self.rail, READ_VIN)?; + Ok(Volts(vin.get()?.0)) + } + pub fn read_phase_current(&self, phase: Phase) -> Result { let iout = pmbus_rail_phase_read!( self.device, From 33624ea999ea19e520a345d839cc0495142aa390 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 15:00:07 -0700 Subject: [PATCH 04/20] Fix static mut access warnings. This only covers cases that appeared trivially correct on local inspection. --- task/control-plane-agent/src/mgs_gimlet.rs | 6 +++--- task/control-plane-agent/src/mgs_gimlet/host_phase2.rs | 2 +- task/host-sp-comms/src/main.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/task/control-plane-agent/src/mgs_gimlet.rs b/task/control-plane-agent/src/mgs_gimlet.rs index ffeca002f..c6209f3fe 100644 --- a/task/control-plane-agent/src/mgs_gimlet.rs +++ b/task/control-plane-agent/src/mgs_gimlet.rs @@ -1428,7 +1428,7 @@ fn claim_mgs_to_sp_usart_buf_static( // the AtomicBool swap above, combined with the lexical scoping of // `UART_TX_BUF`, means that this reference can't be aliased by any // other reference in the program. - unsafe { &mut UART_TX_BUF } + unsafe { &mut *core::ptr::addr_of_mut!(UART_TX_BUF) } } fn claim_sp_to_mgs_usart_buf_static( @@ -1445,7 +1445,7 @@ fn claim_sp_to_mgs_usart_buf_static( // the AtomicBool swap above, combined with the lexical scoping of // `UART_RX_BUF`, means that this reference can't be aliased by any // other reference in the program. - unsafe { &mut UART_RX_BUF } + unsafe { &mut *core::ptr::addr_of_mut!(UART_RX_BUF) } } fn claim_installinator_image_id_static() -> &'static mut InstallinatorImageIdBuf @@ -1461,5 +1461,5 @@ fn claim_installinator_image_id_static() -> &'static mut InstallinatorImageIdBuf // the AtomicBool swap above, combined with the lexical scoping of // `INSTALLINATOR_IMAGE_ID_BUF`, means that this reference can't be aliased // by any other reference in the program. - unsafe { &mut INSTALLINATOR_IMAGE_ID_BUF } + unsafe { &mut *core::ptr::addr_of_mut!(INSTALLINATOR_IMAGE_ID_BUF) } } diff --git a/task/control-plane-agent/src/mgs_gimlet/host_phase2.rs b/task/control-plane-agent/src/mgs_gimlet/host_phase2.rs index 2c2d3f8a9..c6b8ce5bb 100644 --- a/task/control-plane-agent/src/mgs_gimlet/host_phase2.rs +++ b/task/control-plane-agent/src/mgs_gimlet/host_phase2.rs @@ -331,5 +331,5 @@ fn claim_phase2_buffer( // the AtomicBool swap above, combined with the lexical scoping of // `PHASE2_BUF`, means that this reference can't be aliased by any // other reference in the program. - unsafe { &mut PHASE2_BUF } + unsafe { &mut *core::ptr::addr_of_mut!(PHASE2_BUF) } } diff --git a/task/host-sp-comms/src/main.rs b/task/host-sp-comms/src/main.rs index 2698b6b6b..7646de872 100644 --- a/task/host-sp-comms/src/main.rs +++ b/task/host-sp-comms/src/main.rs @@ -1342,7 +1342,7 @@ fn claim_uart_rx_buf() -> &'static mut Vec { // the AtomicBool swap above, combined with the lexical scoping of // `UART_RX_BUF`, means that this reference can't be aliased by any // other reference in the program. - unsafe { &mut UART_RX_BUF } + unsafe { &mut *core::ptr::addr_of_mut!(UART_RX_BUF) } } mod idl { From f19a8318214f78fd32f74d068f0ea94e0feff323 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 15:00:36 -0700 Subject: [PATCH 05/20] Fix "unused" constants. These are not actually unused, annoyingly. --- task/host-sp-comms/src/main.rs | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/task/host-sp-comms/src/main.rs b/task/host-sp-comms/src/main.rs index 7646de872..e7920ca0c 100644 --- a/task/host-sp-comms/src/main.rs +++ b/task/host-sp-comms/src/main.rs @@ -1038,20 +1038,22 @@ impl ServerImpl { sequence, &SpToHost::KeyLookupResult(KeyLookupResult::Ok), |buf| { - // `MIN_SP_TO_HOST_FILL_DATA_LEN` is calculated assuming - // `SpToHost::MAX_SIZE`, but we know in this callback - // we're appending to - // `SpToHost::KeyLookupResult(KeyLookupResult::Ok)`, - // which is only 2 bytes. Recompute the exact max space - // we have for our response, then statically guarantee - // we have sufficient space in `buf` for longest - // possible DTRACE_CONF blob. - const SP_TO_HOST_FILL_DATA_LEN: usize = - MIN_SP_TO_HOST_FILL_DATA_LEN + SpToHost::MAX_SIZE - - 2; - const_assert!( + const_assert!({ + // `MIN_SP_TO_HOST_FILL_DATA_LEN` is calculated + // assuming `SpToHost::MAX_SIZE`, but we know in + // this callback we're appending to + // `SpToHost::KeyLookupResult(KeyLookupResult::Ok)`, + // which is only 2 bytes. Recompute the exact max + // space we have for our response, then statically + // guarantee we have sufficient space in `buf` for + // longest possible DTRACE_CONF blob. + #[allow(dead_code)] // suppress warning in nightly + const SP_TO_HOST_FILL_DATA_LEN: usize = + MIN_SP_TO_HOST_FILL_DATA_LEN + + SpToHost::MAX_SIZE + - 2; SP_TO_HOST_FILL_DATA_LEN >= MAX_DTRACE_CONF_LEN - ); + }); buf[..response_len].copy_from_slice( &self.host_kv_storage.dtrace_conf[..response_len], From 0cc4c031d24a619985ae6a02c675d63c1830a6a1 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:02:17 -0700 Subject: [PATCH 06/20] Fix manual_unwrap_or_default warnings All _new_ instances of this warning firing are false positives, and are suppressed in this commit. --- drv/gimlet-hf-server/src/main.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drv/gimlet-hf-server/src/main.rs b/drv/gimlet-hf-server/src/main.rs index 49312a930..949d639a8 100644 --- a/drv/gimlet-hf-server/src/main.rs +++ b/drv/gimlet-hf-server/src/main.rs @@ -422,6 +422,8 @@ impl ServerImpl { addr: Option, raw_data: &RawPersistentData, ) -> Result<(), HfError> { + // Clippy misfire as of 2024-04 + #[allow(clippy::manual_unwrap_or_default)] let addr = match addr { Some(a) => a, None => { From cf938faf102745f16cdf1c14a9063f7d5fbce2c8 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:02:33 -0700 Subject: [PATCH 07/20] Fix enum default warning. Clippy would like us to automatically derive default for enums where possible instead of writing our own impl. I happen to agree with Clippy. --- drv/i2c-devices/src/pca9956b.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drv/i2c-devices/src/pca9956b.rs b/drv/i2c-devices/src/pca9956b.rs index 627a56478..d84fffe51 100644 --- a/drv/i2c-devices/src/pca9956b.rs +++ b/drv/i2c-devices/src/pca9956b.rs @@ -93,20 +93,15 @@ const MAX_AUTO_INC_REG: Register = Register::ALLCALLADR; const MAX_BUF_SIZE: usize = MAX_AUTO_INC_REG as usize; /// ERR representations per Table 21 of the datasheet -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[derive(Copy, Clone, Debug, PartialEq, Eq, Default)] pub enum LedErr { + #[default] NoError = 0b00, ShortCircuit = 0b01, OpenCircuit = 0b10, Invalid = 0b11, } -impl Default for LedErr { - fn default() -> Self { - LedErr::NoError - } -} - impl From for LedErr { fn from(i: u8) -> Self { match i { From 0d061704ca4e643f22ee7705756b1e5014926470 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:02:53 -0700 Subject: [PATCH 08/20] Fix "new without Default" warnings. This adds Default impls where it seemed appropriate and/or easy to do so, and suppresses the warning in cases where it'd just be dead code generated to appease the clipster. --- drv/stm32h7-eth/src/ring.rs | 3 +++ lib/update-buffer/src/lib.rs | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/drv/stm32h7-eth/src/ring.rs b/drv/stm32h7-eth/src/ring.rs index 23574a3c9..61169d38a 100644 --- a/drv/stm32h7-eth/src/ring.rs +++ b/drv/stm32h7-eth/src/ring.rs @@ -45,6 +45,7 @@ unsafe impl Sync for Buffer {} impl Buffer { /// Creates a zero-initialized buffer. + #[allow(clippy::new_without_default)] pub const fn new() -> Self { Self(UnsafeCell::new([0; BUFSZ])) } @@ -58,6 +59,7 @@ impl Buffer { /// When configured in VLAN mode, we write _two_ descriptors (each 4 bytes): /// - the configuration descriptor, which sets the VLAN for subsequent packets /// - the actual packet transmit descriptor +#[derive(Default)] #[repr(transparent)] pub struct TxDesc { /// Transmit descriptor @@ -348,6 +350,7 @@ impl TxRing { /// /// This is deliberately opaque to viewers outside this module, so that we can /// carefully control accesses to its contents. +#[derive(Default)] #[repr(transparent)] pub struct RxDesc { rdes: [AtomicU32; 4], diff --git a/lib/update-buffer/src/lib.rs b/lib/update-buffer/src/lib.rs index f5ae1d435..2d77a9391 100644 --- a/lib/update-buffer/src/lib.rs +++ b/lib/update-buffer/src/lib.rs @@ -22,7 +22,14 @@ pub struct UpdateBuffer { data: Mutex<[u8; N]>, } -impl UpdateBuffer { +impl Default for UpdateBuffer { + fn default() -> Self { + Self::new() + } +} + + +impl UpdateBuffer { pub const MAX_CAPACITY: usize = N; pub const fn new() -> Self { @@ -31,7 +38,9 @@ impl UpdateBuffer { data: Mutex::new([0; N]), } } +} +impl UpdateBuffer { /// Borrow this buffer with an artifical cap of `capacity`. /// /// On success, records that this buffer is owned by `owner` until the From 8f0ad28646d16c4611d0bcc456327e2d8d285ca7 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:03:04 -0700 Subject: [PATCH 09/20] Fix unnecessary Result::map_err warnings --- task/net/src/server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/task/net/src/server.rs b/task/net/src/server.rs index 9ef8ce55d..10d33bee5 100644 --- a/task/net/src/server.rs +++ b/task/net/src/server.rs @@ -171,7 +171,7 @@ where i: u16, ) -> Result> { if i >= 1024 { - return Err(KszError::BadMacIndex).map_err(RequestError::from); + return Err(RequestError::from(KszError::BadMacIndex)); } let (_eth, bsp) = self.eth_bsp(); let ksz8463 = bsp.ksz8463(); From 7475407ec02ff986471d05c1ae537392e0877042 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:02:53 -0700 Subject: [PATCH 10/20] Fix "new without Default" warnings. This adds Default impls where it seemed appropriate and/or easy to do so, and suppresses the warning in cases where it'd just be dead code generated to appease the clipster. --- lib/update-buffer/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/update-buffer/src/lib.rs b/lib/update-buffer/src/lib.rs index 2d77a9391..7ee8a6625 100644 --- a/lib/update-buffer/src/lib.rs +++ b/lib/update-buffer/src/lib.rs @@ -28,7 +28,6 @@ impl Default for UpdateBuffer { } } - impl UpdateBuffer { pub const MAX_CAPACITY: usize = N; From d0a3ca7a79f62bac70c6070005ccd9828e8f716d Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:03:04 -0700 Subject: [PATCH 11/20] Fix unnecessary Result::map_err warnings --- task/monorail-server/src/server.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/task/monorail-server/src/server.rs b/task/monorail-server/src/server.rs index 51b9ead80..3c3b5738b 100644 --- a/task/monorail-server/src/server.rs +++ b/task/monorail-server/src/server.rs @@ -365,9 +365,7 @@ impl<'a, R: Vsc7448Rw> idl::InOrderMonorailImpl for ServerImpl<'a, R> { if let Some(Err(e)) = self.bsp.phy_fn(port, |phy| { phy.read(phy::STANDARD::INTERRUPT_STATUS()) }) { - return Err(e) - .map_err(MonorailError::from) - .map_err(RequestError::from); + return Err(RequestError::from(MonorailError::from(e))); } // Clear the two bits that we use to detect link drops From c8b70403303983a97234fe088063d05b48e814ad Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:22:10 -0700 Subject: [PATCH 12/20] Fix unread fields intended for the debugger. --- task/hiffy/src/generic.rs | 4 +++- task/hiffy/src/lpc55.rs | 3 ++- task/hiffy/src/stm32g0.rs | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/task/hiffy/src/generic.rs b/task/hiffy/src/generic.rs index b5f5d9a0f..8b7c79c1f 100644 --- a/task/hiffy/src/generic.rs +++ b/task/hiffy/src/generic.rs @@ -5,7 +5,9 @@ use hif::Function; use hubris_num_tasks::Task; -pub struct Buffer(u8); +// This type is only used by the debugger apparently, so its field appears +// unused to the compiler. +pub struct Buffer(#[allow(dead_code)] u8); pub enum Functions { Sleep(u16, u32), diff --git a/task/hiffy/src/lpc55.rs b/task/hiffy/src/lpc55.rs index eff356ad7..d731c812b 100644 --- a/task/hiffy/src/lpc55.rs +++ b/task/hiffy/src/lpc55.rs @@ -26,7 +26,8 @@ enum Trace { ringbuf!(Trace, 64, Trace::None); -pub struct Buffer(u8); +// TODO: this type is copy-pasted in several modules +pub struct Buffer(#[allow(dead_code)] u8); // // The order in this enum must match the order in the functions array that diff --git a/task/hiffy/src/stm32g0.rs b/task/hiffy/src/stm32g0.rs index cb234b2b7..ad224ec5e 100644 --- a/task/hiffy/src/stm32g0.rs +++ b/task/hiffy/src/stm32g0.rs @@ -20,7 +20,8 @@ use drv_i2c_api::{ #[cfg(feature = "i2c")] task_slot!(I2C, i2c_driver); -pub struct Buffer(u8); +// TODO: this type is copy-pasted in several modules +pub struct Buffer(#[allow(dead_code)] u8); // // The order in this enum must match the order in the functions array that From f8dca129e4e083c2e0fbf0a3328a3cd512bc3b79 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:34:24 -0700 Subject: [PATCH 13/20] Fix unnecessary fallible conversion warnings. --- drv/sidecar-seq-server/src/front_io.rs | 4 +--- drv/sidecar-seq-server/src/main.rs | 2 +- lib/dice/src/mfg.rs | 3 +-- lib/lpc55-rot-startup/src/images.rs | 2 +- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/drv/sidecar-seq-server/src/front_io.rs b/drv/sidecar-seq-server/src/front_io.rs index f2b6c06c9..2aae1f459 100644 --- a/drv/sidecar-seq-server/src/front_io.rs +++ b/drv/sidecar-seq-server/src/front_io.rs @@ -83,9 +83,7 @@ impl FrontIOBoard { }); if let Err(e) = controller.load_bitstream(self.auxflash_task) { - ringbuf_entry!(Trace::FpgaBitstreamError( - u32::try_from(e).unwrap() - )); + ringbuf_entry!(Trace::FpgaBitstreamError(u32::from(e))); return Err(e); } diff --git a/drv/sidecar-seq-server/src/main.rs b/drv/sidecar-seq-server/src/main.rs index 2fe9c93a9..fe7096c0a 100644 --- a/drv/sidecar-seq-server/src/main.rs +++ b/drv/sidecar-seq-server/src/main.rs @@ -834,7 +834,7 @@ fn main() -> ! { .load_bitstream(AUXFLASH.get_task_id()) { Err(e) => { - let code = u32::try_from(e).unwrap_lite(); + let code = u32::from(e); ringbuf_entry!(Trace::FpgaBitstreamError(code)); // If this is an auxflash error indicating that we can't diff --git a/lib/dice/src/mfg.rs b/lib/dice/src/mfg.rs index 1b338867a..d2f94d273 100644 --- a/lib/dice/src/mfg.rs +++ b/lib/dice/src/mfg.rs @@ -187,8 +187,7 @@ impl<'a> SerialMfg<'a> { } fn send_ack(&mut self) -> Result<(), Error> { - let hash: MessageHash = - self.hash.finalize_fixed_reset().try_into().unwrap(); + let hash: MessageHash = self.hash.finalize_fixed_reset().into(); self.send_msg(MfgMessage::Ack(hash)) } diff --git a/lib/lpc55-rot-startup/src/images.rs b/lib/lpc55-rot-startup/src/images.rs index f7e7731c4..44b4c4806 100644 --- a/lib/lpc55-rot-startup/src/images.rs +++ b/lib/lpc55-rot-startup/src/images.rs @@ -145,7 +145,7 @@ impl Image { } } - hash.finalize().try_into().unwrap_lite() + hash.finalize().into() } pub fn get_image_version(&self) -> ImageVersion { From b76d982b22ac46d38c94013be0c74fb7918c8b5a Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:37:48 -0700 Subject: [PATCH 14/20] Fix unnecessary ? warnings. --- drv/lpc55-update-server/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index f0c497876..7a98c7eb3 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -910,7 +910,7 @@ fn copy_from_caboose_chunk( // Early exit if the caller didn't provide enough space in the lease let mut remaining = pos.end - pos.start; if remaining as usize > data.len() { - return Err(RequestError::Fail(ClientError::BadLease))?; + return Err(RequestError::Fail(ClientError::BadLease)); } const BUF_SIZE: usize = 128; @@ -938,7 +938,7 @@ fn copy_from_flash_range( // Early exit if the caller didn't provide enough space in the lease let mut remaining = pos.end - pos.start; if remaining as usize > data.len() { - return Err(RequestError::Fail(ClientError::BadLease))?; + return Err(RequestError::Fail(ClientError::BadLease)); } const BUF_SIZE: usize = 128; From 97db6ba2cfdc05c5256616fa230471f9b1ead88c Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:38:00 -0700 Subject: [PATCH 15/20] Fix overly complex match scrutinee warnings. --- drv/lpc55-sprot-server/src/handler.rs | 78 +++++++++++-------- task/monorail-server/src/server.rs | 107 ++++++++++++++------------ 2 files changed, 100 insertions(+), 85 deletions(-) diff --git a/drv/lpc55-sprot-server/src/handler.rs b/drv/lpc55-sprot-server/src/handler.rs index 8921054fc..0dff9ae44 100644 --- a/drv/lpc55-sprot-server/src/handler.rs +++ b/drv/lpc55-sprot-server/src/handler.rs @@ -122,16 +122,18 @@ impl<'a> Handler { tx_buf, ) } else { - match Response::pack_with_cb(&rsp_body, tx_buf, |buf| { - self.update - .read_raw_caboose( - slot, - start, - &mut buf[..blob_size], - ) - .map_err(|e| RspBody::Caboose(Err(e)))?; - Ok(blob_size) - }) { + let pack_result = + Response::pack_with_cb(&rsp_body, tx_buf, |buf| { + self.update + .read_raw_caboose( + slot, + start, + &mut buf[..blob_size], + ) + .map_err(|e| RspBody::Caboose(Err(e)))?; + Ok(blob_size) + }); + match pack_result { Ok(size) => size, Err(e) => Response::pack(&Ok(e), tx_buf), } @@ -151,12 +153,14 @@ impl<'a> Handler { tx_buf, ) } else { - match Response::pack_with_cb(&rsp_body, tx_buf, |buf| { - self.attest - .cert(index, offset, &mut buf[..size]) - .map_err(|e| RspBody::Attest(Err(e)))?; - Ok(size) - }) { + let pack_result = + Response::pack_with_cb(&rsp_body, tx_buf, |buf| { + self.attest + .cert(index, offset, &mut buf[..size]) + .map_err(|e| RspBody::Attest(Err(e)))?; + Ok(size) + }); + match pack_result { Ok(size) => size, Err(e) => Response::pack(&Ok(e), tx_buf), } @@ -168,12 +172,14 @@ impl<'a> Handler { lpc55_rom_data::FLASH_PAGE_SIZE <= drv_sprot_api::MAX_BLOB_SIZE ); - match Response::pack_with_cb(&rsp_body, tx_buf, |buf| { - self.update - .read_rot_page(page, &mut buf[..size]) - .map_err(|e| RspBody::Page(Err(e)))?; - Ok(size) - }) { + let pack_result = + Response::pack_with_cb(&rsp_body, tx_buf, |buf| { + self.update + .read_rot_page(page, &mut buf[..size]) + .map_err(|e| RspBody::Page(Err(e)))?; + Ok(size) + }); + match pack_result { Ok(size) => size, Err(e) => Response::pack(&Ok(e), tx_buf), } @@ -188,12 +194,14 @@ impl<'a> Handler { tx_buf, ) } else { - match Response::pack_with_cb(&rsp_body, tx_buf, |buf| { - self.attest - .log(offset, &mut buf[..size]) - .map_err(|e| RspBody::Attest(Err(e)))?; - Ok(size) - }) { + let pack_result = + Response::pack_with_cb(&rsp_body, tx_buf, |buf| { + self.attest + .log(offset, &mut buf[..size]) + .map_err(|e| RspBody::Attest(Err(e)))?; + Ok(size) + }); + match pack_result { Ok(size) => size, Err(e) => Response::pack(&Ok(e), tx_buf), } @@ -208,12 +216,14 @@ impl<'a> Handler { tx_buf, ) } else { - match Response::pack_with_cb(&rsp_body, tx_buf, |buf| { - self.attest - .attest(nonce, &mut buf[..write_size as usize]) - .map_err(|e| RspBody::Attest(Err(e)))?; - Ok(write_size as usize) - }) { + let pack_result = + Response::pack_with_cb(&rsp_body, tx_buf, |buf| { + self.attest + .attest(nonce, &mut buf[..write_size as usize]) + .map_err(|e| RspBody::Attest(Err(e)))?; + Ok(write_size as usize) + }); + match pack_result { Ok(size) => size, Err(e) => Response::pack(&Ok(e), tx_buf), } diff --git a/task/monorail-server/src/server.rs b/task/monorail-server/src/server.rs index 3c3b5738b..6a88d0a81 100644 --- a/task/monorail-server/src/server.rs +++ b/task/monorail-server/src/server.rs @@ -241,9 +241,10 @@ impl<'a, R: Vsc7448Rw> idl::InOrderMonorailImpl for ServerImpl<'a, R> { // Take the union of the "link changed" bit on the PHY and our // local sticky value (since the PHY bit is self-resetting) - let v = match self.bsp.phy_fn(port, |phy| { + let r = self.bsp.phy_fn(port, |phy| { phy.read(phy::STANDARD::INTERRUPT_STATUS()) - }) { + }); + let v = match r { // If there is no PHY present, then the PHY link down // indication is always false. None => false, @@ -456,58 +457,62 @@ impl<'a, R: Vsc7448Rw> idl::InOrderMonorailImpl for ServerImpl<'a, R> { port: u8, ) -> Result> { self.check_port(port)?; - match self.bsp.phy_fn(port, |phy| -> Result { - let (_id, ty) = Self::decode_phy_id(&phy)?; - let status = phy.read(phy::STANDARD::MODE_STATUS())?; - let media_link_up = (status.0 & (1 << 2)) != 0; - - // The VSC8504 is running in forced-speed protocol transfer mode. - // Experimentally, packets get through without MAC_LINK_STATUS - // set, and despite what "ENT-AN1175" says, I don't see anything - // in register 24G. As such, we'll be optimistic: if there's a - // valid QSGMII link and MAC_PCS_SIG_DETECT, then let's call it - // good. - let status = phy.read(phy::EXTENDED_3::MAC_SERDES_PCS_STATUS())?; - let mac_serdes = phy.read(phy::EXTENDED_3::MAC_SERDES_STATUS())?; - let qsgmii_mask = ty.qsgmii_okay_mask(); - let mac_link_up = match ty { - PhyType::Vsc8504 => { - if status.mac_pcs_sig_detect() == 0 { - LinkStatus::Down - } else if status.mac_sync_fail() != 0 - || status.mac_cgbad() != 0 - || (mac_serdes.0 & qsgmii_mask) != qsgmii_mask - { - LinkStatus::Error - } else { - LinkStatus::Up + let phy_result = + self.bsp.phy_fn(port, |phy| -> Result { + let (_id, ty) = Self::decode_phy_id(&phy)?; + let status = phy.read(phy::STANDARD::MODE_STATUS())?; + let media_link_up = (status.0 & (1 << 2)) != 0; + + // The VSC8504 is running in forced-speed protocol transfer mode. + // Experimentally, packets get through without MAC_LINK_STATUS + // set, and despite what "ENT-AN1175" says, I don't see anything + // in register 24G. As such, we'll be optimistic: if there's a + // valid QSGMII link and MAC_PCS_SIG_DETECT, then let's call it + // good. + let status = + phy.read(phy::EXTENDED_3::MAC_SERDES_PCS_STATUS())?; + let mac_serdes = + phy.read(phy::EXTENDED_3::MAC_SERDES_STATUS())?; + let qsgmii_mask = ty.qsgmii_okay_mask(); + let mac_link_up = match ty { + PhyType::Vsc8504 => { + if status.mac_pcs_sig_detect() == 0 { + LinkStatus::Down + } else if status.mac_sync_fail() != 0 + || status.mac_cgbad() != 0 + || (mac_serdes.0 & qsgmii_mask) != qsgmii_mask + { + LinkStatus::Error + } else { + LinkStatus::Up + } } - } - PhyType::Vsc8522 | PhyType::Vsc8552 | PhyType::Vsc8562 => { - if status.mac_link_status() == 0 { - LinkStatus::Down - } else if status.mac_sync_fail() != 0 - || status.mac_cgbad() != 0 - || status.mac_pcs_sig_detect() == 0 - || (mac_serdes.0 & qsgmii_mask) != qsgmii_mask - { - LinkStatus::Error - } else { - LinkStatus::Up + PhyType::Vsc8522 | PhyType::Vsc8552 | PhyType::Vsc8562 => { + if status.mac_link_status() == 0 { + LinkStatus::Down + } else if status.mac_sync_fail() != 0 + || status.mac_cgbad() != 0 + || status.mac_pcs_sig_detect() == 0 + || (mac_serdes.0 & qsgmii_mask) != qsgmii_mask + { + LinkStatus::Error + } else { + LinkStatus::Up + } } - } - }; + }; - Ok(PhyStatus { - ty, - mac_link_up, - media_link_up: if media_link_up { - LinkStatus::Up - } else { - LinkStatus::Down - }, - }) - }) { + Ok(PhyStatus { + ty, + mac_link_up, + media_link_up: if media_link_up { + LinkStatus::Up + } else { + LinkStatus::Down + }, + }) + }); + match phy_result { None => Err(MonorailError::NoPhy.into()), Some(r) => { r.map_err(MonorailError::from).map_err(RequestError::from) From 9692c2121c2025330ad84721bb9dc5fddbd11d90 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 17:48:33 -0700 Subject: [PATCH 16/20] Fixed unused import warnings. --- test/test-suite/src/main.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test-suite/src/main.rs b/test/test-suite/src/main.rs index bd840a8a5..4510f02bb 100644 --- a/test/test-suite/src/main.rs +++ b/test/test-suite/src/main.rs @@ -604,7 +604,6 @@ task_slot!(I2C, i2c_driver); #[cfg(feature = "fru-id-eeprom")] mod at24csw080 { use super::*; - use drv_i2c_devices::at24csw080::Error; use drv_i2c_devices::at24csw080::*; const EEPROM_SIZE: u16 = 1024; From 126594b419948aa46405d2c634351f0cf46b24bb Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:16:09 -0700 Subject: [PATCH 17/20] hiffy: suppress warning about being unsound This is the only module that uses static mut and unsafe in a way that is not obviously correct. In the interest of preventing regressions everywhere _else,_ I'm suppressing the relevant warning for this module only. --- task/hiffy/src/main.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/task/hiffy/src/main.rs b/task/hiffy/src/main.rs index 50eec44c7..55075df96 100644 --- a/task/hiffy/src/main.rs +++ b/task/hiffy/src/main.rs @@ -14,6 +14,15 @@ #![no_std] #![no_main] +// +// TODO: Hiffy is using unsafe and static mut in ways that are not obviously +// sound. This became a warning in early 2024. In the interest of preventing +// regressions in everything _else_ I'm suppressing the warning here so we can +// turn Clippy back on. If you're reading this, this file is potentially unsound +// and needs attention! +// +#![allow(static_mut_refs)] + // This trait may not be needed, if compiling for a non-armv6m target. #[allow(unused_imports)] use armv6m_atomic_hack::AtomicU32Ext; From 1469a4d7938e73d4549915ea6466e3258593b9d4 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 9 Apr 2024 15:23:09 -0500 Subject: [PATCH 18/20] Fix two warnings that crept in. --- build/stm32xx-sys/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build/stm32xx-sys/src/lib.rs b/build/stm32xx-sys/src/lib.rs index 0018f734d..fa0687dd7 100644 --- a/build/stm32xx-sys/src/lib.rs +++ b/build/stm32xx-sys/src/lib.rs @@ -160,7 +160,7 @@ impl SysConfig { }, ) in &self.gpio_irqs { - match dispatch_table.get_mut(pin as usize) { + match dispatch_table.get_mut(pin) { Some(Some(curr)) => { anyhow::bail!("pin {pin} is already mapped to IRQ {curr:?}") } @@ -242,7 +242,7 @@ impl SysConfig { fn to_const_name(mut s: String) -> anyhow::Result { s.make_ascii_uppercase(); - let s = s.replace("-", "_"); + let s = s.replace('-', "_"); syn::parse_str::(&s) .with_context(|| format!("`{s}` is not a valid Rust identifier")) } From 80b664529a0da333056e9404f7ac7357239bc729 Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Tue, 9 Apr 2024 15:34:05 -0500 Subject: [PATCH 19/20] Remove unused fields in PSC CPA --- task/control-plane-agent/src/mgs_psc.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/task/control-plane-agent/src/mgs_psc.rs b/task/control-plane-agent/src/mgs_psc.rs index 579986064..59e4d0914 100644 --- a/task/control-plane-agent/src/mgs_psc.rs +++ b/task/control-plane-agent/src/mgs_psc.rs @@ -46,8 +46,6 @@ static UPDATE_MEMORY: UpdateBuffer = UpdateBuffer::new(); pub(crate) struct MgsHandler { common: MgsCommon, - sp_update: SpUpdate, - rot_update: RotUpdate, user_leds: UserLeds, } @@ -57,8 +55,6 @@ impl MgsHandler { pub(crate) fn claim_static_resources(base_mac_address: MacAddress) -> Self { Self { common: MgsCommon::claim_static_resources(base_mac_address), - sp_update: SpUpdate::new(), - rot_update: RotUpdate::new(), user_leds: UserLeds::from(USER_LEDS.get_task_id()), } } From 1522fc4f6b93c6257ccd4b176c5854bd74d3051d Mon Sep 17 00:00:00 2001 From: "Cliff L. Biffle" Date: Thu, 4 Apr 2024 16:10:28 -0700 Subject: [PATCH 20/20] Turn Clippy checks back on. --- .github/workflows/build-one.yml | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build-one.yml b/.github/workflows/build-one.yml index 895de6393..8f8b22b27 100644 --- a/.github/workflows/build-one.yml +++ b/.github/workflows/build-one.yml @@ -79,14 +79,10 @@ jobs: target/release/humility -a target/${{ inputs.app_name }}/dist/build-${{ inputs.app_name }}-image-$image.zip manifest; \ done - # 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 + - 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