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

better handle missing I2C muxes #1425

merged 3 commits into from
Jun 22, 2023

Conversation

bcantrill
Copy link
Collaborator

@bcantrill bcantrill commented Jun 21, 2023

Draft commit comment:

After the fix to #1413, the system is unwilling to ignore mux related
errors -- which has generated a problem on some lab systems that are
missing muxes:  because we can never get the bus into a known mux
state these systems now become entirely unusable, as even devices that
aren't on the missing segments are not accessible.  This fixes that by
observing that a mux that is affirmatively missing -- that is, one
that doesn't reply to its in-band management at all -- can be assumed
to have segments that are similarly missing (and therefore as good as
disabled).  This (naturally) doesn't change the fact that accessing
any devices attached to the missing mux will generate an error, but it
allows the system to broadly drive on absent accesses to those
devices.

@bcantrill bcantrill changed the title wip better handle missing I2C muxes Jun 21, 2023
@bcantrill bcantrill marked this pull request as ready for review June 22, 2023 00:01
Copy link
Collaborator

@cbiffle cbiffle left a comment

Choose a reason for hiding this comment

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

Huh, this diff is way smaller than I was expecting. So the key is just permitting a single error condition on the enable_segment path?

//
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) => {

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.

@bcantrill
Copy link
Collaborator Author

Huh, this diff is way smaller than I was expecting. So the key is just permitting a single error condition on the enable_segment path?

Yeah -- the other paths actually handle themselves, it turns out. When we treat a missing mux as implying that all of its segments are in fact disabled, everything else just... works.

@bcantrill bcantrill enabled auto-merge (squash) June 22, 2023 00:26
@bcantrill bcantrill merged commit 9932687 into master Jun 22, 2023
51 checks passed
@cbiffle cbiffle deleted the mux-fix-frfrfr-ikr branch July 10, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants