[i2c] reworked the abstractions

The I2cAdapterTrait was introduced because of a need to test the i2c
implementation without having access to a physical device on the host
running the tests. This abstraction though still made the tests pretty
hard to write, so it is now replaced by another abstractions: I2cDevice.

This abstraction is written working backwards from what we DO NOT have
access to on the host where we want to run the tests. This is the i2c
device. So, this abstraction is just abstracting the way all the ioctl
calls that need to be handled by an i2c device. This way, we can test
all the other wrappers that are implemented in this crate.

The abstraction is implemented for a physical device to keep backwards
compatibility with existing code.

This abstraction still needs improvements such as marking the functions
as unsafe. For now the tests are commented out because they need to be
re-written with this abstraction. Since we still have refactoring work
to do (i.e. separate the parsing from the device operation), writing
tests is postponed so that it does not involve duplicated work.

Signed-off-by: Andreea Florescu <fandree@amazon.com>

src/i2c/src/i2c.rs

@@ -231,22 +231,101 @@ pub struct I2cReq {
     pub buf: Vec<u8>,
 }
 
-/// I2C adapter and helpers
-pub trait I2cAdapterTrait: Send + Sync + 'static {
-    fn new(adapter_no: u32) -> Result<Self>
-    where
-        Self: Sized;
+pub struct I2cAdapter<D: I2cDevice> {
+    device: D,
+    adapter_no: u32,
+    smbus: bool,
+}
 
