Repository navigation
transceivers: mitigate "stuck" front-io I2C - #2755
jamesmunns wants to merge 1 commit into
Conversation
In #2070, we have seen that the initial FPGA timeout does not necessarily fire in all cases, as #2360 had hoped. This PR adds an additional layer of mitigation that will "give up" waiting for an I2C transaction from completing after 50ms (open to thoughts on this time). This failure state can also be used to mark a port as disabled, meaning that we will no longer attempt to communicate with this port. It is not currently known what is necessary to restore the functionality of a "stuck" port, it may require an insertion cycle of the sidecar. For now, this PR does add ringbuf entries which can be used for diagnosis, we may want to also add ereports in a follow-up PR.
labbott
left a comment
labbott
left a comment
There was a problem hiding this comment.
I have some questions about the value of the ringbuf entries but I think the logic looks okay (you probably also want to get a better review from some of the i2c fans/haters)
| ringbuf_entry!(Trace::StillBusy { | ||
| port: PortLocation { | ||
| controller: fpga_index, | ||
| port, | ||
| }, | ||
| status: status_all.status[port.0 as usize], | ||
| }); |
There was a problem hiding this comment.
I'm having a hard time judging how much the ringbuf will potentially fill up with this message. I think once we hit the timeout it will stop so it won't be infinite. Is this the correct amount of logging you expect? I'm also having a hard time judging if this is actually going to be useful for debugging.
| let error = mask & LogicalPortMask::from(physical_error); | ||
| let failure = mask & LogicalPortMask::from(physical_failure); | ||
| let success = mask & !(error | failure); | ||
| ModuleResult::new(success, failure, error, failure_types).unwrap_lite() |
There was a problem hiding this comment.
This doesn't need to be addressed in this PR but I feel like something is off that we keep calling unwrap on so many of these ModuleResult
| PollTimeout { port: PortLocation, status: u8 }, | ||
| /// A port was busy when we tried to start a transaction. | ||
| StillBusy { port: PortLocation, status: u8 }, |
There was a problem hiding this comment.
PortLocation implements Count, right? should we put #[count(children)] on these?
In #2070, we have seen that the initial FPGA timeout does not necessarily fire in all cases, as #2360 had hoped. This PR adds an additional layer of mitigation that will "give up" waiting for an I2C transaction from completing after 50ms (open to thoughts on this time).
This failure state can also be used to mark a port as disabled, meaning that we will no longer attempt to communicate with this port.
It is not currently known what is necessary to restore the functionality of a "stuck" port, it may require an insertion cycle of the sidecar.
For now, this PR does add ringbuf entries which can be used for diagnosis, we may want to also add ereports in a follow-up PR.
This still needs on-target testing, particularly to figure out how to induce this state to validate the fix is reasonable, and maybe checking with up-stack folks to let them know this is a failure mode that can happen now (the SP disables a port because we think it is stuck).