transceivers: fix I2C read bug (#1787)

#1768 did not properly account for the FIFO behavior of the FPGA's data
buffers. The "check the status byte" portion of the loop happened
outside the part where we read the buffer, and since the buffer was just
memory-mapped registers it could be repeatedly without consequence.
Since the data was now in a FIFO, I was inadvertently draining the FIFO
before the transaction was done. This PR consolidates the "is I2C done
yet" logic into the `get_i2c_status_and_read_buffer` so calling code can
just deal with the status register and the data buffer.

Fixes #1786

Author: Aaron-Hartwig

drv/sidecar-front-io/src/transceivers.rs

@@ -677,13 +677,22 @@ impl core::ops::Index<LogicalPort> for LogicalPortFailureTypes {
 
 #[derive(Copy, Clone)]
 pub struct PortI2CStatus {
+    pub rdata_fifo_empty: bool,
+    pub wdata_fifo_empty: bool,
     pub done: bool,
     pub error: FpgaI2CFailure,
 }
 
 impl PortI2CStatus {
     pub fn new(status: u8) -> Self {
+        // Use QSFP::PORT0 for constants, since they're all identical
         Self {
+            rdata_fifo_empty: (status
+                & Reg::QSFP::PORT0_STATUS::RDATA_FIFO_EMPTY)
+                != 0,
+            wdata_fifo_empty: (status
+                & Reg::QSFP::PORT0_STATUS::WDATA_FIFO_EMPTY)
+                != 0,
             done: (status & Reg::QSFP::PORT0_STATUS::BUSY) == 0,
             error: FpgaI2CFailure::try_from(status).unwrap_lite(),
         }
@@ -1028,13 +1037,13 @@ impl Transceivers {
             .read(Addr::QSFP_PORT0_STATUS as u16 + port_loc.port.0 as u16)
     }
 
-    /// Get `buf.len()` bytes of data, where the first byte is port status and
-    /// trailing bytes are the I2C read buffer for a `port`. The buffer stores
-    /// data from the last I2C read transaction done and thus only the number of
-    /// bytes read will be valid in the buffer.
+    /// Returns a PortI2CStatus and will fill `buf.len()` bytes of data with the
+    /// I2C read buffer for a `port`. The buffer stores data from the last I2C
+    /// read transaction done and thus only the number of bytes read will be
+    /// valid in the buffer.
     ///
     /// Poll the status register for a port until an I2C transaction is done.
-    /// Upon completion, pass the status byte back alongside the read data. The
+    /// Upon completion, return the status struct back and the read data. The
     /// read data buffer stores I2C read data from the last transaction so the
     /// FIFO will only contain that many bytes. The FPGA automatically clears
     /// the read data FIFO before starting a new read, so there's no need for
@@ -1043,14 +1052,23 @@ impl Transceivers {
         &self,
         port: P,
         buf: &mut [u8],
-    ) -> Result<(), FpgaError> {
+    ) -> Result<PortI2CStatus, FpgaError> {
         let port_loc = port.into();
-        buf[0] = self.get_i2c_status(port_loc)?;
-        self.fpga(port_loc.controller).read_bytes(
-            ReadOp::ReadNoAddrIncr,
-            Addr::QSFP_PORT0_I2C_DATA as u16 + port_loc.port.0 as u16,
-            &mut buf[1..],
-        )
+        loop {
+            let status_byte = self.get_i2c_status(port_loc)?;
+            let status = PortI2CStatus::new(status_byte);
+
+            if status.done {
+                self.fpga(port_loc.controller).read_bytes(
+                    ReadOp::ReadNoAddrIncr,
+                    Addr::QSFP_PORT0_I2C_DATA as u16 + port_loc.port.0 as u16,
+                    buf,
+                )?;
+                return Ok(status);
+            }
+
+            userlib::hl::sleep_for(1);
+        }
     }
 
     /// Write `buf.len()` bytes of data into the I2C write buffer of the `mask`

drv/transceivers-server/src/main.rs

@@ -16,7 +16,9 @@ use drv_fpga_api::FpgaError;
 use drv_i2c_devices::pca9956b::Error;
 use drv_sidecar_front_io::{
     leds::{FullErrorSummary, Leds},
-    transceivers::{LogicalPort, LogicalPortMask, Transceivers},
+    transceivers::{
+        FpgaI2CFailure, LogicalPort, LogicalPortMask, Transceivers,
+    },
     Reg,
 };
 use drv_sidecar_seq_api::{SeqError, Sequencer};
@@ -253,28 +255,24 @@ impl ServerImpl {
 
         #[derive(Copy, Clone, FromBytes, AsBytes)]
         #[repr(C)]
-        struct StatusAndTemperature {
-            status: u8,
+        struct Temperature {
             temperature: zerocopy::I16<zerocopy::BigEndian>,
         }
 
-        loop {
-            let mut out = StatusAndTemperature::new_zeroed();
-            self.transceivers
-                .get_i2c_status_and_read_buffer(port, out.as_bytes_mut())?;
-            if out.status & Reg::QSFP::PORT0_STATUS::BUSY == 0 {
-                if out.status & Reg::QSFP::PORT0_STATUS::ERROR != 0 {
-                    return Err(FpgaError::ImplError(out.status));
-                } else {
-                    // "Internally measured free side device temperatures are
-                    // represented as a 16-bit signed twos complement value in
-                    // increments of 1/256 degrees Celsius"
-                    //
-                    // - SFF-8636 rev 2.10a, Section 6.2.4
-                    return Ok(Celsius(out.temperature.get() as f32 / 256.0));
-                }
-            }
-            userlib::hl::sleep_for(1);
+        let mut out = Temperature::new_zeroed();
+        let status = self
+            .transceivers
+            .get_i2c_status_and_read_buffer(port, out.as_bytes_mut())?;
+
+        if status.error == FpgaI2CFailure::NoError {
+            // "Internally measured free side device temperatures are
+            // represented as a 16-bit signed twos complement value in
+            // increments of 1/256 degrees Celsius"
+            //
+            // - SFF-8636 rev 2.10a, Section 6.2.4
+            Ok(Celsius(out.temperature.get() as f32 / 256.0))
+        } else {
+            Err(FpgaError::ImplError(status.error as u8))
         }
     }
 
@@ -287,28 +285,23 @@ impl ServerImpl {
             return Err(FpgaError::CommsError);
         }
 
-        let mut out = [0u8; 2]; // [status, SFF8024Identifier]
+        let mut out = [0u8; 1]; // [SFF8024Identifier]
 
         // Wait for the I2C transaction to complete
-        loop {
-            self.transceivers
-                .get_i2c_status_and_read_buffer(port, &mut out)?;
-            if out[0] & Reg::QSFP::PORT0_STATUS::BUSY == 0 {
-                break;
-            }
-            userlib::hl::sleep_for(1);
-        }
+        let status = self
+            .transceivers
+            .get_i2c_status_and_read_buffer(port, &mut out)?;
 
-        if out[0] & Reg::QSFP::PORT0_STATUS::ERROR == 0 {
-            match out[1] {
+        if status.error == FpgaI2CFailure::NoError {
+            match out[0] {
                 0x1E => Ok(ManagementInterface::Cmis),
                 0x0D | 0x11 => Ok(ManagementInterface::Sff8636),
                 i => Ok(ManagementInterface::Unknown(i)),
             }
         } else {
             // TODO: how should we handle this?
             // Right now, we'll retry on the next pass through the loop.
-            Err(FpgaError::ImplError(out[0]))
+            Err(FpgaError::ImplError(status.error as u8))
         }
     }
 

drv/transceivers-server/src/udp.rs

@@ -19,7 +19,7 @@ use userlib::UnwrapLite;
 use crate::{FrontIOStatus, ServerImpl};
 use drv_sidecar_front_io::transceivers::{
     FpgaI2CFailure, LogicalPort, LogicalPortFailureTypes, LogicalPortMask,
-    ModuleResult, ModuleResultNoFailure, ModuleResultSlim, PortI2CStatus,
+    ModuleResult, ModuleResultNoFailure, ModuleResultSlim,
 };
 use task_net_api::*;
 use transceiver_messages::{
@@ -1156,48 +1156,34 @@ impl ServerImpl {
         let mut failure_types = LogicalPortFailureTypes::default();
 
         for port in result.success().to_indices() {
-            // The status register is contiguous with the output buffer, so
-            // we'll read them all in a single pass.  This should normally
-            // terminate with a single read, since I2C is faster than Hubris
-            // IPC.
-            let mut buf = [0u8; 129];
+            let mut buf = [0u8; 128];
             let port_loc = port.get_physical_location();
-            loop {
-                // If we have not encountered any errors, keep pulling full
-                // status + buffer payloads.
-                if self
-                    .transceivers
-                    .get_i2c_status_and_read_buffer(
-                        port_loc,
-                        &mut buf[0..(buf_len + 1)],
-                    )
-                    .is_err()
-                {
-                    error.set(port);
-                    break;
-                };
-
-                let status = PortI2CStatus::new(buf[0]);
 
-                // Use QSFP::PORT0 for constants, since they're all identical
-                if status.done {
-                    // Check error mask
-                    if status.error != FpgaI2CFailure::NoError {
-                        // Record which port the error ocurred at so we can
-                        // give the host a more meaningful error.
-                        failure.set(port);
-                        failure_types.0[port.0 as usize] = status.error
-                    } else {
-                        // Add data to payload
-                        success.set(port);
-                        let end_idx = idx + buf_len;
-                        out[idx..end_idx].copy_from_slice(&buf[1..][..buf_len]);
-                        idx = end_idx;
-                    }
+            // If we have not encountered any errors, keep pulling full
+            // status + buffer payloads.
+            let status = match self
+                .transceivers
+                .get_i2c_status_and_read_buffer(port_loc, &mut buf[0..buf_len])
+            {
+                Ok(status) => status,
+                Err(_) => {
+                    error.set(port);
                     break;
                 }
+            };
 
-                userlib::hl::sleep_for(1);
+            // Check error mask
+            if status.error != FpgaI2CFailure::NoError {
+                // Record which port the error ocurred at so we can
+                // give the host a more meaningful error.
+                failure.set(port);
+                failure_types.0[port.0 as usize] = status.error
+            } else {
+                // Add data to payload
+                success.set(port);
+                let end_idx = idx + buf_len;
+                out[idx..end_idx].copy_from_slice(&buf[..buf_len]);
+                idx = end_idx;
             }
         }
         let mut final_result =