Fix minor problems in PCI

kernel/comps/pci/src/bus.rs

@@ -10,15 +10,17 @@ use ostd::bus::BusProbeError;
 
 use super::{PciCommonDevice, device_info::PciDeviceId};
 
-/// PciDevice trait.
+/// A trait that represents PCI devices.
 pub trait PciDevice: Sync + Send + Debug {
-    /// Gets device id.
+    /// Returns the device ID.
     fn device_id(&self) -> PciDeviceId;
 }
 
-/// PCI device driver, PCI bus will pass the device through the `probe` function when a new device is registered.
+/// A trait that represents PCI device drivers.
+///
+/// The PCI bus will pass the device through the `probe` function when a new device is registered.
 pub trait PciDriver: Sync + Send + Debug {
-    /// Probe an unclaimed PCI device.
+    /// Probes an unclaimed PCI device.
     ///
     /// If the driver matches and succeeds in initializing the unclaimed device,
     /// then the driver will return an claimed instance of the device,
@@ -33,10 +35,11 @@ pub trait PciDriver: Sync + Send + Debug {
     ) -> Result<Arc<dyn PciDevice>, (BusProbeError, PciCommonDevice)>;
 }
 
-/// The PCI bus used to register PCI devices. If a component wishes to drive a PCI device, it needs to provide the following:
+/// The PCI bus used to register PCI devices.
 ///
