oxidecomputer / oxidecomputer/hubris
i2c_api does not properly deal with i2c_driver restart
@bcantrill is already working on this.
Since Jun 26, 2023.
- Dominant language
- Rust
- Stars
- 3.6k
- Forks
- 239
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 23
Description
When we saw issue #1439, the i2c_driver restarted -- but the tasks that were talking to the i2c_driver simply were unable to do any additional I2C transactions.
Here is a dump with the SP in this state. Ring buffers are filled with the likes of:
$ humility ringbuf thermal
humility: attached to dump
humility: ring buffer task_thermal::__RINGBUF in thermal:
NDX LINE GEN COUNT PAYLOAD
25 898 24 1 ControlPwm(0x0)
26 315 24 1 ControlError(DeviceError)
27 575 24 1 FanReadFailed(SensorId(0x65), BadResponse)
28 575 24 1 FanReadFailed(SensorId(0x66), BadResponse)
29 575 24 1 FanReadFailed(SensorId(0x67), BadResponse)
30 575 24 1 FanReadFailed(SensorId(0x68), BadResponse)
31 575 24 1 FanReadFailed(SensorId(0x69), BadResponse)
0 575 25 1 FanReadFailed(SensorId(0x6a), BadResponse)
1 595 25 1 MiscReadFailed(SensorId(0x0), I2cError(BadResponse))
2 595 25 1 MiscReadFailed(SensorId(0x2), I2cError(BadResponse))
3 595 25 1 MiscReadFailed(SensorId(0x70), I2cError(BadResponse))
4 595 25 1 MiscReadFailed(SensorId(0x6e), I2cError(BadResponse))
5 595 25 1 MiscReadFailed(SensorId(0x6f), I2cError(BadResponse))
6 595 25 1 MiscReadFailed(SensorId(0x1), I2cError(BadResponse))
7 621 25 1 SensorReadFailed(SensorId(0x75), I2cError(BadResponse))
8 621 25 1 SensorReadFailed(SensorId(0x76), I2cError(BadResponse))
9 621 25 1 SensorReadFailed(SensorId(0x77), I2cError(BadResponse))
10 621 25 1 SensorReadFailed(SensorId(0x78), I2cError(BadResponse))
11 621 25 1 SensorReadFailed(SensorId(0x79), I2cError(BadResponse))
12 621 25 1 SensorReadFailed(SensorId(0x7a), I2cError(BadResponse))
13 621 25 1 SensorReadFailed(SensorId(0x7b), I2cError(BadResponse))
14 621 25 1 SensorReadFailed(SensorId(0x7c), I2cError(BadResponse))
15 621 25 1 SensorReadFailed(SensorId(0x7d), I2cError(BadResponse))
16 621 25 1 SensorReadFailed(SensorId(0x7e), I2cError(BadResponse))
17 621 25 1 SensorReadFailed(SensorId(0x7f), I2cError(BadResponse))
18 621 25 1 SensorReadFailed(SensorId(0x80), I2cError(BadResponse))
19 621 25 1 SensorReadFailed(SensorId(0x81), I2cError(BadResponse))
20 621 25 1 SensorReadFailed(SensorId(0x82), I2cError(BadResponse))
21 621 25 1 SensorReadFailed(SensorId(0x83), I2cError(BadResponse))
22 621 25 1 SensorReadFailed(SensorId(0x84), I2cError(BadResponse))
23 898 25 1 ControlPwm(0x0)
24 315 25 1 ControlError(DeviceError)
The problem is that the I2C API isn't properly dealing with the task resets here (and erroneously treating it as a bad response). This code pre-dates Idol (which handles this for clients automatically) and unfortunately we can't handle it the same way here as we do for Idol clients: we don't have mut references where we need them. It might be reasonable to add additional entry points that take mut references and are therefore able to update the task state, but these resets should be very rare (viz. that we haven't hit this bug despite its longevity!) it seems that the best approach for now is to explicitly panic on any i2c_driver reset. Leaving this as a panic (rather than a reset) is both simpler (it does not necessitate adding a Jefe task slot to every consumer of i2c_driver and not unreasonable, as it can be helpful to have a task dump under these circumstances. Finally, note that as a practical matter, this will only affect those tasks that cache the i2c_driver task ID (i.e., thermal and power); those that load it on use (e.g., everyone else) will be practically unaffected by a restart.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.