oxidecomputer / oxidecomputer/hubris

i2c_api does not properly deal with i2c_driver restart

Open
#1,440 0 comments 0 reactions 1 assignee View on GitHub

@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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.