[PATCH v6 0/3] rust: platform: add Io support

[PATCH v6 0/3] rust: platform: add Io support

[Daniel Almeida]

Changes in v6:

- Added Fiona as co-developer in the first patch, as I merged part of his code
from the LED driver series (thanks, Fiona)

- (Fiona) added the ResourceSize type, thereby fixing the u32 vs u64 issues
  pointed out by Christian

- Moved the request_region, release_region and friends to resource.rs

- Added the Region type. This type represents a resource returned by
  `request_region` and friends. It is also owned, representing the fact
  that the region remains marked as busy until release_region is called on
  drop. (Thanks Alice, for pointing out this pattern)

- Rewrote the IoMem abstraction to implement a separate type for exclusive
  access to an underlying region. I really disliked the `EXCLUSIVE` const
  generic, as it was definitely not ergonomic, i.e.:

  `IoMem<0, false>` 

  ...doesn't really say much. In fact, I believe that boolean parameters
  hurt readability in general.

  This new approach lets users build either regular IoMem's, which basically
  call ioremap under the covers, and ExclusiveIoMem's , which also call request_region
  via the Region type.

- Added access to the ioresource_port and ioresource_mem globals.

Link to v5: https://lore.kernel.org/rust-for-linux/20250116125632.65017-1-daniel.almeida@collabora.com/

Changes in v5:

- resend v5, as the r4l list was not cc'd
- use srctree where applicable in the docs (Alice)
- Remove 'mut' in Resource::from_ptr() (Alice)
- Add 'invariants' section for Resource (Alice)
- Fix typos in mem.rs (Alice)
- Turn 'exclusive' into a const generic (Alice)
- Fix example in platform.rs (Alice)
- Rework the resource.is_null() check (Alice)
- Refactor IoMem::new() to return DevRes<IoMem> directly (Danilo)

link to v4: https://lore.kernel.org/rust-for-linux/20250109133057.243751-1-daniel.almeida@collabora.com/

Changes in v4:

- Rebased on top of driver-core-next
- Split series in multiple patches (Danilo)
- Move IoMem and Resource into its own files (Danilo)
- Fix a missing "if exclusive {...}" check (Danilo)
- Fixed the example, since it was using the old API (Danilo)
- Use Opaque in `Resource`, instead of NonNull and PhantomData (Boqun)
- Highlight that non-exclusive access to the iomem might be required in some cases
- Fixed the safety comment in IoMem::deref()

Link to v3: https://lore.kernel.org/rust-for-linux/20241211-topic-panthor-rs-platform_io_support-v3-1-08ba707e5e3b@collabora.com/

Changes in v3:
- Rebased on top of v5 for the PCI/Platform abstractions
- platform_get_resource is now called only once when calling ioremap
- Introduced a platform::Resource type, which is bound to the lifetime of the
 platform Device
- Allow retrieving resources from the platform device either by index or
 name
- Make request_mem_region() optional
- Use resource.name() in request_mem_region
- Reword the example to remove an unaligned, out-of-bounds offset
- Update the safety requirements of platform::IoMem

Changes in v2:
- reworked the commit message
- added missing request_mem_region call (Thanks Alice, Danilo)
- IoMem::new() now takes the platform::Device, the resource number and
 the name, instead of an address and a size (thanks, Danilo)
- Added a new example for both sized and unsized versions of IoMem.
- Compiled the examples using kunit.py (thanks for the tip, Alice!)
- Removed instances of `foo as _`. All `as` casts now spell out the actual
 type.
- Now compiling with CLIPPY=1 (I realized I had forgotten, sorry)
- Rebased on top of rust-next to check for any warnings given the new
 unsafe lints.

Daniel Almeida (3):
  rust: io: add resource abstraction
  rust: io: mem: add a generic iomem abstraction
  rust: platform: allow ioremap of platform resources

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/io.c               |  36 +++++
 rust/kernel/io.rs               |   3 +
 rust/kernel/io/mem.rs           | 125 ++++++++++++++++
 rust/kernel/io/resource.rs      | 252 ++++++++++++++++++++++++++++++++
 rust/kernel/platform.rs         | 123 +++++++++++++++-
 6 files changed, 539 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/io/mem.rs
 create mode 100644 rust/kernel/io/resource.rs

-- 
2.48.0

[PATCH v6 1/3] rust: io: add resource abstraction

[Daniel Almeida]

In preparation for ioremap support, add a Rust abstraction for struct
resource.

A future commit will introduce the Rust API to ioremap a resource from a
platform device. The current abstraction, therefore, adds only the
minimum API needed to get that done.

Co-developed-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/io.c               |  36 +++++
 rust/kernel/io.rs               |   2 +
 rust/kernel/io/resource.rs      | 252 ++++++++++++++++++++++++++++++++
 4 files changed, 291 insertions(+)
 create mode 100644 rust/kernel/io/resource.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e9fdceb568b8..f9c2eedb5b9b 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -16,6 +16,7 @@
 #include <linux/file.h>
 #include <linux/firmware.h>
 #include <linux/fs.h>
+#include <linux/ioport.h>
 #include <linux/jiffies.h>
 #include <linux/jump_label.h>
 #include <linux/mdio.h>
diff --git a/rust/helpers/io.c b/rust/helpers/io.c
index 4c2401ccd720..939ab38ca61d 100644
--- a/rust/helpers/io.c
+++ b/rust/helpers/io.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/io.h>
+#include <linux/ioport.h>
 
 void __iomem *rust_helper_ioremap(phys_addr_t offset, size_t size)
 {
@@ -99,3 +100,38 @@ void rust_helper_writeq_relaxed(u64 value, volatile void __iomem *addr)
 	writeq_relaxed(value, addr);
 }
 #endif
+
+resource_size_t rust_helper_resource_size(struct resource *res)
+{
+	return resource_size(res);
+}
+
+struct resource *rust_helper_request_mem_region(resource_size_t start,
+						resource_size_t n,
+						const char *name)
+{
+	return request_mem_region(start, n, name);
+}
+
+void rust_helper_release_mem_region(resource_size_t start, resource_size_t n)
+{
+	release_mem_region(start, n);
+}
+
+struct resource *rust_helper_request_region(resource_size_t start,
+					    resource_size_t n, const char *name)
+{
+	return request_region(start, n, name);
+}
+
+struct resource *rust_helper_request_muxed_region(resource_size_t start,
+						  resource_size_t n,
+						  const char *name)
+{
+	return request_muxed_region(start, n, name);
+}
+
+void rust_helper_release_region(resource_size_t start, resource_size_t n)
+{
+	release_region(start, n);
+}
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index d4a73e52e3ee..566d8b177e01 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,8 @@
 use crate::error::{code::EINVAL, Result};
 use crate::{bindings, build_assert};
 
+pub mod resource;
+
 /// Raw representation of an MMIO region.
 ///
 /// By itself, the existence of an instance of this structure does not provide any guarantees that
diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
new file mode 100644
index 000000000000..64244a00786a
--- /dev/null
+++ b/rust/kernel/io/resource.rs
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstraction for system resources.
+//!
+//! C header: [`include/linux/ioport.h`](srctree/include/linux/ioport.h)
+
+use core::ops::Deref;
+use core::ptr::NonNull;
+
+use crate::str::CStr;
+use crate::types::Opaque;
+
+type RequestFn = unsafe extern "C" fn(
+    ResourceSize,
+    ResourceSize,
+    *const kernel::ffi::c_char,
+) -> *mut bindings::resource;
+
+#[cfg(CONFIG_HAS_IOPORT)]
+/// Returns a reference to the global `ioport_resource` variable.
+pub fn ioport_resource() -> &'static Resource {
+    // SAFETY: `bindings::ioport_resoure` has global lifetime and is of type Resource.
+    unsafe { Resource::from_ptr(core::ptr::addr_of_mut!(bindings::ioport_resource)) }
+}
+
+/// Returns a reference to the global `iomem_resource` variable.
+pub fn iomem_resource() -> &'static Resource {
+    // SAFETY: `bindings::iomem_resoure` has global lifetime and is of type Resource.
+    unsafe { Resource::from_ptr(core::ptr::addr_of_mut!(bindings::iomem_resource)) }
+}
+
+/// Resource Size type.
+/// This is a type alias to `u64`
+/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
+#[cfg(CONFIG_PHYS_ADDR_T_64BIT)]
+pub type ResourceSize = u64;
+
+/// Resource Size type.
+/// This is a type alias to `u32`
+/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
+#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
+pub type ResourceSize = u32;
+
+/// A region allocated from a parent resource.
+///
+/// # Invariants
+/// - `self.0` points to a valid `bindings::resource` that was obtained through
+/// `__request_region`.
+pub struct Region(NonNull<bindings::resource>);
+
+impl Deref for Region {
+    type Target = Resource;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: Safe as per the invariant of `Region`
+        unsafe { Resource::from_ptr(self.0.as_ptr()) }
+    }
+}
+
+impl Drop for Region {
+    fn drop(&mut self) {
+        // SAFETY: Safe as per the invariant of `Region`
+        let res = unsafe { Resource::from_ptr(self.0.as_ptr()) };
+        let flags = res.flags();
+
+        let release_fn = if flags.contains(flags::IORESOURCE_MEM) {
+            bindings::release_mem_region
+        } else {
+            bindings::release_region
+        };
+
+        // SAFETY: Safe as per the invariant of `Region`
+        unsafe { release_fn(res.start(), res.size()) };
+    }
+}
+
+// SAFETY: `Region` only holds a pointer to a C `struct resource`, which is safe to be used from
+// any thead.
+unsafe impl Send for Region {}
+
+// SAFETY: `Region` only holds a pointer to a C `struct resource`, references to which are
+// safe to be used from any thead.
+unsafe impl Sync for Region {}
+
+/// A resource abstraction.
+///
+/// # Invariants
+///
+/// `Resource` is a transparent wrapper around a valid `bindings::resource`.
+#[repr(transparent)]
+pub struct Resource(Opaque<bindings::resource>);
+
+impl Resource {
+    /// Creates a reference to a [`Resource`] from a valid pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that for the duration of 'a, the pointer will
+    /// point at a valid `bindings::resource`
+    ///
+    /// The caller must also ensure that the `Resource` is only accessed via the
+    /// returned reference for the duration of 'a.
+    pub(crate) const unsafe fn from_ptr<'a>(ptr: *mut bindings::resource) -> &'a Self {
+        // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// A helper to abstract the common pattern of requesting a region.
+    fn request_region_checked(
+        &self,
+        start: ResourceSize,
+        size: ResourceSize,
+        name: &CStr,
+        request_fn: RequestFn,
+    ) -> Option<Region> {
+        // SAFETY: Safe as per the invariant of `Resource`
+        let region = unsafe { request_fn(start, size, name.as_char_ptr()) };
+
+        Some(Region(NonNull::new(region)?))
+    }
+
+    /// Requests a resource region.
+    ///
+    /// Exclusive access will be given and the region will be marked as busy.
+    /// Further calls to `request_region` will return `None` if the region, or a
+    /// part of it, is already in use.
+    pub fn request_region(
+        &self,
+        start: ResourceSize,
+        size: ResourceSize,
+        name: &CStr,
+    ) -> Option<Region> {
+        self.request_region_checked(start, size, name, bindings::request_region)
+    }
+
+    /// Requests a resource region with the IORESOURCE_MUXED flag.
+    ///
+    /// Exclusive access will be given and the region will be marked as busy.
+    /// Further calls to `request_region` will return `None` if the region, or a
+    /// part of it, is already in use.
+    pub fn request_muxed_region(
+        &self,
+        start: ResourceSize,
+        size: ResourceSize,
+        name: &CStr,
+    ) -> Option<Region> {
+        self.request_region_checked(start, size, name, bindings::request_muxed_region)
+    }
+
+    /// Requests a memory resource region, i.e.: a resource of type
+    /// IORESOURCE_MEM.
+    ///
+    /// Exclusive access will be given and the region will be marked as busy.
+    /// Further calls to `request_region` will return `None` if the region, or a
+    /// part of it, is already in use.
+    pub fn request_mem_region(
+        &self,
+        start: ResourceSize,
+        size: ResourceSize,
+        name: &CStr,
+    ) -> Option<Region> {
+        self.request_region_checked(start, size, name, bindings::request_mem_region)
+    }
+
+    /// Returns the size of the resource.
+    pub fn size(&self) -> ResourceSize {
+        let inner = self.0.get();
+        // SAFETY: safe as per the invariants of `Resource`
+        unsafe { bindings::resource_size(inner) }
+    }
+
+    /// Returns the start address of the resource.
+    pub fn start(&self) -> u64 {
+        let inner = self.0.get();
+        // SAFETY: safe as per the invariants of `Resource`
+        unsafe { *inner }.start
+    }
+
+    /// Returns the name of the resource.
+    pub fn name(&self) -> &CStr {
+        let inner = self.0.get();
+        // SAFETY: safe as per the invariants of `Resource`
+        unsafe { CStr::from_char_ptr((*inner).name) }
+    }
+
+    /// Returns the flags associated with the resource.
+    pub fn flags(&self) -> Flags {
+        let inner = self.0.get();
+        // SAFETY: safe as per the invariants of `Resource`
+        let flags = unsafe { *inner }.flags;
+
+        Flags(flags)
+    }
+}
+
+// SAFETY: `Resource` only holds a pointer to a C `struct resource`, which is safe to be used from
+// any thead.
+unsafe impl Send for Resource {}
+
+// SAFETY: `Resource` only holds a pointer to a C `struct resource`, references to which are
+// safe to be used from any thead.
+unsafe impl Sync for Resource {}
+
+/// Resource flags as stored in the C `struct resource::flags` field.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+///
+/// Values can be used from the [`flags`] module.
+#[derive(Clone, Copy, PartialEq)]
+pub struct Flags(u64);
+
+impl Flags {
+    /// Check whether `flags` is contained in `self`.
+    pub fn contains(self, flags: Flags) -> bool {
+        (self & flags) == flags
+    }
+}
+
+impl core::ops::BitOr for Flags {
+    type Output = Self;
+    fn bitor(self, rhs: Self) -> Self::Output {
+        Self(self.0 | rhs.0)
+    }
+}
+
+impl core::ops::BitAnd for Flags {
+    type Output = Self;
+    fn bitand(self, rhs: Self) -> Self::Output {
+        Self(self.0 & rhs.0)
+    }
+}
+
+impl core::ops::Not for Flags {
+    type Output = Self;
+    fn not(self) -> Self::Output {
+        Self(!self.0)
+    }
+}
+
+/// Resource flags as stored in the `struct resource::flags` field.
+pub mod flags {
+    use super::Flags;
+
+    /// PCI/ISA I/O ports
+    pub const IORESOURCE_IO: Flags = Flags(bindings::IORESOURCE_IO as u64);
+
+    /// Resource is software muxed.
+    pub const IORESOURCE_MUXED: Flags = Flags(bindings::IORESOURCE_MUXED as u64);
+
+    /// Resource represents a memory region.
+    pub const IORESOURCE_MEM: Flags = Flags(bindings::IORESOURCE_MEM as u64);
+}
-- 
2.48.0

Re: [PATCH v6 1/3] rust: io: add resource abstraction

[Daniel Sedlak]

Hi,

On 1/30/25 11:05 PM, Daniel Almeida wrote:

> +/// Returns a reference to the global `iomem_resource` variable.
> +pub fn iomem_resource() -> &'static Resource {
> +    // SAFETY: `bindings::iomem_resoure` has global lifetime and is of type Resource.
> +    unsafe { Resource::from_ptr(core::ptr::addr_of_mut!(bindings::iomem_resource)) }
> +}
> +
> +/// Resource Size type.
> +/// This is a type alias to `u64`
> +/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.

The comment seems weirdly formatted, shouldn't it be rather:

/// Resource size type. This is a type alias to `u64`
/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.

or

/// Resource size type.
///
/// This is a type alias to `u64` depending on the config
/// option `CONFIG_PHYS_ADDR_T_64BIT`.

> +#[cfg(CONFIG_PHYS_ADDR_T_64BIT)]
> +pub type ResourceSize = u64;
> +
> +/// Resource Size type.
> +/// This is a type alias to `u32`
> +/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.

Similar to the previous one.

> +#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
> +pub type ResourceSize = u32;
> +
> +/// A region allocated from a parent resource.
> +///
> +/// # Invariants
> +/// - `self.0` points to a valid `bindings::resource` that was obtained through
> +/// `__request_region`.

Shouldn't be there an extra newline after # Invariants, to be consistent 
with others in the patch?

> +pub struct Region(NonNull<bindings::resource>);
> +
> +impl Deref for Region {
> +    type Target = Resource;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: Safe as per the invariant of `Region`
> +        unsafe { Resource::from_ptr(self.0.as_ptr()) }
> +    }
> +}
> +
> +impl Drop for Region {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as per the invariant of `Region`
> +        let res = unsafe { Resource::from_ptr(self.0.as_ptr()) };
> +        let flags = res.flags();
> +
> +        let release_fn = if flags.contains(flags::IORESOURCE_MEM) {
> +            bindings::release_mem_region
> +        } else {
> +            bindings::release_region
> +        };
> +
> +        // SAFETY: Safe as per the invariant of `Region`
> +        unsafe { release_fn(res.start(), res.size()) };
> +    }
> +}
> +
> +// SAFETY: `Region` only holds a pointer to a C `struct resource`, which is safe to be used from
> +// any thead.

typo thead -> thread
> +unsafe impl Send for Region {}
> +
> +// SAFETY: `Region` only holds a pointer to a C `struct resource`, references to which are
> +// safe to be used from any thead.

typo thead -> thread

> +unsafe impl Sync for Region {}
> +
> +/// A resource abstraction.
> +///
> +/// # Invariants
> +///
> +/// `Resource` is a transparent wrapper around a valid `bindings::resource`.
> +#[repr(transparent)]
> +pub struct Resource(Opaque<bindings::resource>);
> +
> +impl Resource {
> +    /// Creates a reference to a [`Resource`] from a valid pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that for the duration of 'a, the pointer will
> +    /// point at a valid `bindings::resource`
> +    ///
> +    /// The caller must also ensure that the `Resource` is only accessed via the
> +    /// returned reference for the duration of 'a.
> +    pub(crate) const unsafe fn from_ptr<'a>(ptr: *mut bindings::resource) -> &'a Self {
> +        // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// A helper to abstract the common pattern of requesting a region.
> +    fn request_region_checked(
> +        &self,
> +        start: ResourceSize,
> +        size: ResourceSize,
> +        name: &CStr,
> +        request_fn: RequestFn,
> +    ) -> Option<Region> {
> +        // SAFETY: Safe as per the invariant of `Resource`
> +        let region = unsafe { request_fn(start, size, name.as_char_ptr()) };
> +
> +        Some(Region(NonNull::new(region)?))
> +    }
> +
> +    /// Requests a resource region.
> +    ///
> +    /// Exclusive access will be given and the region will be marked as busy.
> +    /// Further calls to `request_region` will return `None` if the region, or a
> +    /// part of it, is already in use.
> +    pub fn request_region(
> +        &self,
> +        start: ResourceSize,
> +        size: ResourceSize,
> +        name: &CStr,
> +    ) -> Option<Region> {
> +        self.request_region_checked(start, size, name, bindings::request_region)
> +    }
> +
> +    /// Requests a resource region with the IORESOURCE_MUXED flag.

formatting: IORESOURCE_MUXED -> `IORESOURCE_MUXED`

> +    ///
> +    /// Exclusive access will be given and the region will be marked as busy.
> +    /// Further calls to `request_region` will return `None` if the region, or a
> +    /// part of it, is already in use.
> +    pub fn request_muxed_region(
> +        &self,
> +        start: ResourceSize,
> +        size: ResourceSize,
> +        name: &CStr,
> +    ) -> Option<Region> {
> +        self.request_region_checked(start, size, name, bindings::request_muxed_region)
> +    }
> +
> +    /// Requests a memory resource region, i.e.: a resource of type
> +    /// IORESOURCE_MEM.

formatting: IORESOURCE_MEM -> `IORESOURCE_MEM`

> +    ///
> +    /// Exclusive access will be given and the region will be marked as busy.
> +    /// Further calls to `request_region` will return `None` if the region, or a
> +    /// part of it, is already in use.
> +    pub fn request_mem_region(
> +        &self,
> +        start: ResourceSize,
> +        size: ResourceSize,
> +        name: &CStr,
> +    ) -> Option<Region> {
> +        self.request_region_checked(start, size, name, bindings::request_mem_region)
> +    }
> +
> +    /// Returns the size of the resource.
> +    pub fn size(&self) -> ResourceSize {
> +        let inner = self.0.get();
> +        // SAFETY: safe as per the invariants of `Resource`
> +        unsafe { bindings::resource_size(inner) }
> +    }
> +
> +    /// Returns the start address of the resource.
> +    pub fn start(&self) -> u64 {

Should the address be of type `usize`?

> +        let inner = self.0.get();
> +        // SAFETY: safe as per the invariants of `Resource`
> +        unsafe { *inner }.start
> +    }
> +
> +    /// Returns the name of the resource.
> +    pub fn name(&self) -> &CStr {
> +        let inner = self.0.get();
> +        // SAFETY: safe as per the invariants of `Resource`
> +        unsafe { CStr::from_char_ptr((*inner).name) }
> +    }
> +
> +    /// Returns the flags associated with the resource.
> +    pub fn flags(&self) -> Flags {
> +        let inner = self.0.get();
> +        // SAFETY: safe as per the invariants of `Resource`
> +        let flags = unsafe { *inner }.flags;
> +
> +        Flags(flags)
> +    }
> +}
> +
> +// SAFETY: `Resource` only holds a pointer to a C `struct resource`, which is safe to be used from
> +// any thead.

typo: thead -> thread

> +unsafe impl Send for Resource {}
> +
> +// SAFETY: `Resource` only holds a pointer to a C `struct resource`, references to which are
> +// safe to be used from any thead.

typo: thead -> thread

> +unsafe impl Sync for Resource {}
> +
> +/// Resource flags as stored in the C `struct resource::flags` field.
> +///
> +/// They can be combined with the operators `|`, `&`, and `!`.
> +///
> +/// Values can be used from the [`flags`] module.
> +#[derive(Clone, Copy, PartialEq)]
> +pub struct Flags(u64);
> +
> +impl Flags {
> +    /// Check whether `flags` is contained in `self`.
> +    pub fn contains(self, flags: Flags) -> bool {
> +        (self & flags) == flags
> +    }
> +}
> +
> +impl core::ops::BitOr for Flags {
> +    type Output = Self;
> +    fn bitor(self, rhs: Self) -> Self::Output {
> +        Self(self.0 | rhs.0)
> +    }
> +}
> +
> +impl core::ops::BitAnd for Flags {
> +    type Output = Self;
> +    fn bitand(self, rhs: Self) -> Self::Output {
> +        Self(self.0 & rhs.0)
> +    }
> +}
> +
> +impl core::ops::Not for Flags {
> +    type Output = Self;
> +    fn not(self) -> Self::Output {
> +        Self(!self.0)
> +    }
> +}
> +
> +/// Resource flags as stored in the `struct resource::flags` field.
> +pub mod flags {
> +    use super::Flags;
> +
> +    /// PCI/ISA I/O ports

formatting: period at the end

> +    pub const IORESOURCE_IO: Flags = Flags(bindings::IORESOURCE_IO as u64);
> +
> +    /// Resource is software muxed.
> +    pub const IORESOURCE_MUXED: Flags = Flags(bindings::IORESOURCE_MUXED as u64);
> +
> +    /// Resource represents a memory region.
> +    pub const IORESOURCE_MEM: Flags = Flags(bindings::IORESOURCE_MEM as u64);
> +}

	Daniel

Re: [PATCH v6 1/3] rust: io: add resource abstraction

[Guangbo Cui]

> +/// A region allocated from a parent resource.
> +///
> +/// # Invariants
> +/// - `self.0` points to a valid `bindings::resource` that was obtained through
> +/// `__request_region`.

Doc list item without indentation.

Best regards,
Guangbo Cui

[PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Daniel Almeida]

Add a generic iomem abstraction to safely read and write ioremapped
regions.

The reads and writes are done through IoRaw, and are thus checked either
at compile-time, if the size of the region is known at that point, or at
runtime otherwise.

Non-exclusive access to the underlying memory region is made possible to
cater to cases where overlapped regions are unavoidable.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/io.rs     |   1 +
 rust/kernel/io/mem.rs | 125 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 rust/kernel/io/mem.rs

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 566d8b177e01..9ce3482b5ecd 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,7 @@
 use crate::error::{code::EINVAL, Result};
 use crate::{bindings, build_assert};
 
+pub mod mem;
 pub mod resource;
 
 /// Raw representation of an MMIO region.
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
new file mode 100644
index 000000000000..f87433ed858e
--- /dev/null
+++ b/rust/kernel/io/mem.rs
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic memory-mapped IO.
+
+use core::ops::Deref;
+
+use crate::device::Device;
+use crate::devres::Devres;
+use crate::io::resource::Region;
+use crate::io::resource::Resource;
+use crate::io::Io;
+use crate::io::IoRaw;
+use crate::prelude::*;
+
+/// An exclusive memory-mapped IO region.
+///
+/// # Invariants
+///
+/// - ExclusiveIoMem has exclusive access to the underlying `iomem`.
+pub struct ExclusiveIoMem<const SIZE: usize> {
+    /// The region abstraction. This represents exclusive access to the
+    /// range represented by the underlying `iomem`.
+    ///
+    /// It's placed first to ensure that the region is released before it is
+    /// unmapped as a result of the drop order.
+    #[allow(dead_code)]
+    region: Region,
+    /// The underlying `IoMem` instance.
+    iomem: IoMem<SIZE>,
+}
+
+impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
+    /// Creates a new `ExclusiveIoMem` instance.
+    pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
+        let iomem = IoMem::ioremap(resource)?;
+
+        let start = resource.start();
+        let size = resource.size();
+        let name = resource.name();
+
+        let region = resource
+            .request_mem_region(start, size, name)
+            .ok_or(EBUSY)?;
+
+        let iomem = ExclusiveIoMem { iomem, region };
+
+        Ok(iomem)
+    }
+
+    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
+        let iomem = Self::ioremap(resource)?;
+        let devres = Devres::new(device, iomem, GFP_KERNEL)?;
+
+        Ok(devres)
+    }
+}
+
+impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
+    type Target = Io<SIZE>;
+
+    fn deref(&self) -> &Self::Target {
+        &*self.iomem
+    }
+}
+
+/// A generic memory-mapped IO region.
+///
+/// Accesses to the underlying region is checked either at compile time, if the
+/// region's size is known at that point, or at runtime otherwise.
+///
+/// # Invariants
+///
+/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
+/// start of the I/O memory mapped region.
+pub struct IoMem<const SIZE: usize = 0> {
+    io: IoRaw<SIZE>,
+}
+
+impl<const SIZE: usize> IoMem<SIZE> {
+    fn ioremap(resource: &Resource) -> Result<Self> {
+        let size = resource.size();
+        if size == 0 {
+            return Err(EINVAL);
+        }
+
+        let res_start = resource.start();
+
+        // SAFETY:
+        // - `res_start` and `size` are read from a presumably valid `struct resource`.
+        // - `size` is known not to be zero at this point.
+        let addr = unsafe { bindings::ioremap(res_start, size as kernel::ffi::c_ulong) };
+        if addr.is_null() {
+            return Err(ENOMEM);
+        }
+
+        let io = IoRaw::new(addr as usize, size as usize)?;
+        let io = IoMem { io };
+
+        Ok(io)
+    }
+
+    /// Creates a new `IoMem` instance.
+    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
+        let io = Self::ioremap(resource)?;
+        let devres = Devres::new(device, io, GFP_KERNEL)?;
+
+        Ok(devres)
+    }
+}
+
+impl<const SIZE: usize> Drop for IoMem<SIZE> {
+    fn drop(&mut self) {
+        // SAFETY: Safe as by the invariant of `Io`.
+        unsafe { bindings::iounmap(self.io.addr() as *mut core::ffi::c_void) }
+    }
+}
+
+impl<const SIZE: usize> Deref for IoMem<SIZE> {
+    type Target = Io<SIZE>;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: Safe as by the invariant of `IoMem`.
+        unsafe { Io::from_raw(&self.io) }
+    }
+}
-- 
2.48.0

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Daniel Sedlak]

Hi,

On 1/30/25 11:05 PM, Daniel Almeida wrote:
> Add a generic iomem abstraction to safely read and write ioremapped
> regions.
> 
> The reads and writes are done through IoRaw, and are thus checked either
> at compile-time, if the size of the region is known at that point, or at
> runtime otherwise.
> 
> Non-exclusive access to the underlying memory region is made possible to
> cater to cases where overlapped regions are unavoidable.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>   rust/kernel/io.rs     |   1 +
>   rust/kernel/io/mem.rs | 125 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 126 insertions(+)
>   create mode 100644 rust/kernel/io/mem.rs
> 
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 566d8b177e01..9ce3482b5ecd 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,7 @@
>   use crate::error::{code::EINVAL, Result};
>   use crate::{bindings, build_assert};
>   
> +pub mod mem;
>   pub mod resource;
>   
>   /// Raw representation of an MMIO region.
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> new file mode 100644
> index 000000000000..f87433ed858e
> --- /dev/null
> +++ b/rust/kernel/io/mem.rs
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic memory-mapped IO.
> +
> +use core::ops::Deref;
> +
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::io::resource::Region;
> +use crate::io::resource::Resource;
> +use crate::io::Io;
> +use crate::io::IoRaw;
> +use crate::prelude::*;
> +
> +/// An exclusive memory-mapped IO region.
> +///
> +/// # Invariants
> +///
> +/// - ExclusiveIoMem has exclusive access to the underlying `iomem`.

formatting: ExclusiveIoMem -> [`ExclusiveIoMem`]?

> +pub struct ExclusiveIoMem<const SIZE: usize> {
> +    /// The region abstraction. This represents exclusive access to the
> +    /// range represented by the underlying `iomem`.
> +    ///
> +    /// It's placed first to ensure that the region is released before it is
> +    /// unmapped as a result of the drop order.
> +    #[allow(dead_code)]
> +    region: Region,
> +    /// The underlying `IoMem` instance.
> +    iomem: IoMem<SIZE>,
> +}
> +
> +impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
> +    /// Creates a new `ExclusiveIoMem` instance.
> +    pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
> +        let iomem = IoMem::ioremap(resource)?;
> +
> +        let start = resource.start();
> +        let size = resource.size();
> +        let name = resource.name();
> +
> +        let region = resource
> +            .request_mem_region(start, size, name)
> +            .ok_or(EBUSY)?;
> +
> +        let iomem = ExclusiveIoMem { iomem, region };
> +
> +        Ok(iomem)
> +    }
> +
> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> +        let iomem = Self::ioremap(resource)?;
> +        let devres = Devres::new(device, iomem, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &*self.iomem
> +    }
> +}
> +
> +/// A generic memory-mapped IO region.
> +///
> +/// Accesses to the underlying region is checked either at compile time, if the
> +/// region's size is known at that point, or at runtime otherwise.
> +///
> +/// # Invariants
> +///
> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the

typo: inststance -> instance
> +/// start of the I/O memory mapped region.
> +pub struct IoMem<const SIZE: usize = 0> {
> +    io: IoRaw<SIZE>,
> +}
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    fn ioremap(resource: &Resource) -> Result<Self> {
> +        let size = resource.size();
> +        if size == 0 {
> +            return Err(EINVAL);
> +        }
> +
> +        let res_start = resource.start();
> +
> +        // SAFETY:
> +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
> +        // - `size` is known not to be zero at this point.
> +        let addr = unsafe { bindings::ioremap(res_start, size as kernel::ffi::c_ulong) };
> +        if addr.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        let io = IoRaw::new(addr as usize, size as usize)?;
> +        let io = IoMem { io };
> +
> +        Ok(io)
> +    }
> +
> +    /// Creates a new `IoMem` instance.
> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> +        let io = Self::ioremap(resource)?;
> +        let devres = Devres::new(device, io, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.io.addr() as *mut core::ffi::c_void) }
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: Safe as by the invariant of `IoMem`.
> +        unsafe { Io::from_raw(&self.io) }
> +    }
> +}
	Daniel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Asahi Lina]

On 1/31/25 7:05 AM, Daniel Almeida wrote:
> Add a generic iomem abstraction to safely read and write ioremapped
> regions.
> 
> The reads and writes are done through IoRaw, and are thus checked either
> at compile-time, if the size of the region is known at that point, or at
> runtime otherwise.
> 
> Non-exclusive access to the underlying memory region is made possible to
> cater to cases where overlapped regions are unavoidable.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/io.rs     |   1 +
>  rust/kernel/io/mem.rs | 125 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 rust/kernel/io/mem.rs
> 
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 566d8b177e01..9ce3482b5ecd 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,7 @@
>  use crate::error::{code::EINVAL, Result};
>  use crate::{bindings, build_assert};
>  
> +pub mod mem;
>  pub mod resource;
>  
>  /// Raw representation of an MMIO region.
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> new file mode 100644
> index 000000000000..f87433ed858e
> --- /dev/null
> +++ b/rust/kernel/io/mem.rs
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic memory-mapped IO.
> +
> +use core::ops::Deref;
> +
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::io::resource::Region;
> +use crate::io::resource::Resource;
> +use crate::io::Io;
> +use crate::io::IoRaw;
> +use crate::prelude::*;
> +
> +/// An exclusive memory-mapped IO region.
> +///
> +/// # Invariants
> +///
> +/// - ExclusiveIoMem has exclusive access to the underlying `iomem`.
> +pub struct ExclusiveIoMem<const SIZE: usize> {
> +    /// The region abstraction. This represents exclusive access to the
> +    /// range represented by the underlying `iomem`.
> +    ///
> +    /// It's placed first to ensure that the region is released before it is
> +    /// unmapped as a result of the drop order.
> +    #[allow(dead_code)]
> +    region: Region,
> +    /// The underlying `IoMem` instance.
> +    iomem: IoMem<SIZE>,
> +}
> +
> +impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
> +    /// Creates a new `ExclusiveIoMem` instance.
> +    pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
> +        let iomem = IoMem::ioremap(resource)?;
> +
> +        let start = resource.start();
> +        let size = resource.size();
> +        let name = resource.name();
> +
> +        let region = resource
> +            .request_mem_region(start, size, name)
> +            .ok_or(EBUSY)?;
> +
> +        let iomem = ExclusiveIoMem { iomem, region };
> +
> +        Ok(iomem)
> +    }
> +
> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> +        let iomem = Self::ioremap(resource)?;
> +        let devres = Devres::new(device, iomem, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &*self.iomem
> +    }
> +}
> +
> +/// A generic memory-mapped IO region.
> +///
> +/// Accesses to the underlying region is checked either at compile time, if the
> +/// region's size is known at that point, or at runtime otherwise.
> +///
> +/// # Invariants
> +///
> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> +/// start of the I/O memory mapped region.
> +pub struct IoMem<const SIZE: usize = 0> {
> +    io: IoRaw<SIZE>,
> +}
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    fn ioremap(resource: &Resource) -> Result<Self> {
> +        let size = resource.size();
> +        if size == 0 {
> +            return Err(EINVAL);
> +        }
> +
> +        let res_start = resource.start();
> +
> +        // SAFETY:
> +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
> +        // - `size` is known not to be zero at this point.
> +        let addr = unsafe { bindings::ioremap(res_start, size as kernel::ffi::c_ulong) };

This does not work for all platforms. Some, like Apple platforms,
require ioremap_np(). You should check for the
`IORESOURCE_MEM_NONPOSTED` resource flag and use the appropriate
function, like this (from an older spin of this code):

https://github.com/AsahiLinux/linux/blob/fce34c83f1dca5b10cc2c866fd8832a362de7974/rust/kernel/io_mem.rs#L166C36-L166C70

> +        if addr.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        let io = IoRaw::new(addr as usize, size as usize)?;
> +        let io = IoMem { io };
> +
> +        Ok(io)
> +    }
> +
> +    /// Creates a new `IoMem` instance.
> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> +        let io = Self::ioremap(resource)?;
> +        let devres = Devres::new(device, io, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.io.addr() as *mut core::ffi::c_void) }
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: Safe as by the invariant of `IoMem`.
> +        unsafe { Io::from_raw(&self.io) }
> +    }
> +}

~~ Lina

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Alice Ryhl]

On Sun, Feb 2, 2025 at 11:45 PM Asahi Lina <lina@asahilina.net> wrote:
>
>
>
> On 1/31/25 7:05 AM, Daniel Almeida wrote:
> > Add a generic iomem abstraction to safely read and write ioremapped
> > regions.
> >
> > The reads and writes are done through IoRaw, and are thus checked either
> > at compile-time, if the size of the region is known at that point, or at
> > runtime otherwise.
> >
> > Non-exclusive access to the underlying memory region is made possible to
> > cater to cases where overlapped regions are unavoidable.
> >
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> > ---
> >  rust/kernel/io.rs     |   1 +
> >  rust/kernel/io/mem.rs | 125 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 126 insertions(+)
> >  create mode 100644 rust/kernel/io/mem.rs
> >
> > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> > index 566d8b177e01..9ce3482b5ecd 100644
> > --- a/rust/kernel/io.rs
> > +++ b/rust/kernel/io.rs
> > @@ -7,6 +7,7 @@
> >  use crate::error::{code::EINVAL, Result};
> >  use crate::{bindings, build_assert};
> >
> > +pub mod mem;
> >  pub mod resource;
> >
> >  /// Raw representation of an MMIO region.
> > diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> > new file mode 100644
> > index 000000000000..f87433ed858e
> > --- /dev/null
> > +++ b/rust/kernel/io/mem.rs
> > @@ -0,0 +1,125 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Generic memory-mapped IO.
> > +
> > +use core::ops::Deref;
> > +
> > +use crate::device::Device;
> > +use crate::devres::Devres;
> > +use crate::io::resource::Region;
> > +use crate::io::resource::Resource;
> > +use crate::io::Io;
> > +use crate::io::IoRaw;
> > +use crate::prelude::*;
> > +
> > +/// An exclusive memory-mapped IO region.
> > +///
> > +/// # Invariants
> > +///
> > +/// - ExclusiveIoMem has exclusive access to the underlying `iomem`.
> > +pub struct ExclusiveIoMem<const SIZE: usize> {
> > +    /// The region abstraction. This represents exclusive access to the
> > +    /// range represented by the underlying `iomem`.
> > +    ///
> > +    /// It's placed first to ensure that the region is released before it is
> > +    /// unmapped as a result of the drop order.
> > +    #[allow(dead_code)]
> > +    region: Region,
> > +    /// The underlying `IoMem` instance.
> > +    iomem: IoMem<SIZE>,
> > +}
> > +
> > +impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
> > +    /// Creates a new `ExclusiveIoMem` instance.
> > +    pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
> > +        let iomem = IoMem::ioremap(resource)?;
> > +
> > +        let start = resource.start();
> > +        let size = resource.size();
> > +        let name = resource.name();
> > +
> > +        let region = resource
> > +            .request_mem_region(start, size, name)
> > +            .ok_or(EBUSY)?;
> > +
> > +        let iomem = ExclusiveIoMem { iomem, region };
> > +
> > +        Ok(iomem)
> > +    }
> > +
> > +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> > +        let iomem = Self::ioremap(resource)?;
> > +        let devres = Devres::new(device, iomem, GFP_KERNEL)?;
> > +
> > +        Ok(devres)
> > +    }
> > +}
> > +
> > +impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
> > +    type Target = Io<SIZE>;
> > +
> > +    fn deref(&self) -> &Self::Target {
> > +        &*self.iomem
> > +    }
> > +}
> > +
> > +/// A generic memory-mapped IO region.
> > +///
> > +/// Accesses to the underlying region is checked either at compile time, if the
> > +/// region's size is known at that point, or at runtime otherwise.
> > +///
> > +/// # Invariants
> > +///
> > +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> > +/// start of the I/O memory mapped region.
> > +pub struct IoMem<const SIZE: usize = 0> {
> > +    io: IoRaw<SIZE>,
> > +}
> > +
> > +impl<const SIZE: usize> IoMem<SIZE> {
> > +    fn ioremap(resource: &Resource) -> Result<Self> {
> > +        let size = resource.size();
> > +        if size == 0 {
> > +            return Err(EINVAL);
> > +        }
> > +
> > +        let res_start = resource.start();
> > +
> > +        // SAFETY:
> > +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
> > +        // - `size` is known not to be zero at this point.
> > +        let addr = unsafe { bindings::ioremap(res_start, size as kernel::ffi::c_ulong) };
>
> This does not work for all platforms. Some, like Apple platforms,
> require ioremap_np(). You should check for the
> `IORESOURCE_MEM_NONPOSTED` resource flag and use the appropriate
> function, like this (from an older spin of this code):
>
> https://github.com/AsahiLinux/linux/blob/fce34c83f1dca5b10cc2c866fd8832a362de7974/rust/kernel/io_mem.rs#L166C36-L166C70

Hmm. Is detecting this based on the flag the right approach? On the C
side, they use different methods and I wonder if we should do the
same?

Alice

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Asahi Lina]

On 2/3/25 6:26 PM, Alice Ryhl wrote:
> On Sun, Feb 2, 2025 at 11:45 PM Asahi Lina <lina@asahilina.net> wrote:
>>
>>
>>
>> On 1/31/25 7:05 AM, Daniel Almeida wrote:
>>> Add a generic iomem abstraction to safely read and write ioremapped
>>> regions.
>>>
>>> The reads and writes are done through IoRaw, and are thus checked either
>>> at compile-time, if the size of the region is known at that point, or at
>>> runtime otherwise.
>>>
>>> Non-exclusive access to the underlying memory region is made possible to
>>> cater to cases where overlapped regions are unavoidable.
>>>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> ---
>>>  rust/kernel/io.rs     |   1 +
>>>  rust/kernel/io/mem.rs | 125 ++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 126 insertions(+)
>>>  create mode 100644 rust/kernel/io/mem.rs
>>>
>>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>>> index 566d8b177e01..9ce3482b5ecd 100644
>>> --- a/rust/kernel/io.rs
>>> +++ b/rust/kernel/io.rs
>>> @@ -7,6 +7,7 @@
>>>  use crate::error::{code::EINVAL, Result};
>>>  use crate::{bindings, build_assert};
>>>
>>> +pub mod mem;
>>>  pub mod resource;
>>>
>>>  /// Raw representation of an MMIO region.
>>> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
>>> new file mode 100644
>>> index 000000000000..f87433ed858e
>>> --- /dev/null
>>> +++ b/rust/kernel/io/mem.rs
>>> @@ -0,0 +1,125 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Generic memory-mapped IO.
>>> +
>>> +use core::ops::Deref;
>>> +
>>> +use crate::device::Device;
>>> +use crate::devres::Devres;
>>> +use crate::io::resource::Region;
>>> +use crate::io::resource::Resource;
>>> +use crate::io::Io;
>>> +use crate::io::IoRaw;
>>> +use crate::prelude::*;
>>> +
>>> +/// An exclusive memory-mapped IO region.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - ExclusiveIoMem has exclusive access to the underlying `iomem`.
>>> +pub struct ExclusiveIoMem<const SIZE: usize> {
>>> +    /// The region abstraction. This represents exclusive access to the
>>> +    /// range represented by the underlying `iomem`.
>>> +    ///
>>> +    /// It's placed first to ensure that the region is released before it is
>>> +    /// unmapped as a result of the drop order.
>>> +    #[allow(dead_code)]
>>> +    region: Region,
>>> +    /// The underlying `IoMem` instance.
>>> +    iomem: IoMem<SIZE>,
>>> +}
>>> +
>>> +impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
>>> +    /// Creates a new `ExclusiveIoMem` instance.
>>> +    pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
>>> +        let iomem = IoMem::ioremap(resource)?;
>>> +
>>> +        let start = resource.start();
>>> +        let size = resource.size();
>>> +        let name = resource.name();
>>> +
>>> +        let region = resource
>>> +            .request_mem_region(start, size, name)
>>> +            .ok_or(EBUSY)?;
>>> +
>>> +        let iomem = ExclusiveIoMem { iomem, region };
>>> +
>>> +        Ok(iomem)
>>> +    }
>>> +
>>> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
>>> +        let iomem = Self::ioremap(resource)?;
>>> +        let devres = Devres::new(device, iomem, GFP_KERNEL)?;
>>> +
>>> +        Ok(devres)
>>> +    }
>>> +}
>>> +
>>> +impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
>>> +    type Target = Io<SIZE>;
>>> +
>>> +    fn deref(&self) -> &Self::Target {
>>> +        &*self.iomem
>>> +    }
>>> +}
>>> +
>>> +/// A generic memory-mapped IO region.
>>> +///
>>> +/// Accesses to the underlying region is checked either at compile time, if the
>>> +/// region's size is known at that point, or at runtime otherwise.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
>>> +/// start of the I/O memory mapped region.
>>> +pub struct IoMem<const SIZE: usize = 0> {
>>> +    io: IoRaw<SIZE>,
>>> +}
>>> +
>>> +impl<const SIZE: usize> IoMem<SIZE> {
>>> +    fn ioremap(resource: &Resource) -> Result<Self> {
>>> +        let size = resource.size();
>>> +        if size == 0 {
>>> +            return Err(EINVAL);
>>> +        }
>>> +
>>> +        let res_start = resource.start();
>>> +
>>> +        // SAFETY:
>>> +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
>>> +        // - `size` is known not to be zero at this point.
>>> +        let addr = unsafe { bindings::ioremap(res_start, size as kernel::ffi::c_ulong) };
>>
>> This does not work for all platforms. Some, like Apple platforms,
>> require ioremap_np(). You should check for the
>> `IORESOURCE_MEM_NONPOSTED` resource flag and use the appropriate
>> function, like this (from an older spin of this code):
>>
>> https://github.com/AsahiLinux/linux/blob/fce34c83f1dca5b10cc2c866fd8832a362de7974/rust/kernel/io_mem.rs#L166C36-L166C70
> 
> Hmm. Is detecting this based on the flag the right approach? On the C
> side, they use different methods and I wonder if we should do the
> same?

ioremap() vs. ioremap_np() exist because the second one was added for
Apple platforms, and we need both because ioremap() is required for
external PCI devices and ioremap_np() for internal SoC devices, but no
driver calls ioremap_np() directly. If you look at drivers/of/address.c
and lib/devres.c, you'll see they do the same thing to decide which one
to call. The code in drivers/base/platform.c hooks up the platform
methods together with devres, and the code in drivers/of/platform.c sets
up OF devices as platform devices with the OF resource code, which sets
the flag as needed.

Therefore, that any driver that uses any of the high-level ioremap
wrappers that go through `struct resource` automatically gets the
ioremap()/ioremap_np() handling (platform drivers get ioremap_np() on
Apple platforms, everything else including PCI drivers get ioremap()).
The Rust side should do the same thing.

~~ Lina

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Alice Ryhl]

On Thu, Jan 30, 2025 at 11:14 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Add a generic iomem abstraction to safely read and write ioremapped
> regions.
>
> The reads and writes are done through IoRaw, and are thus checked either
> at compile-time, if the size of the region is known at that point, or at
> runtime otherwise.
>
> Non-exclusive access to the underlying memory region is made possible to
> cater to cases where overlapped regions are unavoidable.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/io.rs     |   1 +
>  rust/kernel/io/mem.rs | 125 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 rust/kernel/io/mem.rs
>
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 566d8b177e01..9ce3482b5ecd 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,7 @@
>  use crate::error::{code::EINVAL, Result};
>  use crate::{bindings, build_assert};
>
> +pub mod mem;
>  pub mod resource;
>
>  /// Raw representation of an MMIO region.
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> new file mode 100644
> index 000000000000..f87433ed858e
> --- /dev/null
> +++ b/rust/kernel/io/mem.rs
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic memory-mapped IO.
> +
> +use core::ops::Deref;
> +
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::io::resource::Region;
> +use crate::io::resource::Resource;
> +use crate::io::Io;
> +use crate::io::IoRaw;
> +use crate::prelude::*;
> +
> +/// An exclusive memory-mapped IO region.
> +///
> +/// # Invariants
> +///
> +/// - ExclusiveIoMem has exclusive access to the underlying `iomem`.
> +pub struct ExclusiveIoMem<const SIZE: usize> {
> +    /// The region abstraction. This represents exclusive access to the
> +    /// range represented by the underlying `iomem`.
> +    ///
> +    /// It's placed first to ensure that the region is released before it is
> +    /// unmapped as a result of the drop order.
> +    #[allow(dead_code)]
> +    region: Region,

Instead of #[allow(dead_code)], I would prefix the name with an
underscore and comment that you hold on to it only for ownership.

> +    /// The underlying `IoMem` instance.
> +    iomem: IoMem<SIZE>,
> +}
> +
> +impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
> +    /// Creates a new `ExclusiveIoMem` instance.
> +    pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
> +        let iomem = IoMem::ioremap(resource)?;
> +
> +        let start = resource.start();
> +        let size = resource.size();
> +        let name = resource.name();
> +
> +        let region = resource
> +            .request_mem_region(start, size, name)
> +            .ok_or(EBUSY)?;
> +
> +        let iomem = ExclusiveIoMem { iomem, region };
> +
> +        Ok(iomem)
> +    }
> +
> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> +        let iomem = Self::ioremap(resource)?;
> +        let devres = Devres::new(device, iomem, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &*self.iomem
> +    }
> +}
> +
> +/// A generic memory-mapped IO region.
> +///
> +/// Accesses to the underlying region is checked either at compile time, if the
> +/// region's size is known at that point, or at runtime otherwise.
> +///
> +/// # Invariants
> +///
> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the

typo: instance

> +/// start of the I/O memory mapped region.
> +pub struct IoMem<const SIZE: usize = 0> {
> +    io: IoRaw<SIZE>,
> +}
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    fn ioremap(resource: &Resource) -> Result<Self> {
> +        let size = resource.size();
> +        if size == 0 {
> +            return Err(EINVAL);
> +        }
> +
> +        let res_start = resource.start();
> +
> +        // SAFETY:
> +        // - `res_start` and `size` are read from a presumably valid `struct resource`.
> +        // - `size` is known not to be zero at this point.
> +        let addr = unsafe { bindings::ioremap(res_start, size as kernel::ffi::c_ulong) };

Once you rebase on v6.14-rc1 this cast is no longer necessary.

> +        if addr.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        let io = IoRaw::new(addr as usize, size as usize)?;

Ditto here. Though isn't usize the wrong type for `addr`? Shouldn't
you have a type alias for phys_addr_t?

> +        let io = IoMem { io };
> +
> +        Ok(io)
> +    }
> +
> +    /// Creates a new `IoMem` instance.
> +    pub(crate) fn new(resource: &Resource, device: &Device) -> Result<Devres<Self>> {
> +        let io = Self::ioremap(resource)?;
> +        let devres = Devres::new(device, io, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.io.addr() as *mut core::ffi::c_void) }

Hmm ... void pointer? I wonder whether IoRaw should store this as a
pointer instead of an integer?

Alice

> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: Safe as by the invariant of `IoMem`.
> +        unsafe { Io::from_raw(&self.io) }
> +    }
> +}
> --
> 2.48.0
>

[PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Guangbo Cui]

> +/// A generic memory-mapped IO region.
> +///
> +/// Accesses to the underlying region is checked either at compile time, if the
> +/// region's size is known at that point, or at runtime otherwise.
> +///
> +/// # Invariants
> +///
> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> +/// start of the I/O memory mapped region.
> +pub struct IoMem<const SIZE: usize = 0> {
> +    io: IoRaw<SIZE>,
> +}

Compile-time checks are only possible when CONFIG_RUST_BUILD_ASSERT_ALLOW=y.
Otherwise, using compile-time check APIs of Io will cause a modpost error 
because the rust_build_error symbol is not exported. Details at the issue[1].

Maybe Io should expose compile-time check APIs only when CONFIG_RUST_BUILD_ASSERT_ALLOW=y?
The expectation is that a build error should occur when calling `Io::readX` and
`Io::writeX` due to a boundary check failure, rather than because the
`rust_build_error` symbol is not exported.

Link: https://github.com/Rust-for-Linux/linux/issues/1141 [1]

Best regards,
Guangbo Cui

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Alice Ryhl]

On Wed, Feb 5, 2025 at 3:56 PM Guangbo Cui <2407018371@qq.com> wrote:
>
> > +/// A generic memory-mapped IO region.
> > +///
> > +/// Accesses to the underlying region is checked either at compile time, if the
> > +/// region's size is known at that point, or at runtime otherwise.
> > +///
> > +/// # Invariants
> > +///
> > +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> > +/// start of the I/O memory mapped region.
> > +pub struct IoMem<const SIZE: usize = 0> {
> > +    io: IoRaw<SIZE>,
> > +}
>
> Compile-time checks are only possible when CONFIG_RUST_BUILD_ASSERT_ALLOW=y.
> Otherwise, using compile-time check APIs of Io will cause a modpost error
> because the rust_build_error symbol is not exported. Details at the issue[1].
>
> Maybe Io should expose compile-time check APIs only when CONFIG_RUST_BUILD_ASSERT_ALLOW=y?
> The expectation is that a build error should occur when calling `Io::readX` and
> `Io::writeX` due to a boundary check failure, rather than because the
> `rust_build_error` symbol is not exported.
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1141 [1]

This compilation failure is correct. You're trying to use writeb even
though the size is not known at compile time. You should use
try_writeb instead.

Alice

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Miguel Ojeda]

On Thu, Feb 6, 2025 at 4:43 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This compilation failure is correct. You're trying to use writeb even
> though the size is not known at compile time. You should use
> try_writeb instead.

Agreed, in the issue I replied that `ioremap_resource()` returns a
runtime-checked `IoMem`. Perhaps the `rust_build_error` part is being
confusing here.

Cheers,
Miguel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Guangbo Cui]

On Thu, Feb 06, 2025 at 04:43:17PM +0100, Alice Ryhl wrote:
> On Wed, Feb 5, 2025 at 3:56 PM Guangbo Cui <2407018371@qq.com> wrote:
> >
> > > +/// A generic memory-mapped IO region.
> > > +///
> > > +/// Accesses to the underlying region is checked either at compile time, if the
> > > +/// region's size is known at that point, or at runtime otherwise.
> > > +///
> > > +/// # Invariants
> > > +///
> > > +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> > > +/// start of the I/O memory mapped region.
> > > +pub struct IoMem<const SIZE: usize = 0> {
> > > +    io: IoRaw<SIZE>,
> > > +}
> >
> > Compile-time checks are only possible when CONFIG_RUST_BUILD_ASSERT_ALLOW=y.
> > Otherwise, using compile-time check APIs of Io will cause a modpost error
> > because the rust_build_error symbol is not exported. Details at the issue[1].
> >
> > Maybe Io should expose compile-time check APIs only when CONFIG_RUST_BUILD_ASSERT_ALLOW=y?
> > The expectation is that a build error should occur when calling `Io::readX` and
> > `Io::writeX` due to a boundary check failure, rather than because the
> > `rust_build_error` symbol is not exported.
> >
> > Link: https://github.com/Rust-for-Linux/linux/issues/1141 [1]
> 
> This compilation failure is correct. You're trying to use writeb even
> though the size is not known at compile time. You should use
> try_writeb instead.

With CONFIG_RUST_BUILD_ASSERT_ALLOW=y enabled, this compilation succeeds.
Even if the size is determined at compile time, the compilation will still fail
if CONFIG_RUST_BUILD_ASSERT_ALLOW is not enabled.

If I make any mistakes, please correct me. Thanks!

Best regards,
Guangbo Cui

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Miguel Ojeda]

On Thu, Feb 6, 2025 at 4:59 PM Guangbo Cui <2407018371@qq.com> wrote:
>
> With CONFIG_RUST_BUILD_ASSERT_ALLOW=y enabled, this compilation succeeds.

Yes, that is expected too (but note that the config option is there
just in case -- it should not happen that it is needed in normal
builds).

> Even if the size is determined at compile time, the compilation will still fail
> if CONFIG_RUST_BUILD_ASSERT_ALLOW is not enabled.

Yes, that is expected -- the idea is that you cannot make the mistake
of calling those.

I think you are suggesting only exposing the methods in the case where
calling them would work? That would be great if a design allows for
it, of course.

By the way, Daniel, in patch 3/3 there is this comment:

+    ///     // Unlike `ioremap_resource_sized`, here the size of the
memory region
+    ///     // is not known at compile time, so only the `try_read*`
and `try_write*`
+    ///     // family of functions are exposed, leading to runtime
checks on every
+    ///     // access.

Is the "only ... are exposed" correct? i.e. are they exposed? / is
this potentially confusing?

Thanks!

Cheers,
Miguel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Danilo Krummrich]

On Fri, Feb 07, 2025 at 12:40:14AM +0800, 崔光博 wrote:
> 
> 
> > 2025年2月7日 00:11，Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> 写道：
> > 
> > On Thu, Feb 6, 2025 at 4:59 PM Guangbo Cui <2407018371@qq.com> wrote:
> >> 
> >> With CONFIG_RUST_BUILD_ASSERT_ALLOW=y enabled, this compilation succeeds.
> > 
> > Yes, that is expected too (but note that the config option is there
> > just in case -- it should not happen that it is needed in normal
> > builds).
> > 
> >> Even if the size is determined at compile time, the compilation will still fail
> >> if CONFIG_RUST_BUILD_ASSERT_ALLOW is not enabled.
> > 
> > Yes, that is expected -- the idea is that you cannot make the mistake
> > of calling those.
> > 
> > I think you are suggesting only exposing the methods in the case where
> > calling them would work? That would be great if a design allows for
> > it, of course.
> 
> Yes, if the methods could not work, we should not expose them.
> 
> > By the way, Daniel, in patch 3/3 there is this comment:
> > 
> > +    ///     // Unlike `ioremap_resource_sized`, here the size of the
> > memory region
> > +    ///     // is not known at compile time, so only the `try_read*`
> > and `try_write*`
> > +    ///     // family of functions are exposed, leading to runtime
> > checks on every
> > +    ///     // access.
> > 
> > Is the "only ... are exposed" correct? i.e. are they exposed? / is
> > this potentially confusing?
> 
> They are exposed. If size is not known at compile time, calling the `read`
> and `write` will never compile failed. Example:

That's two different things here. Miguel questions whether the comment is
correct. And I think it's not, they are indeed exposed.

> 
> ```C
> let raw_io: IoRaw<0> = IoRaw::new(0, 8)?;
> let io = unsafe { Io::from_raw(&raw_io) };
> io.writeb(0xff, 0xffff);
> ```
> If I make any mistakes, please correct me. Thanks!

This behavior is on purpose.

IoRaw::new() is equivalent to IoRaw::<0>::new(), which means that you set the
compile time validated size of the I/O region to zero.

Hence, calling writeb() fails, because every operation exeeds the boundary of
zero.

In your case the runtime boundary is 8, hence the following calls do succeed.

try_readb(0);
try_readb(7);

Whereas the following would fail on runtime.

try_readb(8);

> 
> Best regards,
> Guangbo Cui
> 

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Daniel Almeida]

Sorry, didn’t find time to reply yesterday,


> 
> That's two different things here. Miguel questions whether the comment is
> correct. And I think it's not, they are indeed exposed.
> 

Yes, I said “not exposed”, but in truth I meant “you can’t use it”, as shown by
the build_error being triggered.

My bad. I will fix the wording so that it’s clear in the next iteration.

— Daniel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Daniel Almeida]

Btw, Miguel & others,

IMHO, I think we should write a comment about this somewhere in the docs.

When I first came across this issue myself, it took me a while to understand that
the build_error was actually triggering.

That’s because the result is:

```
ERROR: modpost: "rust_build_error" [rust_platform_uio_driver.ko] undefined!
```

When a symbol is undefined, someone would be within their rights to assume that
something is broken in some KConfig somewhere, like this person did. It specifically
doesn’t tell them that the problem is their own code triggering a build_error because
they are misusing an API.

I know that we can’t really provide a message through build_error itself, hence my
suggestion about the docs.

I can send a patch if you agree, it will prevent this confusion from coming up in the
future.

— Daniel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Miguel Ojeda]

On Thu, Feb 6, 2025 at 4:57 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> I know that we can’t really provide a message through build_error itself, hence my
> suggestion about the docs.
>
> I can send a patch if you agree, it will prevent this confusion from coming up in the
> future.

We have some docs in the Rust items and the crate -- perhaps those can
be improved (e.g. showing an example of the error that one would get)
or rearranged and then a note referencing those from Doc/rust/?

That would definitely help, at least those that read the docs (but not
everyone does, and even those that do may forget details, so it will
not fully prevent the issue).

Ideally we would get a better way to do this, perhaps assisted by the
compiler(s)/linker (`*_assert` in
https://github.com/Rust-for-Linux/linux/issues/354).

Thanks!

Cheers,
Miguel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Joel Fernandes]

On Thu, Feb 06, 2025 at 12:57:30PM -0300, Daniel Almeida wrote:
> Btw, Miguel & others,
> 
> IMHO, I think we should write a comment about this somewhere in the docs.
> 
> When I first came across this issue myself, it took me a while to understand that
> the build_error was actually triggering.
> 
> That’s because the result is:
> 
> ```
> ERROR: modpost: "rust_build_error" [rust_platform_uio_driver.ko] undefined!
> ```
> 
> When a symbol is undefined, someone would be within their rights to assume that
> something is broken in some KConfig somewhere, like this person did. It specifically
> doesn’t tell them that the problem is their own code triggering a build_error because
> they are misusing an API.
> 
> I know that we can’t really provide a message through build_error itself, hence my
> suggestion about the docs.
> 
> I can send a patch if you agree, it will prevent this confusion from coming up in the
> future.

Interesting, I just ran into this. I am writing function as follows that
reads bar0 in the nova driver, however not having the "if current_len + i"
causes the same issue:

ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!

which did not help much about what the issue really is. I had to figure it
out through tedious trial and error. Also what is the reason for this, the
compiler is doing some checks in with_bar? Looking at with_bar
implementation, I could not see any. Also enabling
CONFIG_RUST_BUILD_ASSERT_ALLOW did not show more menaingful messages. Thanks
for taking a look:

    pub(crate) fn read_more(&mut self, bytes: u32) -> Result {
        with_bar!(self.bar0, |bar0| {
            // Get current length
            let current_len = self.data.len();

            // Read ROM data bytes push directly to vector
            for i in 0..bytes as usize {

		// This check fixes:
		// ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
                if current_len + i >= 10000000 {
                    return Err(EINVAL);
                }

                // Read a byte from the VBIOS ROM and push it to the data vector
                let rom_addr = ROM_OFFSET + current_len + i;
                let byte = bar0.readb(rom_addr);
                self.data.push(byte, GFP_KERNEL)?;
            }

            Ok(())
        })?
    }

thanks,

 - Joel

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Danilo Krummrich]

On Tue, Apr 01, 2025 at 11:57:35AM -0400, Joel Fernandes wrote:
> On Thu, Feb 06, 2025 at 12:57:30PM -0300, Daniel Almeida wrote:
> > Btw, Miguel & others,
> > 
> > IMHO, I think we should write a comment about this somewhere in the docs.
> > 
> > When I first came across this issue myself, it took me a while to understand that
> > the build_error was actually triggering.
> > 
> > That’s because the result is:
> > 
> > ```
> > ERROR: modpost: "rust_build_error" [rust_platform_uio_driver.ko] undefined!
> > ```
> > 
> > When a symbol is undefined, someone would be within their rights to assume that
> > something is broken in some KConfig somewhere, like this person did. It specifically
> > doesn’t tell them that the problem is their own code triggering a build_error because
> > they are misusing an API.
> > 
> > I know that we can’t really provide a message through build_error itself, hence my
> > suggestion about the docs.
> > 
> > I can send a patch if you agree, it will prevent this confusion from coming up in the
> > future.
> 
> Interesting, I just ran into this. I am writing function as follows that
> reads bar0 in the nova driver, however not having the "if current_len + i"
> causes the same issue:
> 
> ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
> 
> which did not help much about what the issue really is. I had to figure it
> out through tedious trial and error. Also what is the reason for this, the
> compiler is doing some checks in with_bar? Looking at with_bar
> implementation, I could not see any. Also enabling
> CONFIG_RUST_BUILD_ASSERT_ALLOW did not show more menaingful messages. Thanks
> for taking a look:
> 
>     pub(crate) fn read_more(&mut self, bytes: u32) -> Result {
>         with_bar!(self.bar0, |bar0| {
>             // Get current length
>             let current_len = self.data.len();
> 
>             // Read ROM data bytes push directly to vector
>             for i in 0..bytes as usize {
> 
> 		// This check fixes:
> 		// ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
>                 if current_len + i >= 10000000 {
>                     return Err(EINVAL);
>                 }
> 
>                 // Read a byte from the VBIOS ROM and push it to the data vector
>                 let rom_addr = ROM_OFFSET + current_len + i;
>                 let byte = bar0.readb(rom_addr);

The problem here is that the compiler can't figure out the offset (rom_addr) on
compile time, thus it fails to compile.

The non-try variants of I/O accessors are only supposed to work if the compiler
can figure out the offset on compile time, such that it can be checked against
the expected bar size (not the actual bar size). The expected bar size is what
the driver puts in as a const generic when calling
pci::Device::iomap_region_sized(). This is what makes the non-try variants
infallible.

If the offset is not known at compile time, bar0.try_readb() can be used
instead, which performs a runtime check against the actual bar size instead.

Your above check seems to be enough to let the compiler figure out that
ROM_OFFSET + current_len + i can never be out of bounds for bar0.

>                 self.data.push(byte, GFP_KERNEL)?;
>             }
> 
>             Ok(())
>         })?
>     }
> 
> thanks,
> 
>  - Joel
> 

Re: [PATCH v6 2/3] rust: io: mem: add a generic iomem abstraction

[Joel Fernandes]

On 4/1/2025 12:44 PM, Danilo Krummrich wrote:
> On Tue, Apr 01, 2025 at 11:57:35AM -0400, Joel Fernandes wrote:
>> On Thu, Feb 06, 2025 at 12:57:30PM -0300, Daniel Almeida wrote:
>>> Btw, Miguel & others,
>>>
>>> IMHO, I think we should write a comment about this somewhere in the docs.
>>>
>>> When I first came across this issue myself, it took me a while to understand that
>>> the build_error was actually triggering.
>>>
>>> That’s because the result is:
>>>
>>> ```
>>> ERROR: modpost: "rust_build_error" [rust_platform_uio_driver.ko] undefined!
>>> ```
>>>
>>> When a symbol is undefined, someone would be within their rights to assume that
>>> something is broken in some KConfig somewhere, like this person did. It specifically
>>> doesn’t tell them that the problem is their own code triggering a build_error because
>>> they are misusing an API.
>>>
>>> I know that we can’t really provide a message through build_error itself, hence my
>>> suggestion about the docs.
>>>
>>> I can send a patch if you agree, it will prevent this confusion from coming up in the
>>> future.
>>
>> Interesting, I just ran into this. I am writing function as follows that
>> reads bar0 in the nova driver, however not having the "if current_len + i"
>> causes the same issue:
>>
>> ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
>>
>> which did not help much about what the issue really is. I had to figure it
>> out through tedious trial and error. Also what is the reason for this, the
>> compiler is doing some checks in with_bar? Looking at with_bar
>> implementation, I could not see any. Also enabling
>> CONFIG_RUST_BUILD_ASSERT_ALLOW did not show more menaingful messages. Thanks
>> for taking a look:
>>
>>     pub(crate) fn read_more(&mut self, bytes: u32) -> Result {
>>         with_bar!(self.bar0, |bar0| {
>>             // Get current length
>>             let current_len = self.data.len();
>>
>>             // Read ROM data bytes push directly to vector
>>             for i in 0..bytes as usize {
>>
>> 		// This check fixes:
>> 		// ERROR: modpost: "rust_build_error" [nova_core.ko] undefined!
>>                 if current_len + i >= 10000000 {
>>                     return Err(EINVAL);
>>                 }
>>
>>                 // Read a byte from the VBIOS ROM and push it to the data vector
>>                 let rom_addr = ROM_OFFSET + current_len + i;
>>                 let byte = bar0.readb(rom_addr);
> 
> The problem here is that the compiler can't figure out the offset (rom_addr) on
> compile time, thus it fails to compile.
> 
> The non-try variants of I/O accessors are only supposed to work if the compiler
> can figure out the offset on compile time, such that it can be checked against
> the expected bar size (not the actual bar size). The expected bar size is what
> the driver puts in as a const generic when calling
> pci::Device::iomap_region_sized(). This is what makes the non-try variants
> infallible.
> 
> If the offset is not known at compile time, bar0.try_readb() can be used
> instead, which performs a runtime check against the actual bar size instead.
> 
> Your above check seems to be enough to let the compiler figure out that
> ROM_OFFSET + current_len + i can never be out of bounds for bar0.

Thanks a lot for the quick reply. I tried the try_readb() variant and it works
now. I think instead of me doing a runtime check to satisfy the compiler, it may
be better for me rely on the try accessor's runtime checking so I will stick to
that.

Thanks again!

 - Joel



> 
>>                 self.data.push(byte, GFP_KERNEL)?;
>>             }
>>
>>             Ok(())
>>         })?
>>     }
>>
>> thanks,
>>
>>  - Joel
>>

[PATCH v6 3/3] rust: platform: allow ioremap of platform resources

[Daniel Almeida]

The preceding patches added support for resources, and for a general
IoMem abstraction, but thus far there is no way to access said IoMem
from drivers, as its creation is unsafe and depends on a resource that
must be acquired from some device first.

Now, allow the ioremap of platform resources themselves, thereby making
the IoMem available to platform drivers.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/platform.rs | 123 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 50e6b0421813..0ab10d2dfd5e 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,8 +5,14 @@
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
 use crate::{
-    bindings, container_of, device, driver,
+    bindings, container_of, device,
+    devres::Devres,
+    driver,
     error::{to_result, Result},
+    io::{
+        mem::{ExclusiveIoMem, IoMem},
+        resource::Resource,
+    },
     of,
     prelude::*,
     str::CStr,
@@ -191,6 +197,121 @@ fn as_raw(&self) -> *mut bindings::platform_device {
         // embedded in `struct platform_device`.
         unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
     }
+
+    /// Maps a platform resource through ioremap() where the size is known at
+    /// compile time.
+    ///
+    /// # Examples
+    ///
+    /// ```no_run
+    /// use kernel::{bindings, c_str, platform};
+    ///
+    /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
+    ///     let offset = 0; // Some offset.
+    ///
+    ///     // If the size is known at compile time, use `ioremap_resource_sized`.
+    ///     // No runtime checks will apply when reading and writing.
+    ///     let resource = pdev.resource(0).ok_or(ENODEV)?;
+    ///     let iomem = pdev.ioremap_resource_sized::<42>(&resource)?;
+    ///
+    ///     // Read and write a 32-bit value at `offset`. Calling `try_access()` on
+    ///     // the `Devres` makes sure that the resource is still valid.
+    ///     let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
+    ///
+    ///     iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
+    ///
+    ///     # Ok::<(), Error>(())
+    /// }
+    /// ```
+    pub fn ioremap_resource_sized<const SIZE: usize>(
+        &self,
+        resource: &Resource,
+    ) -> Result<Devres<IoMem<SIZE>>> {
+        IoMem::new(resource, self.as_ref())
+    }
+
+    /// Same as [`Self::ioremap_resource_sized`] but with exclusive access to the
+    /// underlying region.
+    pub fn ioremap_resource_exclusive_sized<const SIZE: usize>(
+        &self,
+        resource: &Resource,
+    ) -> Result<Devres<ExclusiveIoMem<SIZE>>> {
+        ExclusiveIoMem::new(resource, self.as_ref())
+    }
+
+    /// Maps a platform resource through ioremap().
+    ///
+    /// # Examples
+    ///
+    /// ```no_run
+    /// # use kernel::{bindings, c_str, platform};
+    ///
+    /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
+    ///     let offset = 0; // Some offset.
+    ///
+    ///     // Unlike `ioremap_resource_sized`, here the size of the memory region
+    ///     // is not known at compile time, so only the `try_read*` and `try_write*`
+    ///     // family of functions are exposed, leading to runtime checks on every
+    ///     // access.
+    ///     let resource = pdev.resource(0).ok_or(ENODEV)?;
+    ///     let iomem = pdev.ioremap_resource(&resource)?;
+    ///
+    ///     let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
+    ///
+    ///     iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
+    ///
+    ///     # Ok::<(), Error>(())
+    /// }
+    /// ```
+    pub fn ioremap_resource(&self, resource: &Resource) -> Result<Devres<IoMem<0>>> {
+        self.ioremap_resource_sized::<0>(resource)
+    }
+
+    /// Same as [`Self::ioremap_resource`] but with exclusive access to the underlying
+    /// region.
+    pub fn ioremap_resource_exclusive(
+        &self,
+        resource: &Resource,
+    ) -> Result<Devres<ExclusiveIoMem<0>>> {
+        self.ioremap_resource_exclusive_sized::<0>(resource)
+    }
+
+    /// Returns the resource at `index`, if any.
+    pub fn resource(&self, index: u32) -> Option<&Resource> {
+        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct platform_device`.
+        let resource = unsafe {
+            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, index)
+        };
+
+        if resource.is_null() {
+            return None;
+        }
+
+        // SAFETY: `resource` is a valid pointer to a `struct resource` as
+        // returned by `platform_get_resource`.
+        Some(unsafe { Resource::from_ptr(resource) })
+    }
+
+    /// Returns the resource with a given `name`, if any.
+    pub fn resource_by_name(&self, name: &CStr) -> Option<&Resource> {
+        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct
+        // platform_device` and `name` points to a valid C string.
+        let resource = unsafe {
+            bindings::platform_get_resource_byname(
+                self.as_raw(),
+                bindings::IORESOURCE_MEM,
+                name.as_char_ptr(),
+            )
+        };
+
+        if resource.is_null() {
+            return None;
+        }
+
+        // SAFETY: `resource` is a valid pointer to a `struct resource` as
+        // returned by `platform_get_resource`.
+        Some(unsafe { Resource::from_ptr(resource) })
+    }
 }
 
 impl AsRef<device::Device> for Device {
-- 
2.48.0

Re: [PATCH v6 3/3] rust: platform: allow ioremap of platform resources

[Daniel Sedlak]

Hi,

On 1/30/25 11:05 PM, Daniel Almeida wrote:
> +
> +    /// Same as [`Self::ioremap_resource`] but with exclusive access to the underlying
> +    /// region.
> +    pub fn ioremap_resource_exclusive(
> +        &self,
> +        resource: &Resource,
> +    ) -> Result<Devres<ExclusiveIoMem<0>>> {
> +        self.ioremap_resource_exclusive_sized::<0>(resource)
> +    }
> +
> +    /// Returns the resource at `index`, if any.
> +    pub fn resource(&self, index: u32) -> Option<&Resource> {

Maybe we could utilize the Index [1] trait to make the API more rust~ish?
Or at least name it `resource_by_index` to keep consistency with 
`resource_by_name`?

Link: https://doc.rust-lang.org/core/ops/trait.Index.html [1]

> +        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct platform_device`.
> +        let resource = unsafe {
> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, index)
> +        };
> +
> +        if resource.is_null() {
> +            return None;
> +        }
> +
> +        // SAFETY: `resource` is a valid pointer to a `struct resource` as
> +        // returned by `platform_get_resource`.
> +        Some(unsafe { Resource::from_ptr(resource) })
> +    }
> +
> +    /// Returns the resource with a given `name`, if any.
> +    pub fn resource_by_name(&self, name: &CStr) -> Option<&Resource> {
> +        // SAFETY: `self.as_raw()` returns a valid pointer to a `struct
> +        // platform_device` and `name` points to a valid C string.
> +        let resource = unsafe {
> +            bindings::platform_get_resource_byname(
> +                self.as_raw(),
> +                bindings::IORESOURCE_MEM,
> +                name.as_char_ptr(),
> +            )
> +        };
> +
> +        if resource.is_null() {
> +            return None;
> +        }
> +
> +        // SAFETY: `resource` is a valid pointer to a `struct resource` as
> +        // returned by `platform_get_resource`.
> +        Some(unsafe { Resource::from_ptr(resource) })
> +    }
>   }
>   
>   impl AsRef<device::Device> for Device {
	Daniel

Re: [PATCH v6 3/3] rust: platform: allow ioremap of platform resources

[Alice Ryhl]

On Fri, Jan 31, 2025 at 11:19 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Hi,
>
> On 1/30/25 11:05 PM, Daniel Almeida wrote:
> > +
> > +    /// Same as [`Self::ioremap_resource`] but with exclusive access to the underlying
> > +    /// region.
> > +    pub fn ioremap_resource_exclusive(
> > +        &self,
> > +        resource: &Resource,
> > +    ) -> Result<Devres<ExclusiveIoMem<0>>> {
> > +        self.ioremap_resource_exclusive_sized::<0>(resource)
> > +    }
> > +
> > +    /// Returns the resource at `index`, if any.
> > +    pub fn resource(&self, index: u32) -> Option<&Resource> {
>
> Maybe we could utilize the Index [1] trait to make the API more rust~ish?
> Or at least name it `resource_by_index` to keep consistency with
> `resource_by_name`?
>
> Link: https://doc.rust-lang.org/core/ops/trait.Index.html [1]

Using the index trait would result in syntax like device[7] to get the
resource at index 7. I think that would be unclear because it's not
obvious that devices should behave like an array of resources.

Alice