reading BMR491 PMBus data over Idol hangs bus (#1579)

oxidecomputer · Dec 13, 2023 · 5acdace · 5acdace
1 parent 8e3caaa
commit 5acdace
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 4 deletions.
diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml
@@ -124,7 +124,7 @@ notifications = ["timer"]
 name = "task-power"
 features = ["gimlet"]
 priority = 6
-max-sizes = {flash = 32768, ram = 8192 }
+max-sizes = {flash = 65536, ram = 8192 }
 stacksize = 1504
 start = true
 task-slots = ["i2c_driver", "sensor", "gimlet_seq"]

diff --git a/drv/i2c-types/src/lib.rs b/drv/i2c-types/src/lib.rs
@@ -106,6 +106,8 @@ pub enum ResponseCode {
     OperationNotSupported,
     /// Illegal number of leases
     IllegalLeaseCount,
+    /// Too much data -- or not enough buffer
+    TooMuchData,
 }
 
 ///

diff --git a/drv/stm32xx-i2c/src/lib.rs b/drv/stm32xx-i2c/src/lib.rs
@@ -159,6 +159,7 @@ enum Trace {
     KonamiISR(u32),
     Konami(I2cKonamiCode),
     ResetISR(u32),
+    ResetCR2(u32),
     AddrISR(u32),
     AddrMatch,
     AddrNack(u8),
@@ -434,6 +435,8 @@ impl I2cController<'_> {
 
         // And then finally set it
         i2c.cr1.modify(|_, w| w.pe().set_bit());
+
+        ringbuf_entry!(Trace::ResetCR2(i2c.cr2.read().bits()));
     }
 
     ///
@@ -555,6 +558,7 @@ impl I2cController<'_> {
             i2c.cr2.modify(|_, w| { w
                 .nbytes().bits(wlen as u8)
                 .autoend().clear_bit()
+                .reload().clear_bit()
                 .add10().clear_bit()
                 .sadd().bits((addr << 1).into())
                 .rd_wrn().clear_bit()
@@ -614,6 +618,8 @@ impl I2cController<'_> {
             }
         }
 
+        let mut overrun = false;
+
         if rlen != ReadLength::Fixed(0) {
             //
             // If we have both a write and a read, we deliberately do not send
@@ -626,6 +632,7 @@ impl I2cController<'_> {
                 i2c.cr2.modify(|_, w| { w
                     .nbytes().bits(rlen as u8)
                     .autoend().clear_bit()
+                    .reload().clear_bit()
                     .add10().clear_bit()
                     .sadd().bits((addr << 1).into())
                     .rd_wrn().set_bit()
@@ -686,7 +693,15 @@ impl I2cController<'_> {
                     continue;
                 }
 
-                putbyte(pos, byte).ok_or(drv_i2c_api::ResponseCode::BadArg)?;
+                if !overrun && putbyte(pos, byte).is_none() {
+                    //
+                    // If we're unable to accept what we just read, we need to
+                    // keep reading to complete the transfer -- but we will
+                    // not call putbyte again and we will return failure.
+                    //
+                    overrun = true;
+                }
+
                 pos += 1;
             }
 
@@ -712,7 +727,11 @@ impl I2cController<'_> {
         //
         i2c.cr2.modify(|_, w| w.stop().set_bit());
 
-        Ok(())
+        if overrun {
+            Err(drv_i2c_api::ResponseCode::TooMuchData)
+        } else {
+            Ok(())
+        }
     }
 
     ///
@@ -751,6 +770,7 @@ impl I2cController<'_> {
             i2c.cr2.modify(|_, w| { w
                 .nbytes().bits(0u8)
                 .autoend().clear_bit()
+                .reload().clear_bit()
                 .add10().clear_bit()
                 .sadd().bits((addr << 1).into())
                 .rd_wrn().bit(opval)

diff --git a/task/power-api/src/lib.rs b/task/power-api/src/lib.rs
@@ -79,7 +79,9 @@ pub enum Operation {
     MfrMaxTemp3,
 }
 
-pub const MAX_BLOCK_LEN: usize = 17;
+// This is a bit of an arbitrary number, but is set to the maximum read block
+// size that we expect given the devices that we interact with.
+pub const MAX_BLOCK_LEN: usize = 32;
 
 // We use a `u8` for the actual block length; ensure `MAX_BLOCK_LEN` fits.
 static_assertions::const_assert!(MAX_BLOCK_LEN <= u8::MAX as usize);