-/// 1. The structure that implements the PciDevice trait.
+/// If a component wishes to drive a PCI device, it needs to provide the following:
-/// 2. PCI driver.
+/// 1. The structure that implements the [`PciDevice`] trait.
+/// 2. A [`PciDriver`] instance.
 pub struct PciBus {
     common_devices: VecDeque<PciCommonDevice>,
     devices: Vec<Arc<dyn PciDevice>>,
@@ -46,7 +49,7 @@ pub struct PciBus {
 impl PciBus {
     /// Registers a PCI driver to the PCI bus.
     pub fn register_driver(&mut self, driver: Arc<dyn PciDriver>) {
-        debug!("Register driver:{:#x?}", driver);
+        debug!("Register PCI driver: {:#x?}", driver);
         let length = self.common_devices.len();
         for _ in (0..length).rev() {
             let common_device = self.common_devices.pop_front().unwrap();
@@ -71,7 +74,7 @@ impl PciBus {
     }
 
     pub(super) fn register_common_device(&mut self, mut common_device: PciCommonDevice) {
-        debug!("Find pci common devices:{:x?}", common_device);
+        debug!("Find PCI common device: {:#x?}", common_device);
         let device_id = *common_device.device_id();
         for driver in self.drivers.iter() {
             common_device = match driver.probe(common_device) {

kernel/comps/pci/src/capability/mod.rs

@@ -67,7 +67,7 @@ pub enum CapabilityData {
 }
 
 impl Capability {
-    /// 0xFC, the top of the capability position.
+    /// The top of the capability position.
     const CAPABILITY_TOP: u16 = 0xFC;
 
     /// Gets the capability data
@@ -77,26 +77,29 @@ impl Capability {
 
     /// Gets the capabilities of one device
     pub(super) fn device_capabilities(dev: &mut PciCommonDevice) -> Vec<Self> {
-        if !dev.status().contains(Status::CAPABILITIES_LIST) {
+        if !dev.read_status().contains(Status::CAPABILITIES_LIST) {
             return Vec::new();
         }
 
-        // The offset of the first capability pointer is the same for PCI general devices and PCI bridge devices.
+        // The offset of the first capability pointer is the same for PCI general devices and PCI
+        // bridge devices.
         const CAP_OFFSET: u16 = PciGeneralDeviceCfgOffset::CapabilitiesPointer as u16;
         let mut cap_ptr =
             (dev.location().read8(CAP_OFFSET) as u16).align_down(align_of::<u32>() as _);
         let mut cap_ptr_vec = Vec::new();
         let mut capabilities = Vec::new();
 
-        // read all cap_ptr so that it is easy for us to get the length.
+        // Read all capability pointers so that it is easy for us to get the length of each
+        // capability.
         while cap_ptr > 0 {
             cap_ptr_vec.push(cap_ptr);
             cap_ptr = (dev.location().read8(cap_ptr + 1) as u16).align_down(align_of::<u32>() as _);
         }
         cap_ptr_vec.sort();
 
-        // Push here so that we can calculate the length of the last capability.
+        // Push the top position so that we can calculate the length of the last capability.
         cap_ptr_vec.push(Self::CAPABILITY_TOP);
+
         let length = cap_ptr_vec.len();
         for i in 0..length - 1 {
             let cap_ptr = cap_ptr_vec[i];

kernel/comps/pci/src/capability/msix.rs

@@ -15,12 +15,11 @@ use crate::{
 
 /// MSI-X capability. It will set the BAR space it uses to be hidden.
 #[derive(Debug)]
-#[repr(C)]
 pub struct CapabilityMsixData {
     loc: PciDeviceLocation,
     ptr: u16,
     table_size: u16,
-    /// MSIX table entry content:
+    /// MSI-X table entry content:
     /// | Vector Control: u32 | Msg Data: u32 | Msg Upper Addr: u32 | Msg Addr: u32 |
     table_bar: Arc<MemoryBar>,
     /// Pending bits table.
@@ -85,13 +84,11 @@ impl CapabilityMsixData {
         let table_offset = (table_info & !(0b111u32)) as usize;
 
         let table_size = (dev.location().read16(cap_ptr + 2) & 0b11_1111_1111) + 1;
-        // TODO: Different architecture seems to have different, so we should set different address here.
+
+        // Set the message address and disable all MSI-X vectors.
         let message_address = MSIX_DEFAULT_MSG_ADDR;
         let message_upper_address = 0u32;
-
-        // Set message address 0xFEE0_0000
         for i in 0..table_size {
-            // Set message address and disable this msix entry
             table_bar
                 .io_mem()
                 .write_once((16 * i) as usize + table_offset, &message_address)
@@ -106,11 +103,11 @@ impl CapabilityMsixData {
                 .unwrap();
         }
 
-        // enable MSI-X, bit15: MSI-X Enable
+        // Enable MSI-X (bit 15: MSI-X Enable).
         dev.location()
             .write16(cap_ptr + 2, dev.location().read16(cap_ptr + 2) | 0x8000);
-        // disable INTx, enable Bus master.
+        // Disable INTx. Enable bus master.
-        dev.set_command(dev.command() | Command::INTERRUPT_DISABLE | Command::BUS_MASTER);
+        dev.write_command(dev.read_command() | Command::INTERRUPT_DISABLE | Command::BUS_MASTER);
 
         let mut irqs = Vec::with_capacity(table_size as usize);
         for _ in 0..table_size {
@@ -129,13 +126,15 @@ impl CapabilityMsixData {
         }
     }
 
-    /// MSI-X Table size
+    /// Returns the size of the MSI-X Table.
     pub fn table_size(&self) -> u16 {
         // bit 10:0 table size
         (self.loc.read16(self.ptr + 2) & 0b11_1111_1111) + 1
     }
 
-    /// Enables an interrupt line, it will replace the old handle with the new handle.
+    /// Enables an interrupt line.
+    ///
+    /// If the interrupt line has already been enabled, the old [`IrqLine`] will be replaced.
     pub fn set_interrupt_vector(&mut self, irq: IrqLine, index: u16) {
         if index >= self.table_size {
             return;
@@ -164,19 +163,21 @@ impl CapabilityMsixData {
         }
 
         let _old_irq = self.irqs[index as usize].replace(irq);
-        // Enable this msix vector
+        // Enable this MSI-X vector.
         self.table_bar
             .io_mem()
             .write_once((16 * index + 12) as usize + self.table_offset, &0_u32)
             .unwrap();
     }
 
-    /// Gets mutable IrqLine. User can register callbacks by using this function.
+    /// Returns a mutable reference to the [`IrqLine`].
+    ///
+    /// Users can register callbacks using the returned [`IrqLine`] reference.
     pub fn irq_mut(&mut self, index: usize) -> Option<&mut IrqLine> {
         self.irqs[index].as_mut()
     }
 
-    /// Returns true if MSI-X Enable bit is set.
+    /// Returns true if the MSI-X Enable bit is set.
     pub fn is_enabled(&self) -> bool {
         let msg_ctrl = self.loc.read16(self.ptr + 2);
         msg_ctrl & 0x8000 != 0

kernel/comps/pci/src/capability/vendor.rs

@@ -24,55 +24,57 @@ impl CapabilityVndrData {
         }
     }
 
-    /// The length of this capability
+    /// Returns the length of this capability.
     #[expect(clippy::len_without_is_empty)]
     pub fn len(&self) -> u16 {
         self.length
     }
 
-    /// Reads u8 from the capability.
+    /// Reads a `u8` from the capability.
     pub fn read8(&self, offset: u16) -> Result<u8> {
-        self.check_range(offset)?;
+        self.check_range(offset, size_of::<u8>() as u16)?;
         Ok(self.location.read8(self.cap_ptr + offset))
     }
 
-    /// Writes u8 to the capability.
+    /// Writes a `u8` to the capability.
     pub fn write8(&self, offset: u16, value: u8) -> Result<()> {
-        self.check_range(offset)?;
+        self.check_range(offset, size_of::<u8>() as u16)?;
         self.location.write8(self.cap_ptr + offset, value);
         Ok(())
     }
 
-    /// Reads u16 from the capability.
+    /// Reads a `u16` from the capability.
     pub fn read16(&self, offset: u16) -> Result<u16> {
-        self.check_range(offset)?;
+        self.check_range(offset, size_of::<u16>() as u16)?;
         Ok(self.location.read16(self.cap_ptr + offset))
     }
 
-    /// Writes u16 to the capability.
+    /// Writes a `u16` to the capability.
     pub fn write16(&self, offset: u16, value: u16) -> Result<()> {
-        self.check_range(offset)?;
+        self.check_range(offset, size_of::<u16>() as u16)?;
         self.location.write16(self.cap_ptr + offset, value);
         Ok(())
     }
 
-    /// Reads u32 from the capability.
+    /// Reads a `u32` from the capability.
     pub fn read32(&self, offset: u16) -> Result<u32> {
-        self.check_range(offset)?;
+        self.check_range(offset, size_of::<u32>() as u16)?;
         Ok(self.location.read32(self.cap_ptr + offset))
     }
 
-    /// Writes u32 to the capability.
+    /// Writes a `u32` to the capability.
     pub fn write32(&self, offset: u16, value: u32) -> Result<()> {
-        self.check_range(offset)?;
+        self.check_range(offset, size_of::<u32>() as u16)?;
         self.location.write32(self.cap_ptr + offset, value);
         Ok(())
     }
 
-    fn check_range(&self, offset: u16) -> Result<()> {
+    fn check_range(&self, offset: u16, size: u16) -> Result<()> {
-        if self.length < offset {
+        if let Some(end) = offset.checked_add(size)
-            return Err(Error::InvalidArgs);
+            && end <= self.length
+        {
+            return Ok(());
         }
-        Ok(())
+        Err(Error::InvalidArgs)
     }
 }

kernel/comps/pci/src/cfg_space.rs

@@ -283,7 +283,7 @@ impl Bar {
     }
 }
 
-/// Memory BAR
+/// Memory BAR.
 #[derive(Debug)]
 pub struct MemoryBar {
     base: u64,
@@ -294,23 +294,23 @@ pub struct MemoryBar {
 }
 
 impl MemoryBar {
-    /// Memory BAR bits type
+    /// Returns the BAR's address length (32 bits or 64 bits).
     pub fn address_length(&self) -> AddrLen {
         self.address_length
     }
 
-    /// Whether this bar is prefetchable, allowing the CPU to get the data
+    /// Returns whether the BAR is prefetchable, i.e., whether the CPU is allowed to get the data
     /// in advance.
     pub fn prefetchable(&self) -> bool {
         self.prefetchable
     }
 
-    /// Base address
+    /// Returns the BAR's base address.
     pub fn base(&self) -> u64 {
         self.base
     }
 
-    /// Size of the memory
+    /// Returns the BAR's size.
     pub fn size(&self) -> u64 {
         self.size
     }
@@ -422,7 +422,7 @@ impl MemoryBar {
     }
 }
 
-/// Whether this BAR is 64bit address or 32bit address
+/// The address length of a memory BAR (32 bits or 64 bits).
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum AddrLen {
     /// 32 bits
@@ -439,12 +439,12 @@ pub struct IoBar {
 }
 
 impl IoBar {
-    /// Base port
+    /// Returns the BAR's base port.
     pub fn base(&self) -> u32 {
         self.base
     }
 
-    /// Size of the port
+    /// Returns the BAR's size.
     pub fn size(&self) -> u32 {
         self.size
     }

kernel/comps/pci/src/common_device.rs

@@ -14,7 +14,9 @@ use crate::{
     device_info::PciDeviceLocation,
 };
 
-/// PCI common device, Contains a range of information and functions common to PCI devices.
+/// PCI common device.
+///
+/// This type contains a range of information and functions common to PCI devices.
 #[derive(Debug)]
 pub struct PciCommonDevice {
     device_id: PciDeviceId,
@@ -25,55 +27,55 @@ pub struct PciCommonDevice {
 }
 
 impl PciCommonDevice {
-    /// Gets the PCI device ID
+    /// Returns the PCI device ID.
     pub fn device_id(&self) -> &PciDeviceId {
         &self.device_id
     }
 
-    /// Gets the PCI device location
+    /// Returns the PCI device location.
     pub fn location(&self) -> &PciDeviceLocation {
         &self.location
     }
 
-    /// Gets the PCI Base Address Register (BAR) manager
+    /// Returns the PCI Base Address Register (BAR) manager.
     pub fn bar_manager(&self) -> &BarManager {
         &self.bar_manager
     }
 
-    /// Gets the PCI capabilities
+    /// Returns the PCI capabilities.
     pub fn capabilities(&self) -> &Vec<Capability> {
         &self.capabilities
     }
 
-    /// Gets the PCI device type
+    /// Returns the PCI device type.
     pub fn device_type(&self) -> PciDeviceType {
         self.header_type.device_type()
     }
 
-    /// Checks whether the device is a multi-function device
+    /// Checks whether the device is a multi-function device.
     pub fn has_multi_funcs(&self) -> bool {
         self.header_type.has_multi_funcs()
     }
 
-    /// Gets the PCI Command
+    /// Reads the PCI command.
-    pub fn command(&self) -> Command {
+    pub fn read_command(&self) -> Command {
         Command::from_bits_truncate(self.location.read16(PciCommonCfgOffset::Command as u16))
     }
 
-    /// Sets the PCI Command
+    /// Writes the PCI command.
-    pub fn set_command(&self, command: Command) {
+    pub fn write_command(&self, command: Command) {
         self.location
             .write16(PciCommonCfgOffset::Command as u16, command.bits())
     }
 
-    /// Gets the PCI status
+    /// Reads the PCI status.
-    pub fn status(&self) -> Status {
+    pub fn read_status(&self) -> Status {
         Status::from_bits_truncate(self.location.read16(PciCommonCfgOffset::Status as u16))
     }
 
     pub(super) fn new(location: PciDeviceLocation) -> Option<Self> {
         if location.read16(0) == 0xFFFF {
-            // not exists
+            // No device.
             return None;
         }
 
@@ -101,8 +103,8 @@ impl PciCommonDevice {
             capabilities,
         };
 
-        device.set_command(
+        device.write_command(
-            device.command() | Command::MEMORY_SPACE | Command::BUS_MASTER | Command::IO_SPACE,
+            device.read_command() | Command::MEMORY_SPACE | Command::BUS_MASTER | Command::IO_SPACE,
         );
         device.bar_manager = BarManager::new(device.header_type.device_type(), location);
         device.capabilities = Capability::device_capabilities(&mut device);
@@ -138,18 +140,19 @@ impl PciHeaderType {
         })
     }
 
-    /// Gets the device type.
+    /// Returns the device type.
     pub fn device_type(self) -> PciDeviceType {
         self.device_type
     }
 
-    /// Indicates whether the device has multiple functions.
+    /// Returns whether the device has multiple functions.
     pub fn has_multi_funcs(self) -> bool {
         self.has_multi_funcs
     }
 }
 
 /// Represents the type of PCI device, determined by the device's header type.
+///
 /// Used to distinguish between general devices, PCI-to-PCI bridges, and PCI-to-Cardbus bridges.
 #[derive(Debug, Clone, Copy, Eq, PartialEq)]
 #[repr(u8)]
@@ -175,7 +178,7 @@ impl PciDeviceType {
     }
 }
 
-/// Base Address Registers manager.
+/// Base Address Register (BAR) manager.
 #[derive(Debug)]
 pub struct BarManager {
     /// There are at most 6 BARs in PCI device.

kernel/comps/pci/src/device_info.rs

@@ -4,7 +4,7 @@
 
 use crate::cfg_space::PciCommonCfgOffset;
 
-/// PCI device Location
+/// PCI device location.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
 pub struct PciDeviceLocation {
     /// Bus number