Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

better handle missing I2C muxes #1425

Merged
merged 3 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions drv/i2c-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 33 additions & 7 deletions drv/stm32xx-i2c-server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
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
// 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),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these lines are equivalent to other => other, fwiw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯🤯🤯 HOW LONG HAS THAT BEEN THERE?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a really useful pattern (ha!) that I basically assumed (for terrible reasons?) didn't/wouldn't work.

Ok(()) => Ok(()),
}
})?;

//
Expand Down Expand Up @@ -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),
Expand All @@ -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(())
});
Expand Down
2 changes: 1 addition & 1 deletion drv/stm32xx-i2c/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down