I2C Passthrough Fixes

framework_lib/src/ccgx/device.rs

@@ -10,7 +10,7 @@ use core::prelude::rust_2021::derive;
 use crate::ccgx::{AppVersion, BaseVersion, ControllerVersion};
 use crate::chromium_ec::i2c_passthrough::*;
 use crate::chromium_ec::{CrosEc, EcError, EcResult};
-use crate::util::{assert_win_len, Config, Platform};
+use crate::util::{Config, Platform};
 
 use super::*;
 
@@ -244,6 +244,12 @@ impl PdController {
                     err
                 )));
             }
+            // Don't loop forever if the EC doesn't return any data
+            if i2c_response.data.is_empty() {
+                return Err(EcError::DeviceError(
+                    "I2C read returned no data".to_string(),
+                ));
+            }
             data.extend(i2c_response.data);
         }
 
@@ -281,7 +287,7 @@ impl PdController {
 
     pub fn get_silicon_id(&self) -> EcResult<u16> {
         let data = self.ccgx_read(ControlRegisters::SiliconId, 2)?;
-        assert_win_len(data.len(), 2);
+        assert_eq!(data.len(), 2);
         Ok(u16::from_le_bytes([data[0], data[1]]))
     }
 
@@ -342,7 +348,7 @@ impl PdController {
             }
         };
 
