All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 0/3] rust: platform: add Io support
@ 2025-05-09 20:29 Daniel Almeida
  2025-05-09 20:29 ` [PATCH v8 1/3] rust: io: add resource abstraction Daniel Almeida
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Daniel Almeida @ 2025-05-09 20:29 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Andrew Morton, Andy Shevchenko,
	Ilpo Järvinen, Bjorn Helgaas, Mika Westerberg, Ying Huang
  Cc: linux-kernel, rust-for-linux, Fiona Behrens, Daniel Almeida

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: 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

---
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               |  41 ++++++++
 rust/kernel/io.rs               |   3 +
 rust/kernel/io/mem.rs           | 141 +++++++++++++++++++++++++
 rust/kernel/io/resource.rs      | 222 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/platform.rs         | 128 ++++++++++++++++++++++-
 6 files changed, 535 insertions(+), 1 deletion(-)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250318-topics-tyr-platform_iomem-1710a177e1df

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v8 1/3] rust: io: add resource abstraction
  2025-05-09 20:29 [PATCH v8 0/3] rust: platform: add Io support Daniel Almeida
@ 2025-05-09 20:29 ` Daniel Almeida
  2025-05-15 15:53   ` Danilo Krummrich
  2025-05-09 20:29 ` [PATCH v8 2/3] rust: io: mem: add a generic iomem abstraction Daniel Almeida
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Daniel Almeida @ 2025-05-09 20:29 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Andrew Morton, Andy Shevchenko,
	Ilpo Järvinen, Bjorn Helgaas, Mika Westerberg, Ying Huang
  Cc: linux-kernel, rust-for-linux, Fiona Behrens, 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      | 222 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 261 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ab37e1d35c70d52e69b754bf855bc19911d156d8..b0c1a7ec3ef6f64834dc0e8c07931b7d40c975a9 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -19,6 +19,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 4c2401ccd72078af4e6901f4cd95f9070522f396..939ab38ca61dac4cf884677a20edc760094d5812 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 72d80a6f131e3e826ecd9d2c3bcf54e89aa60cc3..1da447078633e058000953a581b59d3ed5eb0e69 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 0000000000000000000000000000000000000000..3eb5c8ef585078398551fe8189fd96c1b6c1eeff
--- /dev/null
+++ b/rust/kernel/io/resource.rs
@@ -0,0 +1,222 @@
+// 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;
+
+#[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)) }
+}
+
+#[cfg(CONFIG_HAS_IOMEM)]
+/// 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 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 from_ptr<'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 `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 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(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.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v8 2/3] rust: io: mem: add a generic iomem abstraction
  2025-05-09 20:29 [PATCH v8 0/3] rust: platform: add Io support Daniel Almeida
  2025-05-09 20:29 ` [PATCH v8 1/3] rust: io: add resource abstraction Daniel Almeida
@ 2025-05-09 20:29 ` Daniel Almeida
  2025-05-15 16:01   ` Danilo Krummrich
  2025-05-09 20:29 ` [PATCH v8 3/3] rust: platform: allow ioremap of platform resources Daniel Almeida
  2025-05-15 15:49 ` [PATCH v8 0/3] rust: platform: add Io support Danilo Krummrich
  3 siblings, 1 reply; 11+ messages in thread
From: Daniel Almeida @ 2025-05-09 20:29 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Andrew Morton, Andy Shevchenko,
	Ilpo Järvinen, Bjorn Helgaas, Mika Westerberg, Ying Huang
  Cc: linux-kernel, rust-for-linux, 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/helpers/io.c     |   5 ++
 rust/kernel/io.rs     |   1 +
 rust/kernel/io/mem.rs | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+)

diff --git a/rust/helpers/io.c b/rust/helpers/io.c
index 939ab38ca61dac4cf884677a20edc760094d5812..4fbd70ab60f64155bef835a43b3c64d441aee7bf 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(volatile void __iomem *addr)
 {
 	iounmap(addr);
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 1da447078633e058000953a581b59d3ed5eb0e69..0249aedd8c6ce435b6b00137be0895a206956bca 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 0000000000000000000000000000000000000000..3e7ef8b6f0ca8b5b195a94c7ed0f94a9e6c72944
--- /dev/null
+++ b/rust/kernel/io/mem.rs
@@ -0,0 +1,141 @@
+// 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;
+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.
+    ///
+    /// This field is needed for ownership of the region.
+    _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_region(start, size, name, io::resource::flags::IORESOURCE_MEM)
+            .ok_or(EBUSY)?;
+
+        let iomem = ExclusiveIoMem {
+            iomem,
+            _region: 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` 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.
+    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.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v8 3/3] rust: platform: allow ioremap of platform resources
  2025-05-09 20:29 [PATCH v8 0/3] rust: platform: add Io support Daniel Almeida
  2025-05-09 20:29 ` [PATCH v8 1/3] rust: io: add resource abstraction Daniel Almeida
  2025-05-09 20:29 ` [PATCH v8 2/3] rust: io: mem: add a generic iomem abstraction Daniel Almeida
@ 2025-05-09 20:29 ` Daniel Almeida
  2025-05-15 16:23   ` Danilo Krummrich
  2025-05-16  8:39   ` Danilo Krummrich
  2025-05-15 15:49 ` [PATCH v8 0/3] rust: platform: add Io support Danilo Krummrich
  3 siblings, 2 replies; 11+ messages in thread
From: Daniel Almeida @ 2025-05-09 20:29 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Andrew Morton, Andy Shevchenko,
	Ilpo Järvinen, Bjorn Helgaas, Mika Westerberg, Ying Huang
  Cc: linux-kernel, rust-for-linux, 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 | 128 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 4917cb34e2fe8027d3d861e90de51de85f006735..5c550fc6d429ffc541a17fb5f8a1c2eb4476b560 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, device, driver,
+    bindings, device,
+    devres::Devres,
+    driver,
     error::{to_result, Result},
+    io::{
+        mem::{ExclusiveIoMem, IoMem},
+        resource::Resource,
+    },
     of,
     prelude::*,
     str::CStr,
@@ -223,6 +229,126 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }
 
+impl Device<device::Core> {
+    /// Maps a platform resource through ioremap() where the size is known at
+    /// compile time.
+    ///
+    /// # Examples
+    ///
+    /// ```no_run
+    /// # use kernel::{bindings, c_str, platform};
+    /// # use kernel::device::Core;
+    ///
+    ///
+    /// fn probe(pdev: &mut platform::Device<Core>, /* ... */) -> 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)?.read32_relaxed(offset);
+    ///
+    ///     iomem.try_access().ok_or(ENODEV)?.write32_relaxed(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};
+    /// # use kernel::device::Core;
+    ///
+    /// fn probe(pdev: &mut platform::Device<Core>, /* ... */) -> 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 should be used, 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_read32_relaxed(offset)?;
+    ///
+    ///     iomem.try_access().ok_or(ENODEV)?.try_write32_relaxed(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 {
     fn as_ref(&self) -> &device::Device {
         // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid

-- 
2.49.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 0/3] rust: platform: add Io support
  2025-05-09 20:29 [PATCH v8 0/3] rust: platform: add Io support Daniel Almeida
                   ` (2 preceding siblings ...)
  2025-05-09 20:29 ` [PATCH v8 3/3] rust: platform: allow ioremap of platform resources Daniel Almeida
@ 2025-05-15 15:49 ` Danilo Krummrich
  3 siblings, 0 replies; 11+ messages in thread
From: Danilo Krummrich @ 2025-05-15 15:49 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux,
	Fiona Behrens

On Fri, May 09, 2025 at 05:29:45PM -0300, Daniel Almeida wrote:
> Changes in v8:
> - Rebased on driver-core-next

Something went wrong I suppose. The patch series does not apply on top of
current driver-core-next and from the patches I can see that if it's based on
it, it must be driver-core-next from at least before 19th April.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 1/3] rust: io: add resource abstraction
  2025-05-09 20:29 ` [PATCH v8 1/3] rust: io: add resource abstraction Daniel Almeida
@ 2025-05-15 15:53   ` Danilo Krummrich
  0 siblings, 0 replies; 11+ messages in thread
From: Danilo Krummrich @ 2025-05-15 15:53 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux,
	Fiona Behrens

On Fri, May 09, 2025 at 05:29:46PM -0300, Daniel Almeida wrote:
> +#[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)) }

Should be possible to use `&raw mut`.

> +}
> +
> +#[cfg(CONFIG_HAS_IOMEM)]
> +/// 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)) }

Same here.

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

Throughout the tree, functions with this signature are usually called as_ref().

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 2/3] rust: io: mem: add a generic iomem abstraction
  2025-05-09 20:29 ` [PATCH v8 2/3] rust: io: mem: add a generic iomem abstraction Daniel Almeida
@ 2025-05-15 16:01   ` Danilo Krummrich
  0 siblings, 0 replies; 11+ messages in thread
From: Danilo Krummrich @ 2025-05-15 16:01 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux

On Fri, May 09, 2025 at 05:29:47PM -0300, Daniel Almeida wrote:
> +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_region(start, size, name, io::resource::flags::IORESOURCE_MEM)
> +            .ok_or(EBUSY)?;
> +
> +        let iomem = ExclusiveIoMem {
> +            iomem,
> +            _region: 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)?;

Here and in IoMem, Devres::new() requires a &Device<Bound>.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] rust: platform: allow ioremap of platform resources
  2025-05-09 20:29 ` [PATCH v8 3/3] rust: platform: allow ioremap of platform resources Daniel Almeida
@ 2025-05-15 16:23   ` Danilo Krummrich
  2025-05-28 17:29     ` Daniel Almeida
  2025-05-16  8:39   ` Danilo Krummrich
  1 sibling, 1 reply; 11+ messages in thread
From: Danilo Krummrich @ 2025-05-15 16:23 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux

On Fri, May 09, 2025 at 05:29:48PM -0300, Daniel Almeida wrote:
> +impl Device<device::Core> {
> +    /// Maps a platform resource through ioremap() where the size is known at
> +    /// compile time.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```no_run
> +    /// # use kernel::{bindings, c_str, platform};
> +    /// # use kernel::device::Core;
> +    ///
> +    ///
> +    /// fn probe(pdev: &mut platform::Device<Core>, /* ... */) -> Result<()> {

Should be &platform::Device<Core> (i.e. not mutable). You should also be able to
just use `Result` as return type. Though, it would probably be better to use the
real probe() function here, i.e.

	# struct Driver;

	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 `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)?.read32_relaxed(offset);
> +    ///
> +    ///     iomem.try_access().ok_or(ENODEV)?.write32_relaxed(data, offset);

Since this won't land for v6.16, can you please use Devres::access() [1]
instead? I.e.

	let iomem = pdev.ioremap_resource_sized::<42>(&resource)?;
	let io = Devres::access(pdev.as_ref())?;

	let data = io.read32_relaxed(offset);
	io.write32_relaxed(data, offset);

Devres::access() is in nova-next and lands in v6.16.

[1] https://gitlab.freedesktop.org/drm/nova/-/commit/f301cb978c068faa8fcd630be2cb317a2d0ec063

> +    ///
> +    ///     # 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};
> +    /// # use kernel::device::Core;
> +    ///
> +    /// fn probe(pdev: &mut platform::Device<Core>, /* ... */) -> 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 should be used, 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_read32_relaxed(offset)?;
> +    ///
> +    ///     iomem.try_access().ok_or(ENODEV)?.try_write32_relaxed(data, offset)?;
> +    ///
> +    ///     # Ok::<(), Error>(())
> +    /// }

Same as above.

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

This method should be a separate patch, no? Also, I think this one can go into
the `impl<Ctx: device::DeviceContext> Device<Ctx>` block, since it should be
valid to call from any device context.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] rust: platform: allow ioremap of platform resources
  2025-05-09 20:29 ` [PATCH v8 3/3] rust: platform: allow ioremap of platform resources Daniel Almeida
  2025-05-15 16:23   ` Danilo Krummrich
@ 2025-05-16  8:39   ` Danilo Krummrich
  1 sibling, 0 replies; 11+ messages in thread
From: Danilo Krummrich @ 2025-05-16  8:39 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux

On Fri, May 09, 2025 at 05:29:48PM -0300, Daniel Almeida wrote:
> +impl Device<device::Core> {

For the ioremap_*() ones, `impl Device<device::Bound>` should be enough.

Also, PCI names those just iomap_*(), maybe we should align those names for
consistency.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] rust: platform: allow ioremap of platform resources
  2025-05-15 16:23   ` Danilo Krummrich
@ 2025-05-28 17:29     ` Daniel Almeida
  2025-05-28 18:06       ` Danilo Krummrich
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Almeida @ 2025-05-28 17:29 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux

Hi Danilo,

[…]

> 
>> +    ///     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)?.read32_relaxed(offset);
>> +    ///
>> +    ///     iomem.try_access().ok_or(ENODEV)?.write32_relaxed(data, offset);
> 
> Since this won't land for v6.16, can you please use Devres::access() [1]
> instead? I.e.
> 
> let iomem = pdev.ioremap_resource_sized::<42>(&resource)?;
> let io = Devres::access(pdev.as_ref())?;
> 
> let data = io.read32_relaxed(offset);
> io.write32_relaxed(data, offset);
> 
> Devres::access() is in nova-next and lands in v6.16.
> 
> [1] https://gitlab.freedesktop.org/drm/nova/-/commit/f301cb978c068faa8fcd630be2cb317a2d0ec063

Devres:access takes &Device<Bound>, but the argument in probe() is
&Device<Core>.

Are these two types supposed to convert between them? I see no explicit
function to do so.

— Daniel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v8 3/3] rust: platform: allow ioremap of platform resources
  2025-05-28 17:29     ` Daniel Almeida
@ 2025-05-28 18:06       ` Danilo Krummrich
  0 siblings, 0 replies; 11+ messages in thread
From: Danilo Krummrich @ 2025-05-28 18:06 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Andrew Morton, Andy Shevchenko, Ilpo Järvinen, Bjorn Helgaas,
	Mika Westerberg, Ying Huang, linux-kernel, rust-for-linux

On Wed, May 28, 2025 at 02:29:44PM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> […]
> 
> > 
> >> +    ///     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)?.read32_relaxed(offset);
> >> +    ///
> >> +    ///     iomem.try_access().ok_or(ENODEV)?.write32_relaxed(data, offset);
> > 
> > Since this won't land for v6.16, can you please use Devres::access() [1]
> > instead? I.e.
> > 
> > let iomem = pdev.ioremap_resource_sized::<42>(&resource)?;
> > let io = Devres::access(pdev.as_ref())?;
> > 
> > let data = io.read32_relaxed(offset);
> > io.write32_relaxed(data, offset);
> > 
> > Devres::access() is in nova-next and lands in v6.16.
> > 
> > [1] https://gitlab.freedesktop.org/drm/nova/-/commit/f301cb978c068faa8fcd630be2cb317a2d0ec063
> 
> Devres:access takes &Device<Bound>, but the argument in probe() is
> &Device<Core>.
> 
> Are these two types supposed to convert between them? I see no explicit
> function to do so.

Yes, it comes from impl_device_context_deref!() [1], which, as the name implies,
implements the corresponding Deref traits.

Device dereference in the following way:

	&Device<Core> -> &Device<Bound> -> &Device (i.e. &Device<Normal>)

You can just pass in the &Device<Core>, it will work.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n284

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-05-28 18:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-09 20:29 [PATCH v8 0/3] rust: platform: add Io support Daniel Almeida
2025-05-09 20:29 ` [PATCH v8 1/3] rust: io: add resource abstraction Daniel Almeida
2025-05-15 15:53   ` Danilo Krummrich
2025-05-09 20:29 ` [PATCH v8 2/3] rust: io: mem: add a generic iomem abstraction Daniel Almeida
2025-05-15 16:01   ` Danilo Krummrich
2025-05-09 20:29 ` [PATCH v8 3/3] rust: platform: allow ioremap of platform resources Daniel Almeida
2025-05-15 16:23   ` Danilo Krummrich
2025-05-28 17:29     ` Daniel Almeida
2025-05-28 18:06       ` Danilo Krummrich
2025-05-16  8:39   ` Danilo Krummrich
2025-05-15 15:49 ` [PATCH v8 0/3] rust: platform: add Io support Danilo Krummrich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.