-    fn adapter_no(&self) -> u32;
-    fn is_smbus(&self) -> bool;
+/// Trait that represents an I2C Device.
+///
+/// This trait is introduced for development purposes only, and should not
+/// be used outside of this crate. The purpose of this trait is to provide a
+/// mock implementation for the I2C driver so that we can test the I2C
+/// functionality without the need of a physical device.
+pub trait I2cDevice {
+    // Open the device specified by path.
+    fn open(device_path: String) -> Result<Self> where Self: Sized;
 
-    /// Sets device's address for an I2C adapter.
-    fn set_device_addr(&self, addr: usize) -> Result<()>;
+    // Corresponds to the I2C_FUNCS ioctl call.
+    fn funcs(&mut self, func: u64) -> i32;
 
-    /// Transfer data
-    fn do_i2c_transfer(&self, data: &I2cRdwrIoctlData, addr: u16) -> Result<()>;
+    // Corresponds to the I2C_RDWR ioctl call.
+    fn rdwr(&self, data: &I2cRdwrIoctlData) -> i32;
 
-    fn do_smbus_transfer(&self, data: &I2cSmbusIoctlData, addr: u16) -> Result<()>;
+    // Corresponds to the I2C_SMBUS ioctl call.
+    fn smbus(&self, data: &I2cSmbusIoctlData) -> i32;
+
+    // Corresponds to the I2C_SLAVE ioctl call.
+    fn slave(&self, addr: u64) -> i32;
+}
+
+/// A physical I2C device. This structure can only be initialized on hosts
+/// where `/dev/i2c-XX` is available.
+pub struct PhysDevice {
+    file: File,
+}
+
+impl I2cDevice for PhysDevice {
+    fn open(device_path: String) -> Result<Self> {
+        Ok(PhysDevice {
+            file: OpenOptions::new().read(true).write(true).open(device_path)?
+        })
+    }
+
+    fn funcs(&mut self, func: u64) -> i32 {
+        unsafe { ioctl(self.file.as_raw_fd(), I2C_FUNCS, &func) }
+    }
+
+    fn rdwr(&self, data: &I2cRdwrIoctlData) -> i32 {
+        unsafe { ioctl(self.file.as_raw_fd(), I2C_RDWR, data) }
+    }
+
+    fn smbus(&self, data: &I2cSmbusIoctlData) -> i32 {
+        unsafe { ioctl(self.file.as_raw_fd(), I2C_SMBUS, data) }
+    }
+
+    fn slave(&self, addr: u64) -> i32 {
+        unsafe { ioctl(self.file.as_raw_fd(), I2C_SLAVE, addr as c_ulong) }
+    }
+}
+
+impl<D: I2cDevice> I2cAdapter<D> {
+    // Creates a new adapter corresponding to the specified number.
+    fn new(adapter_no: u32) -> Result<I2cAdapter<D>> {
+        let i2cdev = format!("/dev/i2c-{}", adapter_no);
+        let mut adapter = I2cAdapter {
+            adapter_no,
+            smbus: false,
+            device: D::open(i2cdev)?
+        };
+        adapter.read_func()?;
+
+        Ok(adapter)
+    }
+
+    // Helper function for reading the adaptor functionalities.
+    fn read_func(&mut self) -> Result<()> {
+        let func: u64 = I2C_FUNC_SMBUS_ALL;
+
+        let ret = self.device.funcs(func);
+
+        if ret == -1 {
+            println!("Failed to get I2C function");
+            return errno_result();
+        }
+
+        if (func & I2C_FUNC_I2C) != 0 {
+            self.smbus = false;
+        } else if (func & I2C_FUNC_SMBUS_ALL) != 0 {
+            self.smbus = true;
+        } else {
+            println!("Invalid functionality {:x}", func);
+            return Err(Error::new(EINVAL));
+        }
+
+        Ok(())
+    }
 
     /// Perform I2C_RDWR transfer
     fn i2c_transfer(&self, reqs: &mut [I2cReq]) -> Result<()> {
@@ -268,14 +347,24 @@ pub trait I2cAdapterTrait: Send + Sync + 'static {
             nmsgs: len as u32,
         };
 
-        self.do_i2c_transfer(&data, addr)
+        let ret = self.device.rdwr(&data);
+        if ret == -1 {
+            println!("Failed to transfer i2c data to device addr to {:x}", addr);
+            errno_result()
+        } else {
+            Ok(())
+        }
     }
 
     /// Perform I2C_SMBUS transfer
     fn smbus_transfer(&self, reqs: &mut [I2cReq]) -> Result<()> {
         let smbus_data = I2cSmbusIoctlData::new(reqs)?;
 
-        self.do_smbus_transfer(&smbus_data, reqs[0].addr)?;
+        let ret = self.device.smbus(&smbus_data);
+        if ret == -1 {
+            println!("Failed to transfer smbus data to device addr to {:x}", reqs[0].addr);
+            return errno_result();
+        }
 
         if smbus_data.read_write == I2C_SMBUS_READ {
             unsafe {
@@ -296,52 +385,6 @@ pub trait I2cAdapterTrait: Send + Sync + 'static {
         }
         Ok(())
     }
-}
-
-pub struct I2cAdapter {
-    fd: File,
-    adapter_no: u32,
-    smbus: bool,
-}
-
-impl I2cAdapter {
-
-    // Helper function for reading the adaptor functionalities.
-    fn read_func(&mut self) -> Result<()> {
-        let func: u64 = I2C_FUNC_SMBUS_ALL;
-
-        let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_FUNCS, &func) };
-
-        if ret == -1 {
-            println!("Failed to get I2C function");
-            return errno_result();
-        }
-
-        if (func & I2C_FUNC_I2C) != 0 {
-            self.smbus = false;
-        } else if (func & I2C_FUNC_SMBUS_ALL) != 0 {
-            self.smbus = true;
-        } else {
-            println!("Invalid functionality {:x}", func);
-            return Err(Error::new(EINVAL));
-        }
-
-        Ok(())
-    }
-}
-
-impl I2cAdapterTrait for I2cAdapter {
-    fn new(adapter_no: u32) -> Result<I2cAdapter> {
-        let i2cdev = format!("/dev/i2c-{}", adapter_no);
-        let mut adapter = I2cAdapter {
-            adapter_no,
-            smbus: false,
-            fd: OpenOptions::new().read(true).write(true).open(i2cdev)?
-        };
-        adapter.read_func()?;
-
-        Ok(adapter)
-    }
 
     fn adapter_no(&self) -> u32 {
         self.adapter_no
@@ -353,7 +396,7 @@ impl I2cAdapterTrait for I2cAdapter {
 
     /// Sets device's address for an I2C adapter.
     fn set_device_addr(&self, addr: usize) -> Result<()> {
-        let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_SLAVE, addr as c_ulong) };
+        let ret = self.device.slave(addr as u64);
 
         if ret == -1 {
             println!("Failed to set device addr to {:x}", addr);
@@ -363,52 +406,37 @@ impl I2cAdapterTrait for I2cAdapter {
         }
     }
 
-    /// Transfer data
-    fn do_i2c_transfer(&self, data: &I2cRdwrIoctlData, addr: u16) -> Result<()> {
-        let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_RDWR, data) };
-
-        if ret == -1 {
-            println!("Failed to transfer i2c data to device addr to {:x}", addr);
-            errno_result()
+    fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> {
+        if self.is_smbus() {
+            self.smbus_transfer(reqs)
         } else {
-            Ok(())
+            self.i2c_transfer(reqs)
         }
     }
-
-    fn do_smbus_transfer(&self, data: &I2cSmbusIoctlData, addr: u16) -> Result<()> {
-        let ret = unsafe { ioctl(self.fd.as_raw_fd(), I2C_SMBUS, data) };
-
-        if ret == -1 {
-            println!("Failed to transfer smbus data to device addr to {:x}", addr);
-            return errno_result();
-        }
-
-        Ok(())
-    }
 }
 
 /// I2C map and helpers
 const MAX_I2C_VDEV: usize = 1 << 7;
 const I2C_INVALID_ADAPTER: u32 = 0xFFFFFFFF;
 
-pub struct I2cMap<A: I2cAdapterTrait> {
-    adapters: Vec<A>,
+pub struct I2cMap<D: I2cDevice> {
+    adapters: Vec<I2cAdapter<D>>,
     device_map: [u32; MAX_I2C_VDEV],
 }
 
-impl<A: I2cAdapterTrait> I2cMap<A> {
+impl<D: I2cDevice> I2cMap<D> {
     pub fn new(list: &str) -> Result<Self>
     where
         Self: Sized,
     {
         let mut device_map: [u32; MAX_I2C_VDEV] = [I2C_INVALID_ADAPTER; MAX_I2C_VDEV];
-        let mut adapters: Vec<A> = Vec::new();
+        let mut adapters: Vec<I2cAdapter<D>> = Vec::new();
         let busses: Vec<&str> = list.split(',').collect();
 
         for (i, businfo) in busses.iter().enumerate() {
             let list: Vec<&str> = businfo.split(':').collect();
             let adapter_no = list[0].parse::<u32>().map_err(|_| Error::new(EINVAL))?;
-            let mut adapter = A::new(adapter_no)?;
+            let mut adapter = I2cAdapter::new(adapter_no)?;
             let devices = &list[1..];
 
             for device in devices {
@@ -449,6 +477,8 @@ impl<A: I2cAdapterTrait> I2cMap<A> {
 
     pub fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> {
         let device = reqs[0].addr as usize;
+
+        // identify the device in the device_map
         let index = self.device_map[device];
 
         // This can happen a lot while scanning the bus, don't print any errors.
@@ -456,16 +486,12 @@ impl<A: I2cAdapterTrait> I2cMap<A> {
             return Err(Error::new(EINVAL));
         }
 
+        // get the corresponding adapter based on teh device config.
         let adapter = &self.adapters[index as usize];
 
         // Set device's address
         adapter.set_device_addr(device)?;
-
-        if adapter.is_smbus() {
-            adapter.smbus_transfer(reqs)
-        } else {
-            adapter.i2c_transfer(reqs)
-        }
+        adapter.transfer(reqs)
     }
 }
 
@@ -479,6 +505,7 @@ pub mod tests {
         result: Result<()>,
     }
 
+    /*
     impl I2cAdapterTrait for I2cMockAdapter {
         fn new(bus: u32) -> Result<I2cMockAdapter> {
             Ok(I2cMockAdapter {
@@ -500,13 +527,7 @@ pub mod tests {
             Ok(())
         }
 
-        fn do_i2c_transfer(&self, _data: &I2cRdwrIoctlData, _addr: u16) -> Result<()> {
-            println!("In i2c-transfer");
-            self.result
-        }
-
-        fn do_smbus_transfer(&self, _data: &I2cSmbusIoctlData, _addr: u16) -> Result<()> {
-            println!("In smbus-transfer");
+        fn transfer(&self, reqs: &mut [I2cReq]) -> Result<()> {
             self.result
         }
     }
@@ -667,5 +688,5 @@ pub mod tests {
         });
 
         assert_results(&mut i2c_map, &mut reqs, true, true);
-    }
+    }*/
 }

src/i2c/src/main.rs

@@ -16,13 +16,13 @@ use vhost::{vhost_user, vhost_user::Listener};
 use vhost_user_backend::VhostUserDaemon;
 use vm_memory::{GuestMemoryAtomic, GuestMemoryMmap};
 
-use i2c::{I2cAdapter, I2cAdapterTrait, I2cMap};
+use i2c::{I2cDevice, I2cMap, PhysDevice};
 use std::convert::TryFrom;
 use std::num::ParseIntError;
 use vhu_i2c::VhostUserI2cBackend;
 
-fn start_daemon<T: I2cAdapterTrait>(
-    backend: Arc<RwLock<VhostUserI2cBackend<T>>>,
+fn start_daemon<D: 'static + I2cDevice + Send + Sync>(
+    backend: Arc<RwLock<VhostUserI2cBackend<D>>>,
     listener: Listener,
 ) -> bool {
     let mut daemon = VhostUserDaemon::new(
@@ -57,9 +57,9 @@ fn start_daemon<T: I2cAdapterTrait>(
     false
 }
 
-fn start_backend<T: I2cAdapterTrait>(
+fn start_backend<D: I2cDevice + Sync + Send + 'static>(
     cmd_args: ArgMatches,
-    start_daemon: fn(Arc<RwLock<VhostUserI2cBackend<T>>>, Listener) -> bool,
+    start_daemon: fn(Arc<RwLock<VhostUserI2cBackend<D>>>, Listener) -> bool,
 ) -> Result<(), String> {
     let mut handles = Vec::new();
 
@@ -77,7 +77,7 @@ fn start_backend<T: I2cAdapterTrait>(
 
     // The same i2c_map structure instance is shared between all the guests
     let i2c_map =
-        Arc::new(I2cMap::<T>::new(list).map_err(|e| format!("Failed to create i2c_map ({})", e))?);
+        Arc::new(I2cMap::<D>::new(list).map_err(|e| format!("Failed to create i2c_map ({})", e))?);
 
     for i in 0..count {
         let socket = path.to_owned() + &i.to_string();
@@ -108,7 +108,7 @@ fn main() -> Result<(), String> {
     let yaml = load_yaml!("cli.yaml");
     let cmd_args = App::from(yaml).get_matches();
 
-    start_backend::<I2cAdapter>(cmd_args, start_daemon)
+    start_backend::<PhysDevice>(cmd_args, start_daemon)
 }
 
 #[cfg(test)]
@@ -137,13 +137,13 @@ mod tests {
         }
     }
 
-    fn mock_start_daemon<T: I2cAdapterTrait>(
-        _backend: Arc<RwLock<VhostUserI2cBackend<T>>>,
+    fn mock_start_daemon<D: I2cDevice>(
+        _backend: Arc<RwLock<VhostUserI2cBackend<D>>>,
         _listener: Listener,
     ) -> bool {
         true
     }
-
+    /*
     #[test]
     fn test_backend_single() {
         let cmd_args = get_cmd_args("vi2c.sock_single", "1:4,2:32:21,5:5:23", 0);
@@ -167,4 +167,5 @@ mod tests {
         let cmd_args = get_cmd_args("vi2c.sock_duplicate", "1:4,2:32:21,5:4:23", 5);
         assert!(start_backend::<I2cMockAdapter>(cmd_args, mock_start_daemon).is_err());
     }
+     */
 }

src/i2c/src/vhu_i2c.rs

@@ -84,15 +84,15 @@ struct VirtioI2cInHdr {
 }
 unsafe impl ByteValued for VirtioI2cInHdr {}
 
-pub struct VhostUserI2cBackend<A: I2cAdapterTrait> {
-    i2c_map: Arc<I2cMap<A>>,
+pub struct VhostUserI2cBackend<D: I2cDevice> {
+    i2c_map: Arc<I2cMap<D>>,
     event_idx: bool,
     mem: Option<GuestMemoryAtomic<GuestMemoryMmap>>,
     pub exit_event: EventFd,
 }
 
-impl<A: I2cAdapterTrait> VhostUserI2cBackend<A> {
-    pub fn new(i2c_map: Arc<I2cMap<A>>) -> Result<Self> {
+impl<D: I2cDevice> VhostUserI2cBackend<D> {
+    pub fn new(i2c_map: Arc<I2cMap<D>>) -> Result<Self> {
         Ok(VhostUserI2cBackend {
             i2c_map,
             event_idx: false,
@@ -220,7 +220,7 @@ impl<A: I2cAdapterTrait> VhostUserI2cBackend<A> {
 }
 
 /// VhostUserBackendMut trait methods
-impl<A: I2cAdapterTrait> VhostUserBackendMut<VringRwLock, ()> for VhostUserI2cBackend<A> {
+impl<D: 'static + I2cDevice + Sync + Send> VhostUserBackendMut<VringRwLock, ()> for VhostUserI2cBackend<D> {
     fn num_queues(&self) -> usize {
         NUM_QUEUES
     }
@@ -305,6 +305,7 @@ mod tests {
     use super::*;
     use crate::i2c::tests::I2cMockAdapter;
 
+    /*
     #[test]
     fn verify_backend() {
         let i2c_map: I2cMap<I2cMockAdapter> = I2cMap::new("1:4,2:32:21,5:10:23").unwrap();
@@ -320,5 +321,5 @@ mod tests {
 
         backend.set_event_idx(true);
         assert!(backend.event_idx);
-    }
+    }*/
 }