Introduce a new trait `RawDeviceIdIndex`, which extends `RawDeviceId` to provide support for device ID types that include an index or context field (e.g., `driver_data`). This separates the concerns of layout compatibility and index-based data embedding, and allows `RawDeviceId` to be implemented for types that do not contain a `driver_data` field. Several such structures are defined in include/linux/mod_devicetable.h. Refactor `IdArray::new()` into a generic `build()` function, which takes an optional offset. Based on the presence of `RawDeviceIdIndex`, index writing is conditionally enabled. A new `new_without_index()` constructor is also provided for use cases where no index should be written. This refactoring is a preparation for enabling the PHY abstractions to use the RawDeviceId trait. The changes to acpi.rs and driver.rs were made by Danilo. Acked-by: Danilo Krummrich Reviewed-by: Trevor Gross Signed-off-by: FUJITA Tomonori --- rust/kernel/acpi.rs | 15 ++++--- rust/kernel/auxiliary.rs | 11 ++--- rust/kernel/device_id.rs | 91 +++++++++++++++++++++++++++++----------- rust/kernel/driver.rs | 8 +++- rust/kernel/of.rs | 15 ++++--- rust/kernel/pci.rs | 11 ++--- 6 files changed, 104 insertions(+), 47 deletions(-) diff --git a/rust/kernel/acpi.rs b/rust/kernel/acpi.rs index 2af4d4f92924..7ae317368b00 100644 --- a/rust/kernel/acpi.rs +++ b/rust/kernel/acpi.rs @@ -2,7 +2,11 @@ //! Advanced Configuration and Power Interface abstractions. -use crate::{bindings, device_id::RawDeviceId, prelude::*}; +use crate::{ + bindings, + device_id::{RawDeviceId, RawDeviceIdIndex}, + prelude::*, +}; /// IdTable type for ACPI drivers. pub type IdTable = &'static dyn kernel::device_id::IdTable; @@ -12,13 +16,14 @@ #[derive(Clone, Copy)] pub struct DeviceId(bindings::acpi_device_id); -// SAFETY: -// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct acpi_device_id` and does not add -// additional invariants, so it's safe to transmute to `RawType`. -// * `DRIVER_DATA_OFFSET` is the offset to the `data` field. +// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `acpi_device_id` and does not add +// additional invariants, so it's safe to transmute to `RawType`. unsafe impl RawDeviceId for DeviceId { type RawType = bindings::acpi_device_id; +} +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field. +unsafe impl RawDeviceIdIndex for DeviceId { const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::acpi_device_id, driver_data); fn index(&self) -> usize { diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs index 2985673181b7..12a4d36dc452 100644 --- a/rust/kernel/auxiliary.rs +++ b/rust/kernel/auxiliary.rs @@ -6,7 +6,7 @@ use crate::{ bindings, container_of, device, - device_id::RawDeviceId, + device_id::{RawDeviceId, RawDeviceIdIndex}, driver, error::{from_result, to_result, Result}, prelude::*, @@ -134,13 +134,14 @@ pub const fn new(modname: &'static CStr, name: &'static CStr) -> Self { } } -// SAFETY: -// * `DeviceId` is a `#[repr(transparent)`] wrapper of `auxiliary_device_id` and does not add -// additional invariants, so it's safe to transmute to `RawType`. -// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field. +// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `auxiliary_device_id` and does not add +// additional invariants, so it's safe to transmute to `RawType`. unsafe impl RawDeviceId for DeviceId { type RawType = bindings::auxiliary_device_id; +} +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field. +unsafe impl RawDeviceIdIndex for DeviceId { const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::auxiliary_device_id, driver_data); diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 0a4eb56d98f2..8ed2c946144c 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -14,32 +14,41 @@ /// /// # Safety /// -/// Implementers must ensure that: -/// - `Self` is layout-compatible with [`RawDeviceId::RawType`]; i.e. it's safe to transmute to -/// `RawDeviceId`. +/// Implementers must ensure that `Self` is layout-compatible with [`RawDeviceId::RawType`]; +/// i.e. it's safe to transmute to `RawDeviceId`. /// -/// This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building -/// the ID table. +/// This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building +/// the ID table. /// -/// Ideally, this should be achieved using a const function that does conversion instead of -/// transmute; however, const trait functions relies on `const_trait_impl` unstable feature, -/// which is broken/gone in Rust 1.73. -/// -/// - `DRIVER_DATA_OFFSET` is the offset of context/data field of the device ID (usually named -/// `driver_data`) of the device ID, the field is suitable sized to write a `usize` value. -/// -/// Similar to the previous requirement, the data should ideally be added during `Self` to -/// `RawType` conversion, but there's currently no way to do it when using traits in const. +/// Ideally, this should be achieved using a const function that does conversion instead of +/// transmute; however, const trait functions relies on `const_trait_impl` unstable feature, +/// which is broken/gone in Rust 1.73. pub unsafe trait RawDeviceId { /// The raw type that holds the device id. /// /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array. type RawType: Copy; +} - /// The offset to the context/data field. +/// Extension trait for [`RawDeviceId`] for devices that embed an index or context value. +/// +/// This is typically used when the device ID struct includes a field like `driver_data` +/// that is used to store a pointer-sized value (e.g., an index or context pointer). +/// +/// # Safety +/// +/// Implementers must ensure that `DRIVER_DATA_OFFSET` is the correct offset (in bytes) to +/// the context/data field (e.g., the `driver_data` field) within the raw device ID structure. +/// This field must be correctly sized to hold a `usize`. +/// +/// Ideally, the data should be added during `Self` to `RawType` conversion, +/// but there's currently no way to do it when using traits in const. +pub unsafe trait RawDeviceIdIndex: RawDeviceId { + /// The offset (in bytes) to the context/data field in the raw device ID. const DRIVER_DATA_OFFSET: usize; - /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceId`] trait. + /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceIdIndex`] + /// trait. fn index(&self) -> usize; } @@ -68,7 +77,15 @@ impl IdArray { /// Creates a new instance of the array. /// /// The contents are derived from the given identifiers and context information. - pub const fn new(ids: [(T, U); N]) -> Self { + /// + /// # Safety + /// + /// `data_offset` as `None` is always safe. + /// If `data_offset` is `Some(data_offset)`, then: + /// - `data_offset` must be the correct offset (in bytes) to the context/data field + /// (e.g., the `driver_data` field) within the raw device ID structure. + /// - The field at `data_offset` must be correctly sized to hold a `usize`. + const unsafe fn build(ids: [(T, U); N], data_offset: Option) -> Self { let mut raw_ids = [const { MaybeUninit::::uninit() }; N]; let mut infos = [const { MaybeUninit::uninit() }; N]; @@ -77,14 +94,16 @@ impl IdArray { // SAFETY: by the safety requirement of `RawDeviceId`, we're guaranteed that `T` is // layout-wise compatible with `RawType`. raw_ids[i] = unsafe { core::mem::transmute_copy(&ids[i].0) }; - // SAFETY: by the safety requirement of `RawDeviceId`, this would be effectively - // `raw_ids[i].driver_data = i;`. - unsafe { - raw_ids[i] - .as_mut_ptr() - .byte_offset(T::DRIVER_DATA_OFFSET as _) - .cast::() - .write(i); + if let Some(data_offset) = data_offset { + // SAFETY: by the safety requirement of this function, this would be effectively + // `raw_ids[i].driver_data = i;`. + unsafe { + raw_ids[i] + .as_mut_ptr() + .byte_offset(data_offset as _) + .cast::() + .write(i); + } } // SAFETY: this is effectively a move: `infos[i] = ids[i].1`. We make a copy here but @@ -109,12 +128,34 @@ impl IdArray { } } + /// Creates a new instance of the array without writing index values. + /// + /// The contents are derived from the given identifiers and context information. + /// If the device implements [`RawDeviceIdIndex`], consider using [`IdArray::new`] instead. + pub const fn new_without_index(ids: [(T, U); N]) -> Self { + // SAFETY: Calling `Self::build` with `offset = None` is always safe, + // because no raw memory writes are performed in this case. + unsafe { Self::build(ids, None) } + } + /// Reference to the contained [`RawIdArray`]. pub const fn raw_ids(&self) -> &RawIdArray { &self.raw_ids } } +impl IdArray { + /// Creates a new instance of the array. + /// + /// The contents are derived from the given identifiers and context information. + pub const fn new(ids: [(T, U); N]) -> Self { + // SAFETY: by the safety requirement of `RawDeviceIdIndex`, + // `T::DRIVER_DATA_OFFSET` is guaranteed to be the correct offset (in bytes) to + // a field within `T::RawType`. + unsafe { Self::build(ids, Some(T::DRIVER_DATA_OFFSET)) } + } +} + /// A device id table. /// /// This trait is only implemented by `IdArray`. diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs index f8dd7593e8dc..a8f2675ba7a7 100644 --- a/rust/kernel/driver.rs +++ b/rust/kernel/driver.rs @@ -170,7 +170,7 @@ fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> { // and does not add additional invariants, so it's safe to transmute. let id = unsafe { &*raw_id.cast::() }; - Some(table.info(::index(id))) + Some(table.info(::index(id))) } } } @@ -204,7 +204,11 @@ fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> { // and does not add additional invariants, so it's safe to transmute. let id = unsafe { &*raw_id.cast::() }; - Some(table.info(::index(id))) + Some( + table.info(::index( + id, + )), + ) } } } diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs index 52fe0c77b9a1..0888469bddb7 100644 --- a/rust/kernel/of.rs +++ b/rust/kernel/of.rs @@ -2,7 +2,11 @@ //! Device Tree / Open Firmware abstractions. -use crate::{bindings, device_id::RawDeviceId, prelude::*}; +use crate::{ + bindings, + device_id::{RawDeviceId, RawDeviceIdIndex}, + prelude::*, +}; /// IdTable type for OF drivers. pub type IdTable = &'static dyn kernel::device_id::IdTable; @@ -12,13 +16,14 @@ #[derive(Clone, Copy)] pub struct DeviceId(bindings::of_device_id); -// SAFETY: -// * `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id` and does not add -// additional invariants, so it's safe to transmute to `RawType`. -// * `DRIVER_DATA_OFFSET` is the offset to the `data` field. +// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id` and +// does not add additional invariants, so it's safe to transmute to `RawType`. unsafe impl RawDeviceId for DeviceId { type RawType = bindings::of_device_id; +} +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `data` field. +unsafe impl RawDeviceIdIndex for DeviceId { const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data); fn index(&self) -> usize { diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 8b884e324dcf..ba9a5c6fc513 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -6,7 +6,7 @@ use crate::{ bindings, container_of, device, - device_id::RawDeviceId, + device_id::{RawDeviceId, RawDeviceIdIndex}, devres::Devres, driver, error::{from_result, to_result, Result}, @@ -159,13 +159,14 @@ pub const fn from_class(class: u32, class_mask: u32) -> Self { } } -// SAFETY: -// * `DeviceId` is a `#[repr(transparent)]` wrapper of `pci_device_id` and does not add -// additional invariants, so it's safe to transmute to `RawType`. -// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field. +// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `pci_device_id` and does not add +// additional invariants, so it's safe to transmute to `RawType`. unsafe impl RawDeviceId for DeviceId { type RawType = bindings::pci_device_id; +} +// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field. +unsafe impl RawDeviceIdIndex for DeviceId { const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data); fn index(&self) -> usize { -- 2.43.0 Refactor the DeviceId struct to be a #[repr(transparent)] wrapper around the C struct bindings::mdio_device_id. This refactoring is a preparation for enabling the PHY abstractions to use the RawDeviceId trait. Reviewed-by: Trevor Gross Signed-off-by: FUJITA Tomonori --- rust/kernel/net/phy.rs | 55 +++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 32ea43ece646..f44e8107cec4 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -507,7 +507,7 @@ pub const fn create_phy_driver() -> DriverVTable { DriverVTable(Opaque::new(bindings::phy_driver { name: T::NAME.as_char_ptr().cast_mut(), flags: T::FLAGS, - phy_id: T::PHY_DEVICE_ID.id, + phy_id: T::PHY_DEVICE_ID.id(), phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(), soft_reset: if T::HAS_SOFT_RESET { Some(Adapter::::soft_reset_callback) @@ -691,42 +691,41 @@ fn drop(&mut self) { /// /// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate /// PHY driver. -pub struct DeviceId { - id: u32, - mask: DeviceMask, -} +#[repr(transparent)] +#[derive(Clone, Copy)] +pub struct DeviceId(bindings::mdio_device_id); impl DeviceId { /// Creates a new instance with the exact match mask. pub const fn new_with_exact_mask(id: u32) -> Self { - DeviceId { - id, - mask: DeviceMask::Exact, - } + Self(bindings::mdio_device_id { + phy_id: id, + phy_id_mask: DeviceMask::Exact.as_int(), + }) } /// Creates a new instance with the model match mask. pub const fn new_with_model_mask(id: u32) -> Self { - DeviceId { - id, - mask: DeviceMask::Model, - } + Self(bindings::mdio_device_id { + phy_id: id, + phy_id_mask: DeviceMask::Model.as_int(), + }) } /// Creates a new instance with the vendor match mask. pub const fn new_with_vendor_mask(id: u32) -> Self { - DeviceId { - id, - mask: DeviceMask::Vendor, - } + Self(bindings::mdio_device_id { + phy_id: id, + phy_id_mask: DeviceMask::Vendor.as_int(), + }) } /// Creates a new instance with a custom match mask. pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self { - DeviceId { - id, - mask: DeviceMask::Custom(mask), - } + Self(bindings::mdio_device_id { + phy_id: id, + phy_id_mask: DeviceMask::Custom(mask).as_int(), + }) } /// Creates a new instance from [`Driver`]. @@ -734,18 +733,20 @@ pub const fn new_with_driver() -> Self { T::PHY_DEVICE_ID } - /// Get a `mask` as u32. + /// Get the MDIO device's PHY ID. + pub const fn id(&self) -> u32 { + self.0.phy_id + } + + /// Get the MDIO device's match mask. pub const fn mask_as_int(&self) -> u32 { - self.mask.as_int() + self.0.phy_id_mask } // macro use only #[doc(hidden)] pub const fn mdio_device_id(&self) -> bindings::mdio_device_id { - bindings::mdio_device_id { - phy_id: self.id, - phy_id_mask: self.mask.as_int(), - } + self.0 } } -- 2.43.0 Change module_phy_driver macro to build device tables which are exported to userspace by using module_device_table macro. Reviewed-by: Trevor Gross Signed-off-by: FUJITA Tomonori --- rust/kernel/net/phy.rs | 51 ++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index f44e8107cec4..8b94f964f936 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -6,7 +6,7 @@ //! //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h). -use crate::{error::*, prelude::*, types::Opaque}; +use crate::{device_id::RawDeviceId, error::*, prelude::*, types::Opaque}; use core::{marker::PhantomData, ptr::addr_of_mut}; pub mod reg; @@ -750,6 +750,12 @@ pub const fn mdio_device_id(&self) -> bindings::mdio_device_id { } } +// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct mdio_device_id` +// and does not add additional invariants, so it's safe to transmute to `RawType`. +unsafe impl RawDeviceId for DeviceId { + type RawType = bindings::mdio_device_id; +} + enum DeviceMask { Exact, Model, @@ -850,19 +856,18 @@ const fn as_int(&self) -> u32 { /// } /// }; /// -/// const _DEVICE_TABLE: [::kernel::bindings::mdio_device_id; 2] = [ -/// ::kernel::bindings::mdio_device_id { -/// phy_id: 0x00000001, -/// phy_id_mask: 0xffffffff, -/// }, -/// ::kernel::bindings::mdio_device_id { -/// phy_id: 0, -/// phy_id_mask: 0, -/// }, -/// ]; -/// #[cfg(MODULE)] -/// #[no_mangle] -/// static __mod_device_table__mdio__phydev: [::kernel::bindings::mdio_device_id; 2] = _DEVICE_TABLE; +/// const N: usize = 1; +/// +/// const TABLE: ::kernel::device_id::IdArray<::kernel::net::phy::DeviceId, (), N> = +/// ::kernel::device_id::IdArray::new_without_index([ +/// ::kernel::net::phy::DeviceId( +/// ::kernel::bindings::mdio_device_id { +/// phy_id: 0x00000001, +/// phy_id_mask: 0xffffffff, +/// }), +/// ]); +/// +/// ::kernel::module_device_table!("mdio", phydev, TABLE); /// ``` #[macro_export] macro_rules! module_phy_driver { @@ -873,20 +878,12 @@ macro_rules! module_phy_driver { }; (@device_table [$($dev:expr),+]) => { - // SAFETY: C will not read off the end of this constant since the last element is zero. - const _DEVICE_TABLE: [$crate::bindings::mdio_device_id; - $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [ - $($dev.mdio_device_id()),+, - $crate::bindings::mdio_device_id { - phy_id: 0, - phy_id_mask: 0 - } - ]; + const N: usize = $crate::module_phy_driver!(@count_devices $($dev),+); + + const TABLE: $crate::device_id::IdArray<$crate::net::phy::DeviceId, (), N> = + $crate::device_id::IdArray::new_without_index([ $(($dev,())),+, ]); - #[cfg(MODULE)] - #[no_mangle] - static __mod_device_table__mdio__phydev: [$crate::bindings::mdio_device_id; - $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = _DEVICE_TABLE; + $crate::module_device_table!("mdio", phydev, TABLE); }; (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => { -- 2.43.0