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

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

[Daniel Almeida]

Changes in v12:
- Fixed the typos that Miguel pointed out in resource.rs
- Fixed a typo where thread was written as thead
- Removed ioport_resource() and iomem_resource() (Danilo)
- Created IoRequest<'a> and gave it an unsafe constructor (Danilo)
- Moved all the logic to map resources from platform.rs to IoRequest.
- Dropped the last patch as a result of the above.
- Link to v11: https://lore.kernel.org/r/20250701-topics-tyr-platform_iomem-v11-0-6cd5d5061151@collabora.com

Changes in v11:
- Rebased on top of driver-core-next (to get the latest Devres changes)
- Changed the order between requesting the resource and mapping it
  (Danilo)
- Link to v10: https://lore.kernel.org/r/20250623-topics-tyr-platform_iomem-v10-0-ed860a562940@collabora.com

Changes in v10:
- Rebased on driver-core-next
- Fixed examples (they were still using try_access())
- Removed map_err() from the examples, as it was not needed.
- Added a pub use for Resource in io.rs
- Reworked the platform code to make use of the pub use above
- Link to v9: https://lore.kernel.org/r/20250603-topics-tyr-platform_iomem-v9-0-a27e04157e3e@collabora.com

Changes in v9:
- Rebased on top of nova-next (for Devres::access())
- Reworked the documentation to add more markdown.
- Converted to &raw mut instead of addr_of_mut!()
- Renamed 'from_ptr' to 'as_ref' for consistency
- Changed the IoMem examples to use the signature for probe()
- Changed resource() to resource_by_index(). It's a better fit given
  the existance of resource_by_name().
- Created a separate patch for the resource accessors above.
- Moved the accessors into the generic impl block, so they work with all
  Device contexts.
- Take Device<Bound> where applicable
- Renamed "ioremap_*" to "iomap_*", in order to be consistent with the code
  in pci.rs
- Switched to Devres::access()
- Link to v8: https://lore.kernel.org/r/20250509-topics-tyr-platform_iomem-v8-0-e9f1725a40da@collabora.com

rust: platform: add Io support

Changes in v8:
- Rebased on driver-core-next
- Opted to wait for 'rust/revocable: add try_with() convenience method' to
  land instead of using the suggested closure (let me know if we should
  just switch to the closure anyways)
- Cc'd more people
- Link to v7: https://lore.kernel.org/r/20250318-topics-tyr-platform_iomem-v7-0-7438691d9ef7@collabora.com

Changes in v7:

- Rebased on top of rust-next
- Fixed a few Clippy lints
- Fixed typos (Thanks Daniel!)
- "struct Flags" now contains a usize (thanks Daniel)
- Fixed "Doc list without indentation" warning (thanks, Guangbo)

Thanks, Fiona {
- Removed RequestFn, as all functions simply used request_region and RequestFn
  had issues. Only request_region() is exposed now.
- Gated iomem_resource on CONFIG_HAS_IOMEM
- Require that the name argument be 'static
}

- Correctly check for IORESOURCE_MEM_NONPOSTED. We now call ioremap_np if that
  is set (thanks, Lina!)
- Remove #[dead_code] attribute from ExclusiveIoMem::region.

Changes in v6:

- Added Fiona as co-developer in the first patch, as I merged part of her 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: add resource accessors

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/io.c               |  41 ++++++
 rust/kernel/io.rs               |   5 +
 rust/kernel/io/mem.rs           | 274 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/io/resource.rs      | 209 ++++++++++++++++++++++++++++++
 rust/kernel/platform.rs         |  60 ++++++++-
 6 files changed, 589 insertions(+), 1 deletion(-)
---
base-commit: f5d3ef25d238901a76fe0277787afa44f7714739
change-id: 20250318-topics-tyr-platform_iomem-1710a177e1df

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v12 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               |   4 +
 rust/kernel/io/resource.rs      | 209 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 250 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 7e8f2285064797d5bbac5583990ff823b76c6bdc..5f795e60e889b9fc887013743c81b1cf92a52adb 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -53,6 +53,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 15ea187c5466256effd07efe6f6995a1dd95a967..404776cf6717c8570c7600a24712ce6e4623d3c6 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, 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 72d80a6f131e3e826ecd9d2c3bcf54e89aa60cc3..7b70d5b5477e57d6d0f24bcd26bd8b0071721bc0 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,10 @@
 use crate::error::{code::EINVAL, Result};
 use crate::{bindings, build_assert};
 
+pub mod resource;
+
+pub use resource::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 0000000000000000000000000000000000000000..a8ad04b1abf8e46928ed98564e64a07180250024
--- /dev/null
+++ b/rust/kernel/io/resource.rs
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for [system
+//! resources](https://docs.kernel.org/core-api/kernel-api.html#resources-management).
+//!
+//! 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;
+
+/// Resource Size type.
+///
+/// This is a type alias to either `u32` or `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 either `u32` or `u64` 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
+///   `bindings::__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::as_ref(self.0.as_ptr()) }
+    }
+}
+
+impl Drop for Region {
+    fn drop(&mut self) {
+        // SAFETY: Safe as per the invariant of `Region`.
+        let res = unsafe { Resource::as_ref(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 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 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 as_ref<'a>(ptr: *mut bindings::resource) -> &'a Self {
+        // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Requests a resource region.
+    ///
+    /// Exclusive access will be given and the region will be marked as busy.
+    /// Further calls to [`Self::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: &'static CStr,
+        flags: Flags,
+    ) -> Option<Region> {
+        // SAFETY: Safe as per the invariant of `Resource`.
+        let region = unsafe {
+            bindings::__request_region(
+                self.0.get(),
+                start,
+                size,
+                name.as_char_ptr(),
+                flags.0 as i32,
+            )
+        };
+
+        Some(Region(NonNull::new(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) -> &'static 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 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 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(usize);
+
+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 usize);
+
+    /// Resource is software muxed.
+    pub const IORESOURCE_MUXED: Flags = Flags(bindings::IORESOURCE_MUXED as usize);
+
+    /// Resource represents a memory region.
+    pub const IORESOURCE_MEM: Flags = Flags(bindings::IORESOURCE_MEM as usize);
+
+    /// Resource represents a memory region that must be ioremaped using `ioremap_np`.
+    pub const IORESOURCE_MEM_NONPOSTED: Flags = Flags(bindings::IORESOURCE_MEM_NONPOSTED as usize);
+}

-- 
2.50.0

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

[Daniel Almeida]

Add a generic iomem abstraction to safely read and write ioremapped
regions. This abstraction requires a previously acquired IoRequest
instance. This makes it so that both the resource and the device match,
or, in other words, that the resource is indeed a valid resource for a
given bound device.

A subsequent patch will add the ability to retrieve IoRequest instances
from platform devices.

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/helpers/io.c     |   5 +
 rust/kernel/io.rs     |   1 +
 rust/kernel/io/mem.rs | 274 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)

diff --git a/rust/helpers/io.c b/rust/helpers/io.c
index 404776cf6717c8570c7600a24712ce6e4623d3c6..c475913c69e647b1042e8e7d66b9148d892947a1 100644
--- a/rust/helpers/io.c
+++ b/rust/helpers/io.c
@@ -8,6 +8,11 @@ void __iomem *rust_helper_ioremap(phys_addr_t offset, size_t size)
 	return ioremap(offset, size);
 }
 
+void __iomem *rust_helper_ioremap_np(phys_addr_t offset, size_t size)
+{
+	return ioremap_np(offset, size);
+}
+
 void rust_helper_iounmap(void __iomem *addr)
 {
 	iounmap(addr);
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 7b70d5b5477e57d6d0f24bcd26bd8b0071721bc0..b7fc759f8b5d3c3ac6f33f5a66e9f619c58b7405 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;
 
 pub use resource::Resource;
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b047b9a73ae535bb221b2aea48d875a66a3fbc13
--- /dev/null
+++ b/rust/kernel/io/mem.rs
@@ -0,0 +1,274 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic memory-mapped IO.
+
+use core::ops::Deref;
+
+use crate::device::Bound;
+use crate::device::Device;
+use crate::devres::Devres;
+use crate::io;
+use crate::io::resource::Region;
+use crate::io::resource::Resource;
+use crate::io::Io;
+use crate::io::IoRaw;
+use crate::prelude::*;
+
+/// An IO request for a specific device and resource.
+pub struct IoRequest<'a> {
+    device: &'a Device<Bound>,
+    resource: &'a Resource,
+}
+
+impl<'a> IoRequest<'a> {
+    /// Creates a new [`IoRequest`] instance.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `resource` is valid for `device` during the
+    /// lifetime `'a`.
+    pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> Self {
+        IoRequest { device, resource }
+    }
+
+    /// Maps an [`IoRequest`] where the size is known at compile time.
+    ///
+    /// This uses the
+    /// [`ioremap()`](https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device)
+    /// C API.
+    ///
+    /// # Examples
+    ///
+    /// The following example uses a [`platform::Device`] for illustration
+    /// purposes.
+    ///
+    /// ```no_run
+    /// # use kernel::{bindings, c_str, platform, of, device::Core};
+    /// # struct SampleDriver;
+    ///
+    /// impl platform::Driver for SampleDriver {
+    ///    # type IdInfo = ();
+    ///    # const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
+    ///
+    ///    fn probe(
+    ///       pdev: &platform::Device<Core>,
+    ///       info: Option<&Self::IdInfo>,
+    ///    ) -> Result<Pin<KBox<Self>>> {
+    ///       let offset = 0; // Some offset.
+    ///
+    ///       // If the size is known at compile time, use [`Self::iomap_sized`].
+    ///       //
+    ///       // No runtime checks will apply when reading and writing.
+    ///       let request = pdev.request_io_by_index(0).ok_or(ENODEV)?;
+    ///       let iomem = request.iomap_sized::<42>()?;
+    ///       let iomem = KBox::pin_init(iomem, GFP_KERNEL)?;
+    ///
+    ///       let io = iomem.access(pdev.as_ref())?;
+    ///
+    ///       // Read and write a 32-bit value at `offset`.
+    ///       let data = io.read32_relaxed(offset);
+    ///
+    ///       io.write32_relaxed(data, offset);
+    ///
+    ///       # Ok(KBox::new(SampleDriver, GFP_KERNEL)?.into())
+    ///     }
+    /// }
+    /// ```
+    pub fn iomap_sized<const SIZE: usize>(
+        self,
+    ) -> Result<impl PinInit<Devres<IoMem<SIZE>>, Error> + 'a> {
+        IoMem::new(self)
+    }
+
+    /// Same as [`Self::iomap_sized`] but with exclusive access to the
+    /// underlying region.
+    ///
+    /// This uses the
+    /// [`ioremap()`](https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device)
+    /// C API.
+    pub fn iomap_exclusive_sized<const SIZE: usize>(
+        self,
+    ) -> Result<impl PinInit<Devres<ExclusiveIoMem<SIZE>>, Error> + 'a> {
+        ExclusiveIoMem::new(self)
+    }
+
+    /// Maps an [`IoRequest`] where the size is not known at compile time,
+    ///
+    /// This uses the
+    /// [`ioremap()`](https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device)
+    /// C API.
+    ///
+    /// # Examples
+    ///
+    /// The following example uses a [`platform::Device`] for illustration
+    /// purposes.
+    ///
+    /// ```no_run
+    /// # use kernel::{bindings, c_str, platform, of, device::Core};
+    /// # struct SampleDriver;
+    ///
+    /// impl platform::Driver for SampleDriver {
+    ///    # type IdInfo = ();
+    ///    # const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
+    ///
+    ///    fn probe(
+    ///       pdev: &platform::Device<Core>,
+    ///       info: Option<&Self::IdInfo>,
+    ///    ) -> Result<Pin<KBox<Self>>> {
+    ///       let offset = 0; // Some offset.
+    ///
+    ///       // Unlike [`Self::iomap_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 should be used, leading to runtime checks on every
+    ///       // access.
+    ///       let request = pdev.request_io_by_index(0).ok_or(ENODEV)?;
+    ///       let iomem = request.iomap()?;
+    ///       let iomem = KBox::pin_init(iomem, GFP_KERNEL)?;
+    ///
+    ///       let io = iomem.access(pdev.as_ref())?;
+    ///
+    ///       let data = io.try_read32_relaxed(offset)?;
+    ///
+    ///       io.try_write32_relaxed(data, offset)?;
+    ///
+    ///       # Ok(KBox::new(SampleDriver, GFP_KERNEL)?.into())
+    ///     }
+    /// }
+    /// ```
+    pub fn iomap(self) -> Result<impl PinInit<Devres<IoMem<0>>, Error> + 'a> {
+        Self::iomap_sized::<0>(self)
+    }
+
+    /// Same as [`Self::iomap`] but with exclusive access to the underlying
+    /// region.
+    pub fn iomap_exclusive(self) -> Result<impl PinInit<Devres<ExclusiveIoMem<0>>, Error> + 'a> {
+        Self::iomap_exclusive_sized::<0>(self)
+    }
+}
+
+/// An exclusive memory-mapped IO region.
+///
+/// # Invariants
+///
+/// - [`ExclusiveIoMem`] has exclusive access to the underlying [`IoMem`].
+#[pin_data]
+pub struct ExclusiveIoMem<const SIZE: usize> {
+    /// The underlying `IoMem` instance.
+    iomem: IoMem<SIZE>,
+
+    /// The region abstraction. This represents exclusive access to the
+    /// range represented by the underlying `iomem`.
+    ///
+    /// This field is needed for ownership of the region.
+    _region: Region,
+}
+
+impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
+    /// Creates a new `ExclusiveIoMem` instance.
+    fn ioremap(resource: &Resource) -> Result<Self> {
+        let start = resource.start();
+        let size = resource.size();
+        let name = resource.name();
+
+        let region = resource
+            .request_region(start, size, name, io::resource::flags::IORESOURCE_MEM)
+            .ok_or(EBUSY)?;
+
+        let iomem = IoMem::ioremap(resource)?;
+
+        let iomem = ExclusiveIoMem {
+            iomem,
+            _region: region,
+        };
+
+        Ok(iomem)
+    }
+
+    /// Creates a new `ExclusiveIoMem` instance from a previously acquired [`IoRequest`].
+    pub fn new<'a>(io_request: IoRequest<'a>) -> Result<impl PinInit<Devres<Self>, Error> + 'a> {
+        let iomem = Self::ioremap(io_request.resource)?;
+        let devres = Devres::new(io_request.device, iomem);
+
+        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`] instance 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();
+
+        let addr = if resource
+            .flags()
+            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
+        {
+            // SAFETY:
+            // - `res_start` and `size` are read from a presumably valid `struct resource`.
+            // - `size` is known not to be zero at this point.
+            unsafe { bindings::ioremap_np(res_start, size as usize) }
+        } else {
+            // SAFETY:
+            // - `res_start` and `size` are read from a presumably valid `struct resource`.
+            // - `size` is known not to be zero at this point.
+            unsafe { bindings::ioremap(res_start, size as usize) }
+        };
+
+        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 from a previously acquired [`IoRequest`].
+    pub fn new<'a>(io_request: IoRequest<'a>) -> Result<impl PinInit<Devres<Self>, Error> + 'a> {
+        let io = Self::ioremap(io_request.resource)?;
+        let devres = Devres::new(io_request.device, io);
+
+        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.50.0

[PATCH v12 3/3] rust: platform: add resource accessors

[Daniel Almeida]

The previous patches have added the abstractions for Resources and the
ability to map them and therefore read and write the underlying memory .

The only thing missing to make this accessible for platform devices is
to provide accessors that return instances of IoRequest<'a>. These
ensure that the resource are valid only for the lifetime of the platform
device, and that the platform device is in the Bound state.

Therefore, add these accessors. Also make it possible to retrieve
resources from platform devices in Rust using either a name or an index.

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

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 86f9d73c64b38ffe067be329a77b2fc04564c7fe..b2b4ca4671f019e1080e0579ae42a356eaa5f0bb 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,8 +5,11 @@
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
 use crate::{
-    acpi, bindings, container_of, device, driver,
+    acpi, bindings, container_of,
+    device::{self, Bound},
+    driver,
     error::{to_result, Result},
+    io::{mem::IoRequest, Resource},
     of,
     prelude::*,
     str::CStr,
@@ -211,6 +214,61 @@ impl<Ctx: device::DeviceContext> Device<Ctx> {
     fn as_raw(&self) -> *mut bindings::platform_device {
         self.0.get()
     }
+
+    /// Returns the resource at `index`, if any.
+    pub fn resource_by_index(&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::as_ref(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::as_ref(resource) })
+    }
+}
+
+impl Device<Bound> {
+    /// Returns an `IoRequest` for the resource at `index`, if any.
+    pub fn request_io_by_index(&self, index: u32) -> Option<IoRequest<'_>> {
+        // SAFETY: `resource` is a valid resource for `&self` during the
+        // lifetime of the `IoRequest`.
+        self.resource_by_index(index)
+            .map(|resource| unsafe { IoRequest::new(self.as_ref(), resource) })
+    }
+
+    /// Returns an `IoRequest` for the resource with a given `name`, if any.
+    pub fn request_io_by_name(&self, name: &CStr) -> Option<IoRequest<'_>> {
+        // SAFETY: `resource` is a valid resource for `&self` during the
+        // lifetime of the `IoRequest`.
+        self.resource_by_name(name)
+            .map(|resource| unsafe { IoRequest::new(self.as_ref(), resource) })
+    }
 }
 
 // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic

-- 
2.50.0

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

[Danilo Krummrich]

On Fri, Jul 04, 2025 at 01:25:27PM -0300, Daniel Almeida wrote:

This looks good now, one comment below.

> +    /// Creates a new `IoMem` instance from a previously acquired [`IoRequest`].
> +    pub fn new<'a>(io_request: IoRequest<'a>) -> Result<impl PinInit<Devres<Self>, Error> + 'a> {
> +        let io = Self::ioremap(io_request.resource)?;
> +        let devres = Devres::new(io_request.device, io);
> +
> +        Ok(devres)
> +    }

This method should not return `Result<impl PinInit<Devres<Self>, Error> + 'a>`,
but just `impl PinInit<Devres<Self>, Error> + 'a`.

Here's the full diff of the changes needed. :)

diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index b047b9a73ae5..cb0805ef0860 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -60,7 +60,7 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
     ///       //
     ///       // No runtime checks will apply when reading and writing.
     ///       let request = pdev.request_io_by_index(0).ok_or(ENODEV)?;
-    ///       let iomem = request.iomap_sized::<42>()?;
+    ///       let iomem = request.iomap_sized::<42>();
     ///       let iomem = KBox::pin_init(iomem, GFP_KERNEL)?;
     ///
     ///       let io = iomem.access(pdev.as_ref())?;
@@ -74,9 +74,7 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
     ///     }
     /// }
     /// ```
-    pub fn iomap_sized<const SIZE: usize>(
-        self,
-    ) -> Result<impl PinInit<Devres<IoMem<SIZE>>, Error> + 'a> {
+    pub fn iomap_sized<const SIZE: usize>(self) -> impl PinInit<Devres<IoMem<SIZE>>, Error> + 'a {
         IoMem::new(self)
     }

@@ -88,7 +86,7 @@ pub fn iomap_sized<const SIZE: usize>(
     /// C API.
     pub fn iomap_exclusive_sized<const SIZE: usize>(
         self,
-    ) -> Result<impl PinInit<Devres<ExclusiveIoMem<SIZE>>, Error> + 'a> {
+    ) -> impl PinInit<Devres<ExclusiveIoMem<SIZE>>, Error> + 'a {
         ExclusiveIoMem::new(self)
     }

@@ -122,7 +120,7 @@ pub fn iomap_exclusive_sized<const SIZE: usize>(
     ///       // family of functions should be used, leading to runtime checks on every
     ///       // access.
     ///       let request = pdev.request_io_by_index(0).ok_or(ENODEV)?;
-    ///       let iomem = request.iomap()?;
+    ///       let iomem = request.iomap();
     ///       let iomem = KBox::pin_init(iomem, GFP_KERNEL)?;
     ///
     ///       let io = iomem.access(pdev.as_ref())?;
@@ -135,13 +133,13 @@ pub fn iomap_exclusive_sized<const SIZE: usize>(
     ///     }
     /// }
     /// ```
-    pub fn iomap(self) -> Result<impl PinInit<Devres<IoMem<0>>, Error> + 'a> {
+    pub fn iomap(self) -> impl PinInit<Devres<IoMem<0>>, Error> + 'a {
         Self::iomap_sized::<0>(self)
     }

     /// Same as [`Self::iomap`] but with exclusive access to the underlying
     /// region.
-    pub fn iomap_exclusive(self) -> Result<impl PinInit<Devres<ExclusiveIoMem<0>>, Error> + 'a> {
+    pub fn iomap_exclusive(self) -> impl PinInit<Devres<ExclusiveIoMem<0>>, Error> + 'a {
         Self::iomap_exclusive_sized::<0>(self)
     }
 }
@@ -185,11 +183,11 @@ fn ioremap(resource: &Resource) -> Result<Self> {
     }

     /// Creates a new `ExclusiveIoMem` instance from a previously acquired [`IoRequest`].
-    pub fn new<'a>(io_request: IoRequest<'a>) -> Result<impl PinInit<Devres<Self>, Error> + 'a> {
-        let iomem = Self::ioremap(io_request.resource)?;
-        let devres = Devres::new(io_request.device, iomem);
+    pub fn new<'a>(io_request: IoRequest<'a>) -> impl PinInit<Devres<Self>, Error> + 'a {
+        let dev = io_request.device;
+        let res = io_request.resource;

-        Ok(devres)
+        Devres::new(dev, Self::ioremap(res))
     }
 }

@@ -249,11 +247,11 @@ fn ioremap(resource: &Resource) -> Result<Self> {
     }

     /// Creates a new `IoMem` instance from a previously acquired [`IoRequest`].
-    pub fn new<'a>(io_request: IoRequest<'a>) -> Result<impl PinInit<Devres<Self>, Error> + 'a> {
-        let io = Self::ioremap(io_request.resource)?;
-        let devres = Devres::new(io_request.device, io);
+    pub fn new<'a>(io_request: IoRequest<'a>) -> impl PinInit<Devres<Self>, Error> + 'a {
+        let dev = io_request.device;
+        let res = io_request.resource;

-        Ok(devres)
+        Devres::new(dev, Self::ioremap(res))
     }
 }

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

[Danilo Krummrich]

On Fri, Jul 04, 2025 at 01:25:26PM -0300, Daniel Almeida wrote:
> +/// Resource Size type.
> +///
> +/// This is a type alias to either `u32` or `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 either `u32` or `u64` depending on the config option
> +/// `CONFIG_PHYS_ADDR_T_64BIT`.
> +#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
> +pub type ResourceSize = u32;

I think someone commented on this previously, but maybe I also do not remember
correctly.

Anyways, can't we just do:

	pub type ResourceSize = bindings::phys_addr_t;

Given that phys_addr_t is already either u32 or u64.

Re: [PATCH v12 3/3] rust: platform: add resource accessors

[Danilo Krummrich]

On Fri, Jul 04, 2025 at 01:25:28PM -0300, Daniel Almeida wrote:
> +impl Device<Bound> {
> +    /// Returns an `IoRequest` for the resource at `index`, if any.
> +    pub fn request_io_by_index(&self, index: u32) -> Option<IoRequest<'_>> {
> +        // SAFETY: `resource` is a valid resource for `&self` during the
> +        // lifetime of the `IoRequest`.
> +        self.resource_by_index(index)
> +            .map(|resource| unsafe { IoRequest::new(self.as_ref(), resource) })
> +    }
> +
> +    /// Returns an `IoRequest` for the resource with a given `name`, if any.
> +    pub fn request_io_by_name(&self, name: &CStr) -> Option<IoRequest<'_>> {
> +        // SAFETY: `resource` is a valid resource for `&self` during the
> +        // lifetime of the `IoRequest`.
> +        self.resource_by_name(name)
> +            .map(|resource| unsafe { IoRequest::new(self.as_ref(), resource) })
> +    }
>  }

I think we should name this io_request_by_index() and io_request_by_name()
instead. We're not requesting to remap I/O memory (yet), but trying get an
IoRequest instance, hence I think this order fits better.

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

[Alice Ryhl]

On Sat, Jul 05, 2025 at 07:28:07PM +0200, Danilo Krummrich wrote:
> On Fri, Jul 04, 2025 at 01:25:26PM -0300, Daniel Almeida wrote:
> > +/// Resource Size type.
> > +///
> > +/// This is a type alias to either `u32` or `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 either `u32` or `u64` depending on the config option
> > +/// `CONFIG_PHYS_ADDR_T_64BIT`.
> > +#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
> > +pub type ResourceSize = u32;
> 
> I think someone commented on this previously, but maybe I also do not remember
> correctly.
> 
> Anyways, can't we just do:
> 
> 	pub type ResourceSize = bindings::phys_addr_t;
> 
> Given that phys_addr_t is already either u32 or u64.

In addition, it would be nice for the documentation to mention that it
can be 64-bit even on a 32-bit machine.

Alice

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

[Alice Ryhl]

On Fri, Jul 04, 2025 at 01:25:26PM -0300, Daniel Almeida wrote:
> 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>

Overall looks reasonable, but some comments below:

>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/io.c               |  36 +++++++
>  rust/kernel/io.rs               |   4 +
>  rust/kernel/io/resource.rs      | 209 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 250 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 7e8f2285064797d5bbac5583990ff823b76c6bdc..5f795e60e889b9fc887013743c81b1cf92a52adb 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -53,6 +53,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 15ea187c5466256effd07efe6f6995a1dd95a967..404776cf6717c8570c7600a24712ce6e4623d3c6 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, 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 72d80a6f131e3e826ecd9d2c3bcf54e89aa60cc3..7b70d5b5477e57d6d0f24bcd26bd8b0071721bc0 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,10 @@
>  use crate::error::{code::EINVAL, Result};
>  use crate::{bindings, build_assert};
>  
> +pub mod resource;
> +
> +pub use resource::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 0000000000000000000000000000000000000000..a8ad04b1abf8e46928ed98564e64a07180250024
> --- /dev/null
> +++ b/rust/kernel/io/resource.rs
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for [system
> +//! resources](https://docs.kernel.org/core-api/kernel-api.html#resources-management).
> +//!
> +//! 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;
> +
> +/// Resource Size type.
> +///
> +/// This is a type alias to either `u32` or `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 either `u32` or `u64` 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
> +///   `bindings::__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::as_ref(self.0.as_ptr()) }
> +    }
> +}
> +
> +impl Drop for Region {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as per the invariant of `Region`.
> +        let res = unsafe { Resource::as_ref(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
> +        };

You can avoid this unsafe via the deref() impl.

let (flags, start, size) = {
    let res = &**self;
    (res.flags(), res.start(), res.size())
};

> +        // 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 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 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 as_ref<'a>(ptr: *mut bindings::resource) -> &'a Self {

We usually call this method `from_raw`.

> +        // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Requests a resource region.
> +    ///
> +    /// Exclusive access will be given and the region will be marked as busy.
> +    /// Further calls to [`Self::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: &'static CStr,

Is this name used for a lookup or stored? If just a lookup, then it
doesn't need to be 'static.

> +        flags: Flags,
> +    ) -> Option<Region> {
> +        // SAFETY: Safe as per the invariant of `Resource`.
> +        let region = unsafe {
> +            bindings::__request_region(
> +                self.0.get(),
> +                start,
> +                size,
> +                name.as_char_ptr(),
> +                flags.0 as i32,

I would use `as c_int` here. (Or change the type stored in Flags.)

> +            )
> +        };
> +
> +        Some(Region(NonNull::new(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

This needs to be

unsafe { (*inner).start }

What you wrote is not equivalent. (*inner) is a place expression, but
when you write `unsafe { <place expr> }` that turns it into a value
expression by reading the value. So this code copies the entire struct
to the stack, and then you read `start` from the copy on the stack.

> +    }
> +
> +    /// Returns the name of the resource.
> +    pub fn name(&self) -> &'static 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 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 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(usize);

Based on usage it looks like the correct type is c_int?

> +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 {

You can do:

impl Flags {
    pub const IORESOURCE_IO: Flags = Flags(bindings::IORESOURCE_IO as usize);
}

instead of a module.

> +    use super::Flags;
> +
> +    /// PCI/ISA I/O ports.
> +    pub const IORESOURCE_IO: Flags = Flags(bindings::IORESOURCE_IO as usize);
> +
> +    /// Resource is software muxed.
> +    pub const IORESOURCE_MUXED: Flags = Flags(bindings::IORESOURCE_MUXED as usize);
> +
> +    /// Resource represents a memory region.
> +    pub const IORESOURCE_MEM: Flags = Flags(bindings::IORESOURCE_MEM as usize);
> +
> +    /// Resource represents a memory region that must be ioremaped using `ioremap_np`.
> +    pub const IORESOURCE_MEM_NONPOSTED: Flags = Flags(bindings::IORESOURCE_MEM_NONPOSTED as usize);
> +}
> 
> -- 
> 2.50.0
> 

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

[Alice Ryhl]

On Fri, Jul 04, 2025 at 01:25:27PM -0300, Daniel Almeida wrote:
> Add a generic iomem abstraction to safely read and write ioremapped
> regions. This abstraction requires a previously acquired IoRequest
> instance. This makes it so that both the resource and the device match,
> or, in other words, that the resource is indeed a valid resource for a
> given bound device.
> 
> A subsequent patch will add the ability to retrieve IoRequest instances
> from platform devices.
> 
> 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>

> +    /// ```no_run
> +    /// # use kernel::{bindings, c_str, platform, of, device::Core};
> +    /// # struct SampleDriver;
> +    ///
> +    /// impl platform::Driver for SampleDriver {
> +    ///    # type IdInfo = ();
> +    ///    # const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
> +    ///
> +    ///    fn probe(

When using # to hide lines from the example, it's useful to think about
what's left. The rendered docs will have a weird empty newline at the
beginning and before `fn probe`.

So I would either remove those newlines or just not hide those lines.

> +    /// Same as [`Self::iomap_sized`] but with exclusive access to the
> +    /// underlying region.
> +    ///
> +    /// This uses the
> +    /// [`ioremap()`](https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device)
> +    /// C API.

I would probably format this like this:

/// This uses the [`ioremap()`] C API.
///
/// [`ioremap()`]: https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device

> +/// An exclusive memory-mapped IO region.
> +///
> +/// # Invariants
> +///
> +/// - [`ExclusiveIoMem`] has exclusive access to the underlying [`IoMem`].
> +#[pin_data]
> +pub struct ExclusiveIoMem<const SIZE: usize> {

You don't need #[pin_data] if there aren't any pinned fields.

> +    /// The underlying `IoMem` instance.
> +    iomem: IoMem<SIZE>,
> +
> +    /// The region abstraction. This represents exclusive access to the
> +    /// range represented by the underlying `iomem`.
> +    ///
> +    /// This field is needed for ownership of the region.
> +    _region: Region,
> +}
> [..]
> +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();
> +
> +        let addr = if resource
> +            .flags()
> +            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
> +        {
> +            // SAFETY:
> +            // - `res_start` and `size` are read from a presumably valid `struct resource`.
> +            // - `size` is known not to be zero at this point.
> +            unsafe { bindings::ioremap_np(res_start, size as usize) }

Here you cast from ResourceSize to usize. Are you sure that is correct?
I thought those types could be different.

> +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) }

Just c_void.

Alice

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

[Daniel Almeida]

Hi Alice,

[…]


>> +
>> +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 as_ref<'a>(ptr: *mut bindings::resource) -> &'a Self {
> 
> We usually call this method `from_raw`.

Hmm, pretty sure I have seen the exact opposite being asked. In fact, this was
discussed a bit at length a while ago. See the thread starting at [0] for context.

> 
>> +            )
>> +        };
>> +
>> +        Some(Region(NonNull::new(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
> 
> This needs to be
> 
> unsafe { (*inner).start }
> 
> What you wrote is not equivalent. (*inner) is a place expression, but
> when you write `unsafe { <place expr> }` that turns it into a value
> expression by reading the value. So this code copies the entire struct
> to the stack, and then you read `start` from the copy on the stack.

To be honest, I've seen a bug where the opposite was going on, a field was
being written on the value that was copied to the stack, leaving the original unchanged.

In any case, I thought this would be optimized away by the compiler. Also, IMHO
there should be a lint for this, because it does not make sense to copy a struct
just to read a field if we can just read the original location.

Thanks for catching that though, I will fix it on the next iteration :)

[…]

Thanks for the other comments,

— Daniel

[0] https://lore.kernel.org/rust-for-linux/B3564D77-9E26-4019-9B75-B0C53B26B64F@collabora.com/

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

[Alice Ryhl]

On Tue, Jul 8, 2025 at 7:44 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice,
>
> […]
>
>
> >> +
> >> +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 as_ref<'a>(ptr: *mut bindings::resource) -> &'a Self {
> >
> > We usually call this method `from_raw`.
>
> Hmm, pretty sure I have seen the exact opposite being asked. In fact, this was
> discussed a bit at length a while ago. See the thread starting at [0] for context.

I will submit a patch.

Alicej

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

[Alice Ryhl]

On Tue, Jul 08, 2025 at 02:43:33PM -0300, Daniel Almeida wrote:
> > What you wrote is not equivalent. (*inner) is a place expression, but
> > when you write `unsafe { <place expr> }` that turns it into a value
> > expression by reading the value. So this code copies the entire struct
> > to the stack, and then you read `start` from the copy on the stack.
> 
> To be honest, I've seen a bug where the opposite was going on, a field was
> being written on the value that was copied to the stack, leaving the original unchanged.
> 
> In any case, I thought this would be optimized away by the compiler. Also, IMHO
> there should be a lint for this, because it does not make sense to copy a struct
> just to read a field if we can just read the original location.
> 
> Thanks for catching that though, I will fix it on the next iteration :)

Yes, it is probably optimized out. However, the main problem is that
when you read the entire struct, that could be a data race of another
field which may be UB.

Alice

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

[Daniel Almeida]

Hi Alice,

> Is this name used for a lookup or stored? If just a lookup, then it
> doesn't need to be 'static.
> 
>> +        flags: Flags,
>> +    ) -> Option<Region> {
>> +        // SAFETY: Safe as per the invariant of `Resource`.
>> +        let region = unsafe {
>> +            bindings::__request_region(
>> +                self.0.get(),
>> +                start,
>> +                size,
>> +                name.as_char_ptr(),
>> +                flags.0 as i32,
> 

JFYI, this is stored:

static int __request_region_locked(struct resource *res, struct resource *parent,
				   resource_size_t start, resource_size_t n,
				   const char *name, int flags)
{
	DECLARE_WAITQUEUE(wait, current);

	res->name = name;


— Daniel

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

[Alice Ryhl]

On Thu, Jul 10, 2025 at 3:16 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice,
>
> > Is this name used for a lookup or stored? If just a lookup, then it
> > doesn't need to be 'static.
> >
> >> +        flags: Flags,
> >> +    ) -> Option<Region> {
> >> +        // SAFETY: Safe as per the invariant of `Resource`.
> >> +        let region = unsafe {
> >> +            bindings::__request_region(
> >> +                self.0.get(),
> >> +                start,
> >> +                size,
> >> +                name.as_char_ptr(),
> >> +                flags.0 as i32,
> >
>
> JFYI, this is stored:
>
> static int __request_region_locked(struct resource *res, struct resource *parent,
>                                    resource_size_t start, resource_size_t n,
>                                    const char *name, int flags)
> {
>         DECLARE_WAITQUEUE(wait, current);
>
>         res->name = name;

I think it would be useful for the safety comment to say something
along the lines of "it's okay that __request_region stashes a copy of
the string because it is static".

Alice

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

[Daniel Almeida]

[…]

> 
>> +    }
>> +
>> +    /// Returns the name of the resource.
>> +    pub fn name(&self) -> &'static 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 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 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(usize);
> 
> Based on usage it looks like the correct type is c_int?

Shouldn’t this be c_ulong because of:

#[repr(C)]
#[derive(Copy, Clone)]
pub struct resource {
    pub start: resource_size_t,
    pub end: resource_size_t,
    pub name: *const ffi::c_char,
    pub flags: ffi::c_ulong, <——

In any case, we will have to cast this because __request_region
expects c_int.

— Daniel

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

[Alice Ryhl]

On Thu, Jul 10, 2025 at 3:34 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> […]
>
> >
> >> +    }
> >> +
> >> +    /// Returns the name of the resource.
> >> +    pub fn name(&self) -> &'static 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 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 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(usize);
> >
> > Based on usage it looks like the correct type is c_int?
>
> Shouldn’t this be c_ulong because of:
>
> #[repr(C)]
> #[derive(Copy, Clone)]
> pub struct resource {
>     pub start: resource_size_t,
>     pub end: resource_size_t,
>     pub name: *const ffi::c_char,
>     pub flags: ffi::c_ulong, <——
>
> In any case, we will have to cast this because __request_region
> expects c_int.

I saw that you called a C function that expects an int, so that's why
I suggested c_int. It sounds like the cast from int to long happens in
C code.

Alice

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

[Daniel Almeida]

Hi Alice,

>> +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();
>> +
>> +        let addr = if resource
>> +            .flags()
>> +            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
>> +        {
>> +            // SAFETY:
>> +            // - `res_start` and `size` are read from a presumably valid `struct resource`.
>> +            // - `size` is known not to be zero at this point.
>> +            unsafe { bindings::ioremap_np(res_start, size as usize) }
> 
> Here you cast from ResourceSize to usize. Are you sure that is correct?
> I thought those types could be different.

This seems to what C is doing as well, i.e.:

static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
				    resource_size_t size, <---------
				    enum devm_ioremap_type type)
{

        […]

	case DEVM_IOREMAP_NP:
		addr = ioremap_np(offset, size);
		break;
	}


Where:

`static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size)`

IOW: this stems from the mix and match of types used the C API itself.

What do you suggest here? Maybe a try_into() then?

— Daniel

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

[Alice Ryhl]

On Thu, Jul 10, 2025 at 10:58:13AM -0300, Daniel Almeida wrote:
> Hi Alice,
> 
> >> +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();
> >> +
> >> +        let addr = if resource
> >> +            .flags()
> >> +            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
> >> +        {
> >> +            // SAFETY:
> >> +            // - `res_start` and `size` are read from a presumably valid `struct resource`.
> >> +            // - `size` is known not to be zero at this point.
> >> +            unsafe { bindings::ioremap_np(res_start, size as usize) }
> > 
> > Here you cast from ResourceSize to usize. Are you sure that is correct?
> > I thought those types could be different.
> 
> This seems to what C is doing as well, i.e.:
> 
> static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
> 				    resource_size_t size, <---------
> 				    enum devm_ioremap_type type)
> {
> 
>         […]
> 
> 	case DEVM_IOREMAP_NP:
> 		addr = ioremap_np(offset, size);
> 		break;
> 	}
> 
> 
> Where:
> 
> `static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size)`
> 
> IOW: this stems from the mix and match of types used the C API itself.
> 
> What do you suggest here? Maybe a try_into() then?

What a mess. It looks like there aren't any 32-bit architectures that
define ioremap_np. This means that sometimes this cast will be lossy,
but in those cases the function body just returns NULL and doesn't read
the size.

I would probably cast to an underscore instead of explicitly mentioning
the target type and make a comment about it.

Alice

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

[Danilo Krummrich]

On Mon Jul 14, 2025 at 1:51 PM CEST, Alice Ryhl wrote:
> On Thu, Jul 10, 2025 at 10:58:13AM -0300, Daniel Almeida wrote:
>> Hi Alice,
>> 
>> >> +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();
>> >> +
>> >> +        let addr = if resource
>> >> +            .flags()
>> >> +            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
>> >> +        {
>> >> +            // SAFETY:
>> >> +            // - `res_start` and `size` are read from a presumably valid `struct resource`.
>> >> +            // - `size` is known not to be zero at this point.
>> >> +            unsafe { bindings::ioremap_np(res_start, size as usize) }
>> > 
>> > Here you cast from ResourceSize to usize. Are you sure that is correct?
>> > I thought those types could be different.
>> 
>> This seems to what C is doing as well, i.e.:
>> 
>> static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
>> 				    resource_size_t size, <---------
>> 				    enum devm_ioremap_type type)
>> {
>> 
>>         […]
>> 
>> 	case DEVM_IOREMAP_NP:
>> 		addr = ioremap_np(offset, size);
>> 		break;
>> 	}
>> 
>> 
>> Where:
>> 
>> `static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size)`
>> 
>> IOW: this stems from the mix and match of types used the C API itself.
>> 
>> What do you suggest here? Maybe a try_into() then?
>
> What a mess.

Yeah, it mixes up types describing CPU word width and bus address width. :(

> It looks like there aren't any 32-bit architectures that
> define ioremap_np. This means that sometimes this cast will be lossy,
> but in those cases the function body just returns NULL and doesn't read
> the size.
>
> I would probably cast to an underscore instead of explicitly mentioning
> the target type and make a comment about it.

I think fixing up the C side would be even nicer, but for the scope of this
series that's fine. The comment should mention that, ultimately, we want to fix
up the C side type wise.

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

[Daniel Almeida]

Alice,

> On 14 Jul 2025, at 08:51, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Thu, Jul 10, 2025 at 10:58:13AM -0300, Daniel Almeida wrote:
>> Hi Alice,
>> 
>>>> +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();
>>>> +
>>>> +        let addr = if resource
>>>> +            .flags()
>>>> +            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
>>>> +        {
>>>> +            // SAFETY:
>>>> +            // - `res_start` and `size` are read from a presumably valid `struct resource`.
>>>> +            // - `size` is known not to be zero at this point.
>>>> +            unsafe { bindings::ioremap_np(res_start, size as usize) }
>>> 
>>> Here you cast from ResourceSize to usize. Are you sure that is correct?
>>> I thought those types could be different.
>> 
>> This seems to what C is doing as well, i.e.:
>> 
>> static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
>>     resource_size_t size, <---------
>>     enum devm_ioremap_type type)
>> {
>> 
>>        […]
>> 
>> case DEVM_IOREMAP_NP:
>> addr = ioremap_np(offset, size);
>> break;
>> }
>> 
>> 
>> Where:
>> 
>> `static inline void __iomem *ioremap_np(phys_addr_t offset, size_t size)`
>> 
>> IOW: this stems from the mix and match of types used the C API itself.
>> 
>> What do you suggest here? Maybe a try_into() then?
> 
> What a mess. It looks like there aren't any 32-bit architectures that
> define ioremap_np. This means that sometimes this cast will be lossy,
> but in those cases the function body just returns NULL and doesn't read
> the size.
> 
> I would probably cast to an underscore instead of explicitly mentioning
> the target type and make a comment about it.
> 
> Alice

You replied here but v13 was already out [0]. Can we shift the dicussion
there? I ended up using try_into(), but feel free to suggest the cast to _
+ the TODO if you still feel like this is the right approach.


-- Daniel

[0] https://lore.kernel.org/rust-for-linux/20250711-topics-tyr-platform_iomem-v13-0-06328b514db3@collabora.com/