From 4deb5783cfbec0610f3e3891b88b21d828e27c2f Mon Sep 17 00:00:00 2001 From: Bryan Cantrill Date: Wed, 21 Jun 2023 16:26:47 -0700 Subject: [PATCH 1/2] wip --- drv/i2c-api/src/lib.rs | 4 +-- drv/stm32xx-i2c-server/src/main.rs | 40 ++++++++++++++++++++++++------ drv/stm32xx-i2c/src/lib.rs | 2 +- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/drv/i2c-api/src/lib.rs b/drv/i2c-api/src/lib.rs index 901147bfb..02f324d31 100644 --- a/drv/i2c-api/src/lib.rs +++ b/drv/i2c-api/src/lib.rs @@ -97,8 +97,8 @@ pub enum ResponseCode { SegmentDisconnected, /// Mux disconnected during operation MuxDisconnected, - /// Address used for mux in-band management is invalid - BadMuxAddress, + /// No device at address used for mux in-band management + MuxMissing, /// Register used for mux in-band management is invalid BadMuxRegister, /// I2C bus was spontaneously reset during operation diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index f8ee88e95..7995216c8 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -149,13 +149,38 @@ fn configure_mux( Some(MuxState::Unknown) => { // - // We are in an unknown mux state. Before we can do anything, - // we need to successfully talk to every mux, and disable every - // segment. If there is any failure through here, we'll just - // return the error, leaving our mux state as unknown. + // We are in an unknown mux state. Before we can do anything, we + // need to successfully talk to every mux (or successfully learn + // that the mux is gone entirely!), and disable every segment. If + // there is any failure through here that isn't the mux being + // affirmatively gone, we'll just return the error, leaving our + // mux state as unknown. // all_muxes(controller, port, muxes, |mux| { - mux.driver.enable_segment(mux, controller, None, ctrl) + match mux.driver.enable_segment(mux, controller, None, ctrl) { + Err(code) if code == ResponseCode::MuxMissing => { + // + // The mux is gone entirely. We really don't expect + // this on any production system, but it can be true on + // some special lab systems (you know who you are!). + // Regardless of its origin, we can limit the blast + // radius in this case: if the mux is affirmatively + // gone (that is, no device is acking its address), we + // can assume that the mux is absent rather than + // Byzantine -- and therefore assume that its segments + // are as good as disabled and allow other traffic on + // the bus. So on this error (and only this error), we + // note that we saw it, and drive on. (Note that + // attempting to speak to a device on a segment on the + // missing mux will properly return MuxMissing -- and + // set our bus's mux state to be unknown.) + // + ringbuf_entry!(Trace::MuxMissing(mux.address)); + Ok(()) + } + Err(code) => Err(code), + Ok(()) => Ok(()), + } })?; // @@ -208,7 +233,8 @@ enum Trace { Reset((Controller, PortIndex)), MuxUnknown((Controller, PortIndex)), MuxUnknownRecover((Controller, PortIndex)), - ResetMux(Mux), + MuxMissing(u8), + ResetMux(u8), SegmentFailed(ResponseCodeU8), ConfigureFailed(ResponseCodeU8), Wiggles(u8), @@ -234,7 +260,7 @@ fn reset( // And now reset all muxes on this bus, eating any errors. let _ = all_muxes(controller, port, muxes, |mux| { - ringbuf_entry!(Trace::ResetMux(mux.id)); + ringbuf_entry!(Trace::ResetMux(mux.address)); let _ = mux.driver.reset(mux, &sys); Ok(()) }); diff --git a/drv/stm32xx-i2c/src/lib.rs b/drv/stm32xx-i2c/src/lib.rs index ebdf40bdc..2a821d7ea 100644 --- a/drv/stm32xx-i2c/src/lib.rs +++ b/drv/stm32xx-i2c/src/lib.rs @@ -176,7 +176,7 @@ impl I2cMux<'_> { use drv_i2c_api::ResponseCode; match code { - ResponseCode::NoDevice => ResponseCode::BadMuxAddress, + ResponseCode::NoDevice => ResponseCode::MuxMissing, ResponseCode::NoRegister => ResponseCode::BadMuxRegister, ResponseCode::BusLocked => ResponseCode::BusLockedMux, ResponseCode::BusReset => ResponseCode::BusResetMux, From 78741ce3eaf9ee521aaab0a686e7a7a9fc7f7900 Mon Sep 17 00:00:00 2001 From: Bryan Cantrill Date: Wed, 21 Jun 2023 17:18:41 -0700 Subject: [PATCH 2/2] code review comments from Cliff --- drv/stm32xx-i2c-server/src/main.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drv/stm32xx-i2c-server/src/main.rs b/drv/stm32xx-i2c-server/src/main.rs index 7995216c8..9933a972a 100644 --- a/drv/stm32xx-i2c-server/src/main.rs +++ b/drv/stm32xx-i2c-server/src/main.rs @@ -158,7 +158,7 @@ fn configure_mux( // all_muxes(controller, port, muxes, |mux| { match mux.driver.enable_segment(mux, controller, None, ctrl) { - Err(code) if code == ResponseCode::MuxMissing => { + Err(ResponseCode::MuxMissing) => { // // The mux is gone entirely. We really don't expect // this on any production system, but it can be true on @@ -178,8 +178,7 @@ fn configure_mux( ringbuf_entry!(Trace::MuxMissing(mux.address)); Ok(()) } - Err(code) => Err(code), - Ok(()) => Ok(()), + other => other, } })?;