-        assert_win_len(data.len(), 8);
+        assert_eq!(data.len(), 8);
         let base_ver = BaseVersion::from(&data[..4]);
         let app_ver = AppVersion::from(&data[4..]);
         println!(
@@ -352,7 +358,7 @@ impl PdController {
 
         let data = self.ccgx_read(ControlRegisters::Firmware1Version, 8);
         let data = data.unwrap();
-        assert_win_len(data.len(), 8);
+        assert_eq!(data.len(), 8);
         let base_ver = BaseVersion::from(&data[..4]);
         let app_ver = AppVersion::from(&data[4..]);
         println!(
@@ -362,7 +368,7 @@ impl PdController {
 
         let data = self.ccgx_read(ControlRegisters::Firmware2Version, 8);
         let data = data.unwrap();
-        assert_win_len(data.len(), 8);
+        assert_eq!(data.len(), 8);
         let base_ver = BaseVersion::from(&data[..4]);
         let app_ver = AppVersion::from(&data[4..]);
         println!(
@@ -387,7 +393,7 @@ impl PdController {
 
     pub fn get_port_status(&self) -> EcResult<u8> {
         let data = self.ccgx_read(ControlRegisters::PdPortsEnable, 1)?;
-        assert_win_len(data.len(), 1);
+        assert_eq!(data.len(), 1);
         Ok(data[0])
     }
 }

framework_lib/src/chromium_ec/command.rs

@@ -201,14 +201,9 @@ pub trait EcRequestRaw<R> {
         Self: Sized,
     {
         let response = self.send_command_vec_extra(ec, extra_data)?;
-        // TODO: The Windows driver seems to return 20 more bytes than expected
-        #[cfg(windows)]
-        let expected = response.len() != std::mem::size_of::<R>() + 20;
-        #[cfg(not(windows))]
-        let expected = response.len() != std::mem::size_of::<R>();
-        if expected {
+        if response.len() != std::mem::size_of::<R>() {
             return Err(EcError::DeviceError(format!(
-                "Returned data size ({}) is not the expted size: {}",
+                "Returned data size ({}) is not the expected size: {}",
                 response.len(),
                 std::mem::size_of::<R>()
             )));

framework_lib/src/chromium_ec/i2c_passthrough.rs

@@ -63,92 +63,53 @@ impl EcI2cPassthruResponse {
 /// Indicate that it's a read, not a write
 const I2C_READ_FLAG: u16 = 1 << 15;
 
+/// Read from a device with a one byte register address (two bytes if addr > 0xFF)
 pub fn i2c_read(
     ec: &CrosEc,
     i2c_port: u8,
     i2c_addr: u16,
     addr: u16,
     len: u16,
 ) -> EcResult<EcI2cPassthruResponse> {
-    trace!(
-        "i2c_read(i2c_port: 0x{:X}, i2c_addr: 0x{:X}, addr: 0x{:X}, len: 0x{:X})",
-        i2c_port,
-        i2c_addr,
-        addr,
-        len
-    );
-    if usize::from(len) > MAX_I2C_CHUNK {
-        return EcResult::Err(EcError::DeviceError(format!(
-            "i2c_read too long. Must be <128, is: {}",
-            len
-        )));
-    }
-    let addr_bytes = if addr < 0xFF {
+    let addr_bytes = if addr <= 0xFF {
         vec![addr as u8]
     } else {
         u16::to_le_bytes(addr).to_vec()
     };
-    let messages = vec![
-        EcParamsI2cPassthruMsg {
-            addr_and_flags: i2c_addr,
-            transfer_len: addr_bytes.len() as u16,
-        },
-        EcParamsI2cPassthruMsg {
-            addr_and_flags: i2c_addr + I2C_READ_FLAG,
-            transfer_len: len, // How much to read
-        },
-    ];
-    let msgs_len = size_of::<EcParamsI2cPassthruMsg>() * messages.len();
-    let msgs_buffer: &[u8] = unsafe { util::any_vec_as_u8_slice(&messages) };
-
-    let params = EcParamsI2cPassthru {
-        port: i2c_port,
-        messages: messages.len() as u8,
-        msg: [], // Messages are copied right after this struct
-    };
-    let params_len = size_of::<EcParamsI2cPassthru>();
-    let params_buffer: &[u8] = unsafe { util::any_as_u8_slice(&params) };
-
-    let mut buffer: Vec<u8> = vec![0; params_len + msgs_len + addr_bytes.len()];
-    buffer[0..params_len].copy_from_slice(params_buffer);
-    buffer[params_len..params_len + msgs_len].copy_from_slice(msgs_buffer);
-    buffer[params_len + msgs_len..].copy_from_slice(&addr_bytes);
-
-    let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?;
-    let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) };
-    let res_data = &data[size_of::<_EcI2cPassthruResponse>()..];
-    // TODO: Seems to be either one, non-deterministically
-    debug_assert!(res.messages as usize == messages.len() || res.messages == 0);
-    Ok(EcI2cPassthruResponse {
-        i2c_status: res.i2c_status,
-        data: res_data.to_vec(),
-    })
+    i2c_read_impl(ec, i2c_port, i2c_addr, &addr_bytes, len)
 }
 
-/// I2C read with 16-bit addressing (for larger EEPROMs like 24C32+)
-/// Always sends a 2-byte address, even for addr=0
-pub fn i2c_read_16bit_addr(
+/// Read from a device that always uses two byte register addresses (LSB first)
+pub fn i2c_read_addr16(
     ec: &CrosEc,
     i2c_port: u8,
     i2c_addr: u16,
     addr: u16,
     len: u16,
+) -> EcResult<EcI2cPassthruResponse> {
+    i2c_read_impl(ec, i2c_port, i2c_addr, &u16::to_le_bytes(addr), len)
+}
+
+fn i2c_read_impl(
+    ec: &CrosEc,
+    i2c_port: u8,
+    i2c_addr: u16,
+    addr_bytes: &[u8],
+    len: u16,
 ) -> EcResult<EcI2cPassthruResponse> {
     trace!(
-        "i2c_read_16bit_addr(i2c_port: 0x{:X}, i2c_addr: 0x{:X}, addr: 0x{:X}, len: 0x{:X})",
+        "i2c_read(i2c_port: 0x{:X}, i2c_addr: 0x{:X}, addr: {:X?}, len: 0x{:X})",
         i2c_port,
         i2c_addr,
-        addr,
+        addr_bytes,
         len
     );
     if usize::from(len) > MAX_I2C_CHUNK {
         return EcResult::Err(EcError::DeviceError(format!(
-            "i2c_read too long. Must be <128, is: {}",
-            len
+            "i2c_read too long. Must be <={}, is: {}",
+            MAX_I2C_CHUNK, len
         )));
     }
-    // Always use 16-bit addressing (big-endian for EEPROM)
-    let addr_bytes = u16::to_be_bytes(addr).to_vec();
     let messages = vec![
         EcParamsI2cPassthruMsg {
             addr_and_flags: i2c_addr,
@@ -173,25 +134,41 @@ pub fn i2c_read_16bit_addr(
     let mut buffer: Vec<u8> = vec![0; params_len + msgs_len + addr_bytes.len()];
     buffer[0..params_len].copy_from_slice(params_buffer);
     buffer[params_len..params_len + msgs_len].copy_from_slice(msgs_buffer);
-    buffer[params_len + msgs_len..].copy_from_slice(&addr_bytes);
+    buffer[params_len + msgs_len..].copy_from_slice(addr_bytes);
 
     let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?;
     let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) };
-    let header_len: usize = size_of::<_EcI2cPassthruResponse>();
-    /* Note on windows, you can get extra bytes back, so truncate this to the requested size */
-    let res_data = &data[header_len..(len as usize + header_len)];
-    debug_assert!(res.messages as usize == messages.len() || res.messages == 0);
+    // The EC only returns as many bytes as it managed to read.
+    // On NAK or timeout that's less than requested, possibly nothing at all.
+    let res_data = &data[size_of::<_EcI2cPassthruResponse>()..];
+    // The EC stops at the first message that fails, so it can have processed
+    // any number of messages up to the requested count
+    debug_assert!(res.messages as usize <= messages.len());
+    let i2c_status = res.i2c_status;
     trace!(
-        "  i2c_read_16bit_addr response (len: {}, data: {:#04X?})",
+        "  i2c_read response (status: 0x{:02X}, len: {}, data: {:#04X?})",
+        i2c_status,
         res_data.len(),
-        res_data.to_vec()
+        res_data
     );
     Ok(EcI2cPassthruResponse {
-        i2c_status: res.i2c_status,
+        i2c_status,
         data: res_data.to_vec(),
     })
 }
 
+/// I2C read with 16-bit addressing (for larger EEPROMs like 24C32+)
+/// Always sends a 2-byte address (MSB first), even for addr=0
+pub fn i2c_read_16bit_addr(
+    ec: &CrosEc,
+    i2c_port: u8,
+    i2c_addr: u16,
+    addr: u16,
+    len: u16,
+) -> EcResult<EcI2cPassthruResponse> {
+    i2c_read_impl(ec, i2c_port, i2c_addr, &u16::to_be_bytes(addr), len)
+}
+
 /* Write address and bytes in a single I2C transfer */
 pub fn i2c_write_block(
     ec: &CrosEc,
@@ -231,7 +208,7 @@ pub fn i2c_write_block(
 
     let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?;
     let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) };
-    util::assert_win_len(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header
+    assert_eq!(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header
 
     // Only assert message count if the transfer was successful
     // On error (NAK/timeout), the EC may return 0 messages processed
@@ -283,7 +260,7 @@ pub fn i2c_write(
 
     let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?;
     let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) };
-    util::assert_win_len(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header
+    assert_eq!(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header
 
     // Only assert message count if the transfer was successful
     // On error (NAK/timeout), the EC may return 0 messages processed

framework_lib/src/chromium_ec/mod.rs

@@ -451,7 +451,7 @@ impl CrosEc {
         let limits = &[ChargeLimitControlModes::Set as u8, max, min];
         let data = self.send_command(EcCommands::ChargeLimitControl as u16, 0, limits)?;
 
-        util::assert_win_len(data.len(), 0);
+        assert_eq!(data.len(), 0);
 
         Ok(())
     }
@@ -515,7 +515,7 @@ impl CrosEc {
         let limits = &[percentage, 0x00];
         let data = self.send_command(EcCommands::FpLedLevelControl as u16, 1, limits)?;
 
-        util::assert_win_len(data.len(), 0);
+        assert_eq!(data.len(), 0);
 
         Ok(())
     }
@@ -526,7 +526,7 @@ impl CrosEc {
         let limits = &[level as u8, 0x00];
         let data = self.send_command(EcCommands::FpLedLevelControl as u16, 0, limits)?;
 
-        util::assert_win_len(data.len(), 0);
+        assert_eq!(data.len(), 0);
 
         Ok(())
     }
@@ -1460,6 +1460,12 @@ impl CrosEc {
                     err
                 )));
             }
+            // Don't loop forever if the EC doesn't return any data
+            if i2c_response.data.is_empty() {
+                return Err(EcError::DeviceError(
+                    "I2C read returned no data".to_string(),
+                ));
+            }
             data.extend(i2c_response.data);
         }
 

framework_lib/src/chromium_ec/windows.rs

@@ -169,10 +169,15 @@ pub fn send_command(command: u16, command_version: u8, data: &[u8]) -> EcResult<
         Some(status) => return Err(EcError::Response(status)),
     }
 
-    // TODO: Figure out why that's sometimes bigger
-    let end = std::cmp::min(returned, CROSEC_CMD_MAX_REQUEST as u32);
-
-    let out_buffer = &cmd.buffer[..(end as usize)];
+    // The driver reports how many bytes of CROSEC_COMMAND it filled in, which includes its
+    // 20 byte header. But cmd.buffer already starts after that header, so subtract it to get
+    // the length of the EC response itself.
+    // Otherwise we'd return 20 bytes of leftover request data after the response.
+    let crosec_header_len = std::mem::offset_of!(CrosEcCommand, buffer);
+    let data_len = (returned as usize).saturating_sub(crosec_header_len);
+    let end = std::cmp::min(data_len, CROSEC_CMD_MAX_REQUEST);
+
+    let out_buffer = &cmd.buffer[..end];
     Ok(out_buffer.to_vec())
 }
 

framework_lib/src/parade_retimer.rs

@@ -1,10 +1,11 @@
+use alloc::string::ToString;
 use alloc::vec::Vec;
 use num_traits::FromPrimitive;
 
 use crate::chromium_ec::command::EcRequestRaw;
 use crate::chromium_ec::commands::{EcRequestGetGpuPcie, GpuVendor};
 use crate::chromium_ec::i2c_passthrough::*;
-use crate::chromium_ec::{CrosEc, EcResult};
+use crate::chromium_ec::{CrosEc, EcError, EcResult};
 use crate::os_specific;
 
 pub fn get_version(ec: &CrosEc) -> EcResult<Option<Vec<u8>>> {
@@ -35,13 +36,16 @@ pub fn get_version(ec: &CrosEc) -> EcResult<Option<Vec<u8>>> {
         i2c_response.i2c_status,
         i2c_response.data.len()
     );
-    if i2c_response.i2c_status == 0x01 {
-        // NAK
+    // We know the retimer is there, because the EC reported a compatible dGPU.
+    // So if it doesn't answer, report that instead of pretending it doesn't exist.
+    if let Err(err) = i2c_response.is_successful() {
         warn!("Unable to communicate with dGPU Retimer. Try to force it on by plugging a cable into the dGPU");
+        return Err(err);
     }
     let Some(&safe_mode) = i2c_response.data.first() else {
-        info!("Failed to read retimer safe mode status (empty response)");
-        return Ok(None);
+        return Err(EcError::DeviceError(
+            "Retimer did not return the safe mode status".to_string(),
+        ));
     };
     info!("Retimer safe mode status: {}", safe_mode);
     if safe_mode == 0 {
@@ -61,6 +65,7 @@ pub fn get_version(ec: &CrosEc) -> EcResult<Option<Vec<u8>>> {
 
     // Read version
     let i2c_response = i2c_read(ec, i2c_port, (i2c_addr + 18) >> 1, 0x01, 0x04)?;
+    i2c_response.is_successful()?;
 
     Ok(Some(i2c_response.data))
 }

framework_lib/src/util.rs

@@ -1,6 +1,5 @@
 //! Miscellaneous utility functions to use across modules
 
-use num::{Num, NumCast};
 use std::prelude::v1::*;
 
 #[cfg(feature = "uefi")]
@@ -267,15 +266,6 @@ pub fn find_sequence(haystack: &[u8], needle: &[u8]) -> Option<usize> {
         .position(|window| window == needle)
 }
 
-/// Assert length of an EC response from the windows driver
-/// It's always 20 more than expected. TODO: Figure out why
-pub fn assert_win_len<N: Num + std::fmt::Debug + Ord + NumCast + Copy>(left: N, right: N) {
-    #[cfg(windows)]
-    assert_eq!(left, right + NumCast::from(20).unwrap());
-    #[cfg(not(windows))]
-    assert_eq!(left, right);
-}
-
 pub fn format_duration(duration: &Duration) -> String {
     let hours = duration.as_secs() / 3600;
     let minutes = (duration.as_secs() % 3600) / 60;