rust/bindings/bindings_helper.h | 1 + rust/helpers/io.c | 17 ++++ rust/kernel/platform.rs | 209 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 226 insertions(+), 1 deletion(-)
Add support for iomem regions by providing a struct IoMem abstraction
for the platform bus. This will request a memory region and remap it
into a kernel virtual address using ioremap(). The underlying reads and
writes are performed by struct Io, which can be derived from the IoRaw
embedded in platform::IoMem.
This is the equivalent of pci::Bar for the platform bus.
Memory-mapped I/O devices are common, and this patch offers a way to
program them in Rust, usually by reading and writing to their
memory-mapped register set.
Both sized and unsized versions are exposed. Sized allocations use
`ioremap_resource_sized` and specify their size at compile time. Reading
and writing to IoMem is infallible in this case and no extra runtime
checks are performed. Unsized allocations have to check the offset
against the regions's max length at runtime and so return Result.
Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
revoke access to the region if the device is unbound or the underlying
resource goes out of scope.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
The PCI/Platform abstractions are in-flight and can be found at:
https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@kernel.org/
---
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.
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/io.c | 17 ++++
rust/kernel/platform.rs | 209 +++++++++++++++++++++++++++++++++++++++-
3 files changed, 226 insertions(+), 1 deletion(-)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 43f5c381aab0e71051402188ee001aac087dbbca..dd272a8e940a72036b0bf0602e090b3ff9c6baf1 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -21,6 +21,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 1dde6374c0e24f87a73de7b9543bbea8082e22a7..776c71439998119d8c9d14652d070b71a902151f 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,19 @@ 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);
+}
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index a9b7e52591171309c4177bed830fdf2ecf16e518..8fd0a47f097c5822c4e6c40ef75b22c8c60db9c4 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -4,9 +4,14 @@
//!
//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
+use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
+
use crate::{
- bindings, container_of, device, driver,
+ bindings, container_of, device,
+ devres::Devres,
+ driver,
error::{to_result, Result},
+ io::{Io, IoRaw},
of,
prelude::*,
str::CStr,
@@ -184,6 +189,60 @@ fn as_raw(&self) -> *mut bindings::platform_device {
// embedded in `struct platform_device`.
unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
}
+
+ /// Maps a platform resource through ioremap() where the size is known at
+ /// compile time.
+ pub fn ioremap_resource_sized<const SIZE: usize>(
+ &self,
+ resource: &Resource<'_>,
+ exclusive: bool,
+ ) -> Result<Devres<IoMem<SIZE>>> {
+ // SAFETY: We wrap the resulting `IoMem` in a `Devres`.
+ let io = unsafe { IoMem::new(resource, exclusive) }?;
+ let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
+
+ Ok(devres)
+ }
+
+ /// Maps a platform resource through ioremap().
+ pub fn ioremap_resource(
+ &self,
+ resource: &Resource<'_>,
+ exclusive: bool,
+ ) -> Result<Devres<IoMem>> {
+ self.ioremap_resource_sized::<0>(resource, exclusive)
+ }
+
+ /// 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)
+ };
+
+ Some(Resource {
+ inner: NonNull::new(resource)?,
+ _phantom: PhantomData,
+ })
+ }
+
+ /// 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(),
+ )
+ };
+
+ Some(Resource {
+ inner: NonNull::new(resource)?,
+ _phantom: PhantomData,
+ })
+ }
}
impl AsRef<device::Device> for Device {
@@ -191,3 +250,151 @@ fn as_ref(&self) -> &device::Device {
&self.0
}
}
+
+/// A resource associated to a given platform device.
+///
+/// # Invariants
+///
+/// `inner` is a valid pointer to a `struct resource` retrieved from a `struct
+/// platform_device`.
+pub struct Resource<'a> {
+ inner: NonNull<bindings::resource>,
+ _phantom: PhantomData<&'a bindings::resource>,
+}
+
+impl<'a> Resource<'a> {
+ /// Returns the size of the resource.
+ pub fn size(&self) -> bindings::resource_size_t {
+ // SAFETY: safe as per the invariants of `Resource`
+ unsafe { bindings::resource_size(self.inner.as_ptr()) }
+ }
+
+ /// Returns the start address of the resource.
+ pub fn start(&self) -> u64 {
+ let inner = self.inner.as_ptr();
+ // SAFETY: safe as per the invariants of `Resource`
+ unsafe { *inner }.start
+ }
+
+ /// Returns the name of the resource.
+ pub fn name(&self) -> &CStr {
+ let inner = self.inner.as_ptr();
+ // SAFETY: safe as per the invariants of `Resource`
+ unsafe { CStr::from_char_ptr((*inner).name) }
+ }
+}
+
+/// A I/O-mapped memory region for platform devices.
+///
+/// See also [`kernel::pci::Bar`].
+///
+/// # Examples
+///
+/// ```no_run
+/// # use kernel::{bindings, c_str, platform};
+///
+/// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
+/// let offset = 0; // Some offset.
+///
+/// // If the size is known at compile time, use `ioremap_resource_sized`.
+/// // No runtime checks will apply when reading and writing.
+/// let iomem = pdev.ioremap_resource_sized::<42>(0, None)?;
+///
+/// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
+/// // the `Devres` makes sure that the resource is still valid.
+/// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
+///
+/// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
+///
+/// // Unlike `ioremap_resource_sized`, here the size of the memory region
+/// // is not known at compile time, so only the `try_read*` and `try_write*`
+/// // family of functions are exposed, leading to runtime checks on every
+/// // access.
+/// let iomem = pdev.ioremap_resource(0, None)?;
+///
+/// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
+///
+/// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
+///
+/// # Ok::<(), Error>(())
+/// }
+/// ```
+///
+pub struct IoMem<const SIZE: usize = 0> {
+ io: IoRaw<SIZE>,
+ res_start: u64,
+ exclusive: bool,
+}
+
+impl<const SIZE: usize> IoMem<SIZE> {
+ /// Creates a new `IoMem` instance.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that `IoMem` does not outlive the device it is
+ /// associated with, usually by wrapping the `IoMem` in a `Devres`.
+ unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
+ let size = resource.size();
+ if size == 0 {
+ return Err(ENOMEM);
+ }
+
+ let res_start = resource.start();
+
+ // SAFETY:
+ // - `res_start` and `size` are read from a presumably valid `struct resource`.
+ // - `size` is known not to be zero at this point.
+ // - `resource.name()` returns a valid C string.
+ let mem_region =
+ unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
+
+ if mem_region.is_null() {
+ return Err(EBUSY);
+ }
+
+ // SAFETY:
+ // - `res_start` and `size` are read from a presumably valid `struct resource`.
+ // - `size` is known not to be zero at this point.
+ let addr = unsafe { bindings::ioremap(res_start, size as usize) };
+ if addr.is_null() {
+ if exclusive {
+ // SAFETY:
+ // - `res_start` and `size` are read from a presumably valid `struct resource`.
+ // - `size` is the same as the one passed to `request_mem_region`.
+ unsafe { bindings::release_mem_region(res_start, size) };
+ }
+ return Err(ENOMEM);
+ }
+
+ let io = IoRaw::new(addr as usize, size as usize)?;
+
+ Ok(IoMem {
+ io,
+ res_start,
+ exclusive,
+ })
+ }
+}
+
+impl<const SIZE: usize> Drop for IoMem<SIZE> {
+ fn drop(&mut self) {
+ if self.exclusive {
+ // SAFETY: `res_start` and `io.maxsize()` were the values passed to
+ // `request_mem_region`.
+ unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
+ }
+
+ // 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: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
+ // size `maxsize` given the initialization in `Self::new`.
+ unsafe { Io::from_raw(&self.io) }
+ }
+}
---
base-commit: 1a4ce3837e321b94db1ac4274160e449c462610b
change-id: 20241211-topic-panthor-rs-platform_io_support-ae8ac7feca5d
prerequisite-message-id: <20241210224947.23804-1-dakr@kernel.org>
prerequisite-patch-id: 9721d6d91aaa327a64db90153ac973c39d328fcf
prerequisite-patch-id: 678dbd0e4ef70c658ad7d6def3e1fad82ded9657
prerequisite-patch-id: ea80287941ef43f59fa75a28e6798ff10c8e90c1
prerequisite-patch-id: e922cfa34c5e15c904fd12a08de5a5897915dc96
prerequisite-patch-id: cd9756c9586f394e5b39198497979aa1384ad2b8
prerequisite-patch-id: 313083700e67eab809a6b673d1fd835a6bd18625
prerequisite-patch-id: d0c7198d84ffa229221bbe06541136c97e8aae4e
prerequisite-patch-id: 0c4e157879b92f366feca2e89b5719e0a9bfa36a
prerequisite-patch-id: 515464a50ad216e2e9811043db8ca341262db288
prerequisite-patch-id: c50da45a4d7e62930f78b854f9afc636120dc255
prerequisite-patch-id: 5e32316afbfed41fe68cc096bf5a93201d0c65f9
prerequisite-patch-id: 15b08041af5e8f50619e3098b01ac0c0c0357751
prerequisite-patch-id: d680798b48f799b02f2a514675133911af7b4228
prerequisite-patch-id: 833f8f6310401cec79343ad55376e2f00b5105da
prerequisite-patch-id: c7825a4527d051ac9929fa8e8856ec454cc3f703
prerequisite-patch-id: ea5c28331c595ad00929291b483c8848f3ff84cf
Best regards,
--
Daniel Almeida <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 | 17 +++++
rust/kernel/io.rs | 3 +
rust/kernel/io/mem.rs | 108 ++++++++++++++++++++++++++++++++
rust/kernel/io/resource.rs | 49 +++++++++++++++
rust/kernel/platform.rs | 98 ++++++++++++++++++++++++++++-
6 files changed, 275 insertions(+), 1 deletion(-)
create mode 100644 rust/kernel/io/mem.rs
create mode 100644 rust/kernel/io/resource.rs
--
2.47.1
On Wed, Dec 11, 2024 at 02:51:56PM -0300, Daniel Almeida wrote:
> Add support for iomem regions by providing a struct IoMem abstraction
> for the platform bus. This will request a memory region and remap it
> into a kernel virtual address using ioremap(). The underlying reads and
> writes are performed by struct Io, which can be derived from the IoRaw
> embedded in platform::IoMem.
>
> This is the equivalent of pci::Bar for the platform bus.
>
> Memory-mapped I/O devices are common, and this patch offers a way to
> program them in Rust, usually by reading and writing to their
> memory-mapped register set.
>
> Both sized and unsized versions are exposed. Sized allocations use
> `ioremap_resource_sized` and specify their size at compile time. Reading
> and writing to IoMem is infallible in this case and no extra runtime
> checks are performed. Unsized allocations have to check the offset
> against the regions's max length at runtime and so return Result.
>
> Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
> revoke access to the region if the device is unbound or the underlying
> resource goes out of scope.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> The PCI/Platform abstractions are in-flight and can be found at:
>
> https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@kernel.org/
> ---
> 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.
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/io.c | 17 ++++
> rust/kernel/platform.rs | 209 +++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 226 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 43f5c381aab0e71051402188ee001aac087dbbca..dd272a8e940a72036b0bf0602e090b3ff9c6baf1 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -21,6 +21,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 1dde6374c0e24f87a73de7b9543bbea8082e22a7..776c71439998119d8c9d14652d070b71a902151f 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,19 @@ 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);
> +}
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index a9b7e52591171309c4177bed830fdf2ecf16e518..8fd0a47f097c5822c4e6c40ef75b22c8c60db9c4 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -4,9 +4,14 @@
> //!
> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>
> +use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> +
> use crate::{
> - bindings, container_of, device, driver,
> + bindings, container_of, device,
> + devres::Devres,
> + driver,
> error::{to_result, Result},
> + io::{Io, IoRaw},
> of,
> prelude::*,
> str::CStr,
> @@ -184,6 +189,60 @@ fn as_raw(&self) -> *mut bindings::platform_device {
> // embedded in `struct platform_device`.
> unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
> }
> +
> + /// Maps a platform resource through ioremap() where the size is known at
> + /// compile time.
> + pub fn ioremap_resource_sized<const SIZE: usize>(
> + &self,
> + resource: &Resource<'_>,
> + exclusive: bool,
> + ) -> Result<Devres<IoMem<SIZE>>> {
> + // SAFETY: We wrap the resulting `IoMem` in a `Devres`.
> + let io = unsafe { IoMem::new(resource, exclusive) }?;
> + let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> +
> + Ok(devres)
> + }
> +
> + /// Maps a platform resource through ioremap().
> + pub fn ioremap_resource(
> + &self,
> + resource: &Resource<'_>,
> + exclusive: bool,
> + ) -> Result<Devres<IoMem>> {
> + self.ioremap_resource_sized::<0>(resource, exclusive)
> + }
> +
> + /// 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)
> + };
> +
> + Some(Resource {
> + inner: NonNull::new(resource)?,
> + _phantom: PhantomData,
> + })
> + }
> +
> + /// 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(),
> + )
> + };
> +
> + Some(Resource {
> + inner: NonNull::new(resource)?,
> + _phantom: PhantomData,
> + })
> + }
> }
>
> impl AsRef<device::Device> for Device {
> @@ -191,3 +250,151 @@ fn as_ref(&self) -> &device::Device {
> &self.0
> }
> }
> +
> +/// A resource associated to a given platform device.
> +///
> +/// # Invariants
> +///
> +/// `inner` is a valid pointer to a `struct resource` retrieved from a `struct
> +/// platform_device`.
> +pub struct Resource<'a> {
> + inner: NonNull<bindings::resource>,
> + _phantom: PhantomData<&'a bindings::resource>,
> +}
Please don't introduce arbitrary reference/smart-pointer types. If you
follow what Alice suggested, this should be a:
pub struct Resource(Opaque<bindings::resource>);
and you can replace all the `Resource<'a>` in this patch with a `&'a
Resource` after making the above definition. See
`kernel::device::Device` as an example.
Regards,
Boqun
> +
> +impl<'a> Resource<'a> {
> + /// Returns the size of the resource.
> + pub fn size(&self) -> bindings::resource_size_t {
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { bindings::resource_size(self.inner.as_ptr()) }
> + }
> +
> + /// Returns the start address of the resource.
> + pub fn start(&self) -> u64 {
> + let inner = self.inner.as_ptr();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { *inner }.start
> + }
> +
> + /// Returns the name of the resource.
> + pub fn name(&self) -> &CStr {
> + let inner = self.inner.as_ptr();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { CStr::from_char_ptr((*inner).name) }
> + }
> +}
> +
> +/// A I/O-mapped memory region for platform devices.
> +///
> +/// See also [`kernel::pci::Bar`].
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// # use kernel::{bindings, c_str, platform};
> +///
> +/// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
> +/// let offset = 0; // Some offset.
> +///
> +/// // If the size is known at compile time, use `ioremap_resource_sized`.
> +/// // No runtime checks will apply when reading and writing.
> +/// let iomem = pdev.ioremap_resource_sized::<42>(0, None)?;
> +///
> +/// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
> +/// // the `Devres` makes sure that the resource is still valid.
> +/// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
> +///
> +/// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
> +///
> +/// // Unlike `ioremap_resource_sized`, here the size of the memory region
> +/// // is not known at compile time, so only the `try_read*` and `try_write*`
> +/// // family of functions are exposed, leading to runtime checks on every
> +/// // access.
> +/// let iomem = pdev.ioremap_resource(0, None)?;
> +///
> +/// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
> +///
> +/// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
> +///
> +/// # Ok::<(), Error>(())
> +/// }
> +/// ```
> +///
> +pub struct IoMem<const SIZE: usize = 0> {
> + io: IoRaw<SIZE>,
> + res_start: u64,
> + exclusive: bool,
> +}
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> + /// Creates a new `IoMem` instance.
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure that `IoMem` does not outlive the device it is
> + /// associated with, usually by wrapping the `IoMem` in a `Devres`.
> + unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
> + let size = resource.size();
> + if size == 0 {
> + return Err(ENOMEM);
> + }
> +
> + let res_start = resource.start();
> +
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + // - `resource.name()` returns a valid C string.
> + let mem_region =
> + unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
> +
> + if mem_region.is_null() {
> + return Err(EBUSY);
> + }
> +
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + let addr = unsafe { bindings::ioremap(res_start, size as usize) };
> + if addr.is_null() {
> + if exclusive {
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is the same as the one passed to `request_mem_region`.
> + unsafe { bindings::release_mem_region(res_start, size) };
> + }
> + return Err(ENOMEM);
> + }
> +
> + let io = IoRaw::new(addr as usize, size as usize)?;
> +
> + Ok(IoMem {
> + io,
> + res_start,
> + exclusive,
> + })
> + }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> + fn drop(&mut self) {
> + if self.exclusive {
> + // SAFETY: `res_start` and `io.maxsize()` were the values passed to
> + // `request_mem_region`.
> + unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
> + }
> +
> + // 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: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> + // size `maxsize` given the initialization in `Self::new`.
> + unsafe { Io::from_raw(&self.io) }
> + }
> +}
>
> ---
> base-commit: 1a4ce3837e321b94db1ac4274160e449c462610b
> change-id: 20241211-topic-panthor-rs-platform_io_support-ae8ac7feca5d
> prerequisite-message-id: <20241210224947.23804-1-dakr@kernel.org>
> prerequisite-patch-id: 9721d6d91aaa327a64db90153ac973c39d328fcf
> prerequisite-patch-id: 678dbd0e4ef70c658ad7d6def3e1fad82ded9657
> prerequisite-patch-id: ea80287941ef43f59fa75a28e6798ff10c8e90c1
> prerequisite-patch-id: e922cfa34c5e15c904fd12a08de5a5897915dc96
> prerequisite-patch-id: cd9756c9586f394e5b39198497979aa1384ad2b8
> prerequisite-patch-id: 313083700e67eab809a6b673d1fd835a6bd18625
> prerequisite-patch-id: d0c7198d84ffa229221bbe06541136c97e8aae4e
> prerequisite-patch-id: 0c4e157879b92f366feca2e89b5719e0a9bfa36a
> prerequisite-patch-id: 515464a50ad216e2e9811043db8ca341262db288
> prerequisite-patch-id: c50da45a4d7e62930f78b854f9afc636120dc255
> prerequisite-patch-id: 5e32316afbfed41fe68cc096bf5a93201d0c65f9
> prerequisite-patch-id: 15b08041af5e8f50619e3098b01ac0c0c0357751
> prerequisite-patch-id: d680798b48f799b02f2a514675133911af7b4228
> prerequisite-patch-id: 833f8f6310401cec79343ad55376e2f00b5105da
> prerequisite-patch-id: c7825a4527d051ac9929fa8e8856ec454cc3f703
> prerequisite-patch-id: ea5c28331c595ad00929291b483c8848f3ff84cf
>
> Best regards,
> --
> Daniel Almeida <daniel.almeida@collabora.com>
>
On Wed, Dec 11, 2024 at 02:51:56PM -0300, Daniel Almeida wrote:
> Add support for iomem regions by providing a struct IoMem abstraction
> for the platform bus. This will request a memory region and remap it
> into a kernel virtual address using ioremap(). The underlying reads and
> writes are performed by struct Io, which can be derived from the IoRaw
> embedded in platform::IoMem.
>
> This is the equivalent of pci::Bar for the platform bus.
>
> Memory-mapped I/O devices are common, and this patch offers a way to
> program them in Rust, usually by reading and writing to their
> memory-mapped register set.
>
> Both sized and unsized versions are exposed. Sized allocations use
> `ioremap_resource_sized` and specify their size at compile time. Reading
> and writing to IoMem is infallible in this case and no extra runtime
> checks are performed. Unsized allocations have to check the offset
> against the regions's max length at runtime and so return Result.
>
> Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
> revoke access to the region if the device is unbound or the underlying
> resource goes out of scope.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> The PCI/Platform abstractions are in-flight and can be found at:
>
> https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@kernel.org/
> ---
> 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.
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/io.c | 17 ++++
> rust/kernel/platform.rs | 209 +++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 226 insertions(+), 1 deletion(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 43f5c381aab0e71051402188ee001aac087dbbca..dd272a8e940a72036b0bf0602e090b3ff9c6baf1 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -21,6 +21,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 1dde6374c0e24f87a73de7b9543bbea8082e22a7..776c71439998119d8c9d14652d070b71a902151f 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,19 @@ 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);
> +}
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index a9b7e52591171309c4177bed830fdf2ecf16e518..8fd0a47f097c5822c4e6c40ef75b22c8c60db9c4 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -4,9 +4,14 @@
> //!
> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>
> +use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
> +
> use crate::{
> - bindings, container_of, device, driver,
> + bindings, container_of, device,
> + devres::Devres,
> + driver,
> error::{to_result, Result},
> + io::{Io, IoRaw},
> of,
> prelude::*,
> str::CStr,
> @@ -184,6 +189,60 @@ fn as_raw(&self) -> *mut bindings::platform_device {
> // embedded in `struct platform_device`.
> unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
> }
> +
> + /// Maps a platform resource through ioremap() where the size is known at
> + /// compile time.
> + pub fn ioremap_resource_sized<const SIZE: usize>(
> + &self,
> + resource: &Resource<'_>,
> + exclusive: bool,
> + ) -> Result<Devres<IoMem<SIZE>>> {
> + // SAFETY: We wrap the resulting `IoMem` in a `Devres`.
> + let io = unsafe { IoMem::new(resource, exclusive) }?;
> + let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> +
> + Ok(devres)
> + }
> +
> + /// Maps a platform resource through ioremap().
> + pub fn ioremap_resource(
> + &self,
> + resource: &Resource<'_>,
> + exclusive: bool,
> + ) -> Result<Devres<IoMem>> {
> + self.ioremap_resource_sized::<0>(resource, exclusive)
> + }
> +
> + /// 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)
> + };
> +
> + Some(Resource {
> + inner: NonNull::new(resource)?,
> + _phantom: PhantomData,
> + })
> + }
> +
> + /// 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(),
> + )
> + };
> +
> + Some(Resource {
> + inner: NonNull::new(resource)?,
> + _phantom: PhantomData,
> + })
> + }
> }
>
> impl AsRef<device::Device> for Device {
> @@ -191,3 +250,151 @@ fn as_ref(&self) -> &device::Device {
> &self.0
> }
> }
> +
> +/// A resource associated to a given platform device.
> +///
> +/// # Invariants
> +///
> +/// `inner` is a valid pointer to a `struct resource` retrieved from a `struct
> +/// platform_device`.
> +pub struct Resource<'a> {
> + inner: NonNull<bindings::resource>,
> + _phantom: PhantomData<&'a bindings::resource>,
> +}
> +
> +impl<'a> Resource<'a> {
> + /// Returns the size of the resource.
> + pub fn size(&self) -> bindings::resource_size_t {
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { bindings::resource_size(self.inner.as_ptr()) }
> + }
> +
> + /// Returns the start address of the resource.
> + pub fn start(&self) -> u64 {
> + let inner = self.inner.as_ptr();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { *inner }.start
> + }
> +
> + /// Returns the name of the resource.
> + pub fn name(&self) -> &CStr {
> + let inner = self.inner.as_ptr();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { CStr::from_char_ptr((*inner).name) }
> + }
> +}
> +
> +/// A I/O-mapped memory region for platform devices.
> +///
> +/// See also [`kernel::pci::Bar`].
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// # use kernel::{bindings, c_str, platform};
> +///
> +/// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
> +/// let offset = 0; // Some offset.
> +///
> +/// // If the size is known at compile time, use `ioremap_resource_sized`.
> +/// // No runtime checks will apply when reading and writing.
> +/// let iomem = pdev.ioremap_resource_sized::<42>(0, None)?;
This doesn't seem to match the API introduced by this patch. I assume you forgot
to update the example code?
> +///
> +/// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
> +/// // the `Devres` makes sure that the resource is still valid.
> +/// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
> +///
> +/// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
> +///
> +/// // Unlike `ioremap_resource_sized`, here the size of the memory region
> +/// // is not known at compile time, so only the `try_read*` and `try_write*`
> +/// // family of functions are exposed, leading to runtime checks on every
> +/// // access.
> +/// let iomem = pdev.ioremap_resource(0, None)?;
> +///
> +/// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
> +///
> +/// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
> +///
> +/// # Ok::<(), Error>(())
> +/// }
> +/// ```
> +///
> +pub struct IoMem<const SIZE: usize = 0> {
> + io: IoRaw<SIZE>,
> + res_start: u64,
> + exclusive: bool,
> +}
I think both the `Resource` and `IoMem` implementation do not belong into
platform.rs. Neither of those depends on any platform bus structures. They're
only used by platform structures.
I think we should move this into files under rust/kernel/io/ and create separate
commits out of this one.
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> + /// Creates a new `IoMem` instance.
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure that `IoMem` does not outlive the device it is
> + /// associated with, usually by wrapping the `IoMem` in a `Devres`.
More precisely, `Devres` revokes when the device is unbound from the matched
driver, i.e. the driver should not be able to control the device anymore. This
may be much earlier than when the device disappears.
> + unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
> + let size = resource.size();
> + if size == 0 {
> + return Err(ENOMEM);
> + }
> +
> + let res_start = resource.start();
> +
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + // - `resource.name()` returns a valid C string.
> + let mem_region =
> + unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
This should only be called if exclusive == true, right?
Btw. what's the use-case for non-exclusive access? Shouldn't we rather support
partial exclusive mappings?
> +
> + if mem_region.is_null() {
> + return Err(EBUSY);
> + }
> +
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + let addr = unsafe { bindings::ioremap(res_start, size as usize) };
> + if addr.is_null() {
> + if exclusive {
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is the same as the one passed to `request_mem_region`.
> + unsafe { bindings::release_mem_region(res_start, size) };
> + }
> + return Err(ENOMEM);
> + }
> +
> + let io = IoRaw::new(addr as usize, size as usize)?;
> +
> + Ok(IoMem {
> + io,
> + res_start,
> + exclusive,
> + })
> + }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> + fn drop(&mut self) {
> + if self.exclusive {
> + // SAFETY: `res_start` and `io.maxsize()` were the values passed to
> + // `request_mem_region`.
> + unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
> + }
> +
> + // 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: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> + // size `maxsize` given the initialization in `Self::new`.
I think it'd be cleaner to have this as an invariant section in the
documentation of `IoMem` and then refer to that.
> + unsafe { Io::from_raw(&self.io) }
> + }
> +}
>
> ---
> base-commit: 1a4ce3837e321b94db1ac4274160e449c462610b
> change-id: 20241211-topic-panthor-rs-platform_io_support-ae8ac7feca5d
> prerequisite-message-id: <20241210224947.23804-1-dakr@kernel.org>
> prerequisite-patch-id: 9721d6d91aaa327a64db90153ac973c39d328fcf
> prerequisite-patch-id: 678dbd0e4ef70c658ad7d6def3e1fad82ded9657
> prerequisite-patch-id: ea80287941ef43f59fa75a28e6798ff10c8e90c1
> prerequisite-patch-id: e922cfa34c5e15c904fd12a08de5a5897915dc96
> prerequisite-patch-id: cd9756c9586f394e5b39198497979aa1384ad2b8
> prerequisite-patch-id: 313083700e67eab809a6b673d1fd835a6bd18625
> prerequisite-patch-id: d0c7198d84ffa229221bbe06541136c97e8aae4e
> prerequisite-patch-id: 0c4e157879b92f366feca2e89b5719e0a9bfa36a
> prerequisite-patch-id: 515464a50ad216e2e9811043db8ca341262db288
> prerequisite-patch-id: c50da45a4d7e62930f78b854f9afc636120dc255
> prerequisite-patch-id: 5e32316afbfed41fe68cc096bf5a93201d0c65f9
> prerequisite-patch-id: 15b08041af5e8f50619e3098b01ac0c0c0357751
> prerequisite-patch-id: d680798b48f799b02f2a514675133911af7b4228
> prerequisite-patch-id: 833f8f6310401cec79343ad55376e2f00b5105da
> prerequisite-patch-id: c7825a4527d051ac9929fa8e8856ec454cc3f703
> prerequisite-patch-id: ea5c28331c595ad00929291b483c8848f3ff84cf
>
> Best regards,
> --
> Daniel Almeida <daniel.almeida@collabora.com>
>
Hi Danilo,
> On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Dec 11, 2024 at 02:51:56PM -0300, Daniel Almeida wrote:
>> Add support for iomem regions by providing a struct IoMem abstraction
>> for the platform bus. This will request a memory region and remap it
>> into a kernel virtual address using ioremap(). The underlying reads and
>> writes are performed by struct Io, which can be derived from the IoRaw
>> embedded in platform::IoMem.
>>
>> This is the equivalent of pci::Bar for the platform bus.
>>
>> Memory-mapped I/O devices are common, and this patch offers a way to
>> program them in Rust, usually by reading and writing to their
>> memory-mapped register set.
>>
>> Both sized and unsized versions are exposed. Sized allocations use
>> `ioremap_resource_sized` and specify their size at compile time. Reading
>> and writing to IoMem is infallible in this case and no extra runtime
>> checks are performed. Unsized allocations have to check the offset
>> against the regions's max length at runtime and so return Result.
>>
>> Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
>> revoke access to the region if the device is unbound or the underlying
>> resource goes out of scope.
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> The PCI/Platform abstractions are in-flight and can be found at:
>>
>> https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@kernel.org/
>> ---
>> 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.
>> ---
>> rust/bindings/bindings_helper.h | 1 +
>> rust/helpers/io.c | 17 ++++
>> rust/kernel/platform.rs | 209 +++++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 226 insertions(+), 1 deletion(-)
>>
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 43f5c381aab0e71051402188ee001aac087dbbca..dd272a8e940a72036b0bf0602e090b3ff9c6baf1 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -21,6 +21,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 1dde6374c0e24f87a73de7b9543bbea8082e22a7..776c71439998119d8c9d14652d070b71a902151f 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,19 @@ 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);
>> +}
>> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
>> index a9b7e52591171309c4177bed830fdf2ecf16e518..8fd0a47f097c5822c4e6c40ef75b22c8c60db9c4 100644
>> --- a/rust/kernel/platform.rs
>> +++ b/rust/kernel/platform.rs
>> @@ -4,9 +4,14 @@
>> //!
>> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
>>
>> +use core::{marker::PhantomData, ops::Deref, ptr::NonNull};
>> +
>> use crate::{
>> - bindings, container_of, device, driver,
>> + bindings, container_of, device,
>> + devres::Devres,
>> + driver,
>> error::{to_result, Result},
>> + io::{Io, IoRaw},
>> of,
>> prelude::*,
>> str::CStr,
>> @@ -184,6 +189,60 @@ fn as_raw(&self) -> *mut bindings::platform_device {
>> // embedded in `struct platform_device`.
>> unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
>> }
>> +
>> + /// Maps a platform resource through ioremap() where the size is known at
>> + /// compile time.
>> + pub fn ioremap_resource_sized<const SIZE: usize>(
>> + &self,
>> + resource: &Resource<'_>,
>> + exclusive: bool,
>> + ) -> Result<Devres<IoMem<SIZE>>> {
>> + // SAFETY: We wrap the resulting `IoMem` in a `Devres`.
>> + let io = unsafe { IoMem::new(resource, exclusive) }?;
>> + let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
>> +
>> + Ok(devres)
>> + }
>> +
>> + /// Maps a platform resource through ioremap().
>> + pub fn ioremap_resource(
>> + &self,
>> + resource: &Resource<'_>,
>> + exclusive: bool,
>> + ) -> Result<Devres<IoMem>> {
>> + self.ioremap_resource_sized::<0>(resource, exclusive)
>> + }
>> +
>> + /// 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)
>> + };
>> +
>> + Some(Resource {
>> + inner: NonNull::new(resource)?,
>> + _phantom: PhantomData,
>> + })
>> + }
>> +
>> + /// 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(),
>> + )
>> + };
>> +
>> + Some(Resource {
>> + inner: NonNull::new(resource)?,
>> + _phantom: PhantomData,
>> + })
>> + }
>> }
>>
>> impl AsRef<device::Device> for Device {
>> @@ -191,3 +250,151 @@ fn as_ref(&self) -> &device::Device {
>> &self.0
>> }
>> }
>> +
>> +/// A resource associated to a given platform device.
>> +///
>> +/// # Invariants
>> +///
>> +/// `inner` is a valid pointer to a `struct resource` retrieved from a `struct
>> +/// platform_device`.
>> +pub struct Resource<'a> {
>> + inner: NonNull<bindings::resource>,
>> + _phantom: PhantomData<&'a bindings::resource>,
>> +}
>> +
>> +impl<'a> Resource<'a> {
>> + /// Returns the size of the resource.
>> + pub fn size(&self) -> bindings::resource_size_t {
>> + // SAFETY: safe as per the invariants of `Resource`
>> + unsafe { bindings::resource_size(self.inner.as_ptr()) }
>> + }
>> +
>> + /// Returns the start address of the resource.
>> + pub fn start(&self) -> u64 {
>> + let inner = self.inner.as_ptr();
>> + // SAFETY: safe as per the invariants of `Resource`
>> + unsafe { *inner }.start
>> + }
>> +
>> + /// Returns the name of the resource.
>> + pub fn name(&self) -> &CStr {
>> + let inner = self.inner.as_ptr();
>> + // SAFETY: safe as per the invariants of `Resource`
>> + unsafe { CStr::from_char_ptr((*inner).name) }
>> + }
>> +}
>> +
>> +/// A I/O-mapped memory region for platform devices.
>> +///
>> +/// See also [`kernel::pci::Bar`].
>> +///
>> +/// # Examples
>> +///
>> +/// ```no_run
>> +/// # use kernel::{bindings, c_str, platform};
>> +///
>> +/// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
>> +/// let offset = 0; // Some offset.
>> +///
>> +/// // If the size is known at compile time, use `ioremap_resource_sized`.
>> +/// // No runtime checks will apply when reading and writing.
>> +/// let iomem = pdev.ioremap_resource_sized::<42>(0, None)?;
>
> This doesn't seem to match the API introduced by this patch. I assume you forgot
> to update the example code?
Yes.
>
>> +///
>> +/// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
>> +/// // the `Devres` makes sure that the resource is still valid.
>> +/// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
>> +///
>> +/// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
>> +///
>> +/// // Unlike `ioremap_resource_sized`, here the size of the memory region
>> +/// // is not known at compile time, so only the `try_read*` and `try_write*`
>> +/// // family of functions are exposed, leading to runtime checks on every
>> +/// // access.
>> +/// let iomem = pdev.ioremap_resource(0, None)?;
>> +///
>> +/// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
>> +///
>> +/// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
>> +///
>> +/// # Ok::<(), Error>(())
>> +/// }
>> +/// ```
>> +///
>> +pub struct IoMem<const SIZE: usize = 0> {
>> + io: IoRaw<SIZE>,
>> + res_start: u64,
>> + exclusive: bool,
>> +}
>
> I think both the `Resource` and `IoMem` implementation do not belong into
> platform.rs. Neither of those depends on any platform bus structures. They're
> only used by platform structures.
>
> I think we should move this into files under rust/kernel/io/ and create separate
> commits out of this one.
Just to be clear, one commit with the boilerplate to create rust/kernel/io, and another one with
kernel::io::Resource and kernel::io::IoMem?
>
>> +
>> +impl<const SIZE: usize> IoMem<SIZE> {
>> + /// Creates a new `IoMem` instance.
>> + ///
>> + /// # Safety
>> + ///
>> + /// The caller must ensure that `IoMem` does not outlive the device it is
>> + /// associated with, usually by wrapping the `IoMem` in a `Devres`.
>
> More precisely, `Devres` revokes when the device is unbound from the matched
> driver, i.e. the driver should not be able to control the device anymore. This
> may be much earlier than when the device disappears.
>
>> + unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
>> + let size = resource.size();
>> + if size == 0 {
>> + return Err(ENOMEM);
>> + }
>> +
>> + let res_start = resource.start();
>> +
>> + // SAFETY:
>> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
>> + // - `size` is known not to be zero at this point.
>> + // - `resource.name()` returns a valid C string.
>> + let mem_region =
>> + unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
>
> This should only be called if exclusive == true, right?
Yes (oops)
>
> Btw. what's the use-case for non-exclusive access? Shouldn't we rather support
> partial exclusive mappings?
Rob pointed out that lots of drivers do not call `request_mem_region` in his review for v2, which
Is why I added support for non-exclusive access.
What do you mean by `partial exclusive mappings` ?
>
>> +
>> + if mem_region.is_null() {
>> + return Err(EBUSY);
>> + }
>> +
>> + // SAFETY:
>> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
>> + // - `size` is known not to be zero at this point.
>> + let addr = unsafe { bindings::ioremap(res_start, size as usize) };
>> + if addr.is_null() {
>> + if exclusive {
>> + // SAFETY:
>> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
>> + // - `size` is the same as the one passed to `request_mem_region`.
>> + unsafe { bindings::release_mem_region(res_start, size) };
>> + }
>> + return Err(ENOMEM);
>> + }
>> +
>> + let io = IoRaw::new(addr as usize, size as usize)?;
>> +
>> + Ok(IoMem {
>> + io,
>> + res_start,
>> + exclusive,
>> + })
>> + }
>> +}
>> +
>> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
>> + fn drop(&mut self) {
>> + if self.exclusive {
>> + // SAFETY: `res_start` and `io.maxsize()` were the values passed to
>> + // `request_mem_region`.
>> + unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
>> + }
>> +
>> + // 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: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
>> + // size `maxsize` given the initialization in `Self::new`.
>
> I think it'd be cleaner to have this as an invariant section in the
> documentation of `IoMem` and then refer to that.
>
>> + unsafe { Io::from_raw(&self.io) }
>> + }
>> +}
>>
>> ---
>> base-commit: 1a4ce3837e321b94db1ac4274160e449c462610b
>> change-id: 20241211-topic-panthor-rs-platform_io_support-ae8ac7feca5d
>> prerequisite-message-id: <20241210224947.23804-1-dakr@kernel.org>
>> prerequisite-patch-id: 9721d6d91aaa327a64db90153ac973c39d328fcf
>> prerequisite-patch-id: 678dbd0e4ef70c658ad7d6def3e1fad82ded9657
>> prerequisite-patch-id: ea80287941ef43f59fa75a28e6798ff10c8e90c1
>> prerequisite-patch-id: e922cfa34c5e15c904fd12a08de5a5897915dc96
>> prerequisite-patch-id: cd9756c9586f394e5b39198497979aa1384ad2b8
>> prerequisite-patch-id: 313083700e67eab809a6b673d1fd835a6bd18625
>> prerequisite-patch-id: d0c7198d84ffa229221bbe06541136c97e8aae4e
>> prerequisite-patch-id: 0c4e157879b92f366feca2e89b5719e0a9bfa36a
>> prerequisite-patch-id: 515464a50ad216e2e9811043db8ca341262db288
>> prerequisite-patch-id: c50da45a4d7e62930f78b854f9afc636120dc255
>> prerequisite-patch-id: 5e32316afbfed41fe68cc096bf5a93201d0c65f9
>> prerequisite-patch-id: 15b08041af5e8f50619e3098b01ac0c0c0357751
>> prerequisite-patch-id: d680798b48f799b02f2a514675133911af7b4228
>> prerequisite-patch-id: 833f8f6310401cec79343ad55376e2f00b5105da
>> prerequisite-patch-id: c7825a4527d051ac9929fa8e8856ec454cc3f703
>> prerequisite-patch-id: ea5c28331c595ad00929291b483c8848f3ff84cf
>>
>> Best regards,
>> --
>> Daniel Almeida <daniel.almeida@collabora.com>
On Wed, Dec 11, 2024 at 06:00:31PM -0300, Daniel Almeida wrote:
> Hi Danilo,
>
> > On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, Dec 11, 2024 at 02:51:56PM -0300, Daniel Almeida wrote:
> >> Add support for iomem regions by providing a struct IoMem abstraction
> >> for the platform bus. This will request a memory region and remap it
> >> into a kernel virtual address using ioremap(). The underlying reads and
> >> writes are performed by struct Io, which can be derived from the IoRaw
> >> embedded in platform::IoMem.
> >>
> >> This is the equivalent of pci::Bar for the platform bus.
> >>
> >> Memory-mapped I/O devices are common, and this patch offers a way to
> >> program them in Rust, usually by reading and writing to their
> >> memory-mapped register set.
> >>
> >> Both sized and unsized versions are exposed. Sized allocations use
> >> `ioremap_resource_sized` and specify their size at compile time. Reading
> >> and writing to IoMem is infallible in this case and no extra runtime
> >> checks are performed. Unsized allocations have to check the offset
> >> against the regions's max length at runtime and so return Result.
> >>
> >> Lastly, like pci::Bar, platform::IoMem uses the Devres abstraction to
> >> revoke access to the region if the device is unbound or the underlying
> >> resource goes out of scope.
> >>
> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> >> ---
> >> The PCI/Platform abstractions are in-flight and can be found at:
> >>
> >> https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@kernel.org/
> >> ---
> >> 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.
> >> ---
> >> rust/bindings/bindings_helper.h | 1 +
> >> rust/helpers/io.c | 17 ++++
> >> rust/kernel/platform.rs | 209 +++++++++++++++++++++++++++++++++++++++-
> >> 3 files changed, 226 insertions(+), 1 deletion(-)
[...]
> >> + unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
> >> + let size = resource.size();
> >> + if size == 0 {
> >> + return Err(ENOMEM);
> >> + }
> >> +
> >> + let res_start = resource.start();
> >> +
> >> + // SAFETY:
> >> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> >> + // - `size` is known not to be zero at this point.
> >> + // - `resource.name()` returns a valid C string.
> >> + let mem_region =
> >> + unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
> >
> > This should only be called if exclusive == true, right?
>
> Yes (oops)
>
> >
> > Btw. what's the use-case for non-exclusive access? Shouldn't we rather support
> > partial exclusive mappings?
>
> Rob pointed out that lots of drivers do not call `request_mem_region` in his review for v2, which
> Is why I added support for non-exclusive access.
>
> What do you mean by `partial exclusive mappings` ?
I dug into the history of this some and I may be misremembering where
the problem is exactly. The issue for DT is we can't call
platform_device_add() because it calls insert_resource() and that may
fail. Now I'm not sure if the drivers in overlapping case have to avoid
calling request_mem_region(). I think so...
Here's some relevant commits:
8171d5f7bf26 Revert "of/platform: Use platform_device interface"
b6d2233f2916 of/platform: Use platform_device interface
02bbde7849e6 Revert "of: use platform_device_add"
aac73f34542b of: use platform_device_add
There was another attempt to address this here[1], but I don't think
anything ever landed.
Grant mentioned mpc5200 ethernet and mdio as one example overlapping.
Indeed it does:
ethernet@3000 {
compatible = "fsl,mpc5200-fec";
reg = <0x3000 0x400>;
local-mac-address = [ 00 00 00 00 00 00 ];
interrupts = <2 5 0>;
phy-handle = <&phy0>;
};
mdio@3000 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "fsl,mpc5200-mdio";
reg = <0x3000 0x400>; // fec range, since we need to setup fec interrupts
interrupts = <2 5 0>; // these are for "mii command finished", not link changes & co.
phy0: ethernet-phy@0 {
reg = <0>;
};
};
The FEC driver does request_mem_region(), but the MDIO driver does not.
Rob
[1] https://lore.kernel.org/all/20150607140138.026C4C412C8@trevor.secretlab.ca/
On Wed, Dec 11, 2024 at 06:00:31PM -0300, Daniel Almeida wrote:
> Hi Danilo,
>
> > On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@kernel.org> wrote:
> >> +///
> >> +/// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
> >> +/// // the `Devres` makes sure that the resource is still valid.
> >> +/// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
> >> +///
> >> +/// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
> >> +///
> >> +/// // Unlike `ioremap_resource_sized`, here the size of the memory region
> >> +/// // is not known at compile time, so only the `try_read*` and `try_write*`
> >> +/// // family of functions are exposed, leading to runtime checks on every
> >> +/// // access.
> >> +/// let iomem = pdev.ioremap_resource(0, None)?;
> >> +///
> >> +/// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
> >> +///
> >> +/// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
> >> +///
> >> +/// # Ok::<(), Error>(())
> >> +/// }
> >> +/// ```
> >> +///
> >> +pub struct IoMem<const SIZE: usize = 0> {
> >> + io: IoRaw<SIZE>,
> >> + res_start: u64,
> >> + exclusive: bool,
> >> +}
> >
> > I think both the `Resource` and `IoMem` implementation do not belong into
> > platform.rs. Neither of those depends on any platform bus structures. They're
> > only used by platform structures.
> >
> > I think we should move this into files under rust/kernel/io/ and create separate
> > commits out of this one.
>
> Just to be clear, one commit with the boilerplate to create rust/kernel/io, and another one with
> kernel::io::Resource and kernel::io::IoMem?
I don't think there will be much boilerplate. I was thinking of one for
io/resource.rs and one for io/mem.rs. Does that make sense?
>
> >
> >> +
> >> +impl<const SIZE: usize> IoMem<SIZE> {
> >> + /// Creates a new `IoMem` instance.
> >> + ///
> >> + /// # Safety
> >> + ///
> >> + /// The caller must ensure that `IoMem` does not outlive the device it is
> >> + /// associated with, usually by wrapping the `IoMem` in a `Devres`.
> >
> > More precisely, `Devres` revokes when the device is unbound from the matched
> > driver, i.e. the driver should not be able to control the device anymore. This
> > may be much earlier than when the device disappears.
> >
> >> + unsafe fn new(resource: &Resource<'_>, exclusive: bool) -> Result<Self> {
> >> + let size = resource.size();
> >> + if size == 0 {
> >> + return Err(ENOMEM);
> >> + }
> >> +
> >> + let res_start = resource.start();
> >> +
> >> + // SAFETY:
> >> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> >> + // - `size` is known not to be zero at this point.
> >> + // - `resource.name()` returns a valid C string.
> >> + let mem_region =
> >> + unsafe { bindings::request_mem_region(res_start, size, resource.name().as_char_ptr()) };
> >
> > This should only be called if exclusive == true, right?
>
> Yes (oops)
>
> >
> > Btw. what's the use-case for non-exclusive access? Shouldn't we rather support
> > partial exclusive mappings?
>
> Rob pointed out that lots of drivers do not call `request_mem_region` in his review for v2, which
> Is why I added support for non-exclusive access.
>
> What do you mean by `partial exclusive mappings` ?
I was assuming that the reason for non-exclusive access would be that a single
resource contains multiple hardware interfaces, hence multiple mappings.
If we allow partial mappings of the resource and make them exclusive instead it
would be a better solution IMHO.
But this presumes that we don't need non-exclusive access for different reasons.
If not for the reason of having multiple hardware interfaces in the same
resource, which reasons do we have (in Rust) to have multiple mappings of the
same thing?
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.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/io.c | 7 +++++
rust/kernel/io.rs | 2 ++
rust/kernel/io/resource.rs | 49 +++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+)
create mode 100644 rust/kernel/io/resource.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index e9fdceb568b8..f9c2eedb5b9b 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -16,6 +16,7 @@
#include <linux/file.h>
#include <linux/firmware.h>
#include <linux/fs.h>
+#include <linux/ioport.h>
#include <linux/jiffies.h>
#include <linux/jump_label.h>
#include <linux/mdio.h>
diff --git a/rust/helpers/io.c b/rust/helpers/io.c
index 4c2401ccd720..3cb47bd01942 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,9 @@ 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);
+}
+
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index d4a73e52e3ee..566d8b177e01 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,8 @@
use crate::error::{code::EINVAL, Result};
use crate::{bindings, build_assert};
+pub mod resource;
+
/// Raw representation of an MMIO region.
///
/// By itself, the existence of an instance of this structure does not provide any guarantees that
diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
new file mode 100644
index 000000000000..84876af1699c
--- /dev/null
+++ b/rust/kernel/io/resource.rs
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstraction for system resources.
+//!
+//! C header: [`include/linux/ioport.h`](../../../../include/linux/ioport.h)
+
+use crate::str::CStr;
+use crate::types::Opaque;
+
+/// A resource abstraction.
+#[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) unsafe fn from_ptr<'a>(ptr: *mut bindings::resource) -> &'a Self {
+ // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
+ unsafe { &mut *ptr.cast() }
+ }
+
+ /// Returns the size of the resource.
+ pub fn size(&self) -> bindings::resource_size_t {
+ let inner = self.0.get();
+ // SAFETY: safe as per the invariants of `Resource`
+ unsafe { bindings::resource_size(inner) }
+ }
+
+ /// Returns the start address of the resource.
+ pub fn start(&self) -> u64 {
+ let inner = self.0.get();
+ // SAFETY: safe as per the invariants of `Resource`
+ unsafe { *inner }.start
+ }
+
+ /// Returns the name of the resource.
+ pub fn name(&self) -> &CStr {
+ let inner = self.0.get();
+ // SAFETY: safe as per the invariants of `Resource`
+ unsafe { CStr::from_char_ptr((*inner).name) }
+ }
+}
--
2.47.1
On 9 Jan 2025, at 14:30, 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.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/io.c | 7 +++++
> rust/kernel/io.rs | 2 ++
> rust/kernel/io/resource.rs | 49 +++++++++++++++++++++++++++++++++
> 4 files changed, 59 insertions(+)
> create mode 100644 rust/kernel/io/resource.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index e9fdceb568b8..f9c2eedb5b9b 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -16,6 +16,7 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/ioport.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> #include <linux/mdio.h>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index 4c2401ccd720..3cb47bd01942 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,9 @@ 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);
> +}
> +
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee..566d8b177e01 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod resource;
> +
> /// Raw representation of an MMIO region.
> ///
> /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
> new file mode 100644
> index 000000000000..84876af1699c
> --- /dev/null
> +++ b/rust/kernel/io/resource.rs
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstraction for system resources.
> +//!
> +//! C header: [`include/linux/ioport.h`](../../../../include/linux/ioport.h)
> +
> +use crate::str::CStr;
> +use crate::types::Opaque;
> +
> +/// A resource abstraction.
> +#[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) unsafe fn from_ptr<'a>(ptr: *mut bindings::resource) -> &'a Self {
> + // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
> + unsafe { &mut *ptr.cast() }
> + }
> +
> + /// Returns the size of the resource.
> + pub fn size(&self) -> bindings::resource_size_t {
> + 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
> + }
`start` and `size` return different types in the rust abstraction but on the C side both are of type `resource_size_t`.
(That’s why I created a type alias depending on the config for my abstraction[0] which I know am rewriting to use this)
[0]: https://lore.kernel.org/rust-for-linux/20250113121620.21598-5-me@kloenk.dev/
Thanks
Fiona
> +
> + /// Returns the name of the resource.
> + pub fn name(&self) -> &CStr {
> + let inner = self.0.get();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { CStr::from_char_ptr((*inner).name) }
> + }
> +}
> --
> 2.47.1
On Thu, Jan 9, 2025 at 2:32 PM Daniel Almeida
<daniel.almeida@collabora.com> 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.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/io.c | 7 +++++
> rust/kernel/io.rs | 2 ++
> rust/kernel/io/resource.rs | 49 +++++++++++++++++++++++++++++++++
> 4 files changed, 59 insertions(+)
> create mode 100644 rust/kernel/io/resource.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index e9fdceb568b8..f9c2eedb5b9b 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -16,6 +16,7 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/ioport.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> #include <linux/mdio.h>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index 4c2401ccd720..3cb47bd01942 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,9 @@ 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);
> +}
> +
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee..566d8b177e01 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod resource;
> +
> /// Raw representation of an MMIO region.
> ///
> /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
> new file mode 100644
> index 000000000000..84876af1699c
> --- /dev/null
> +++ b/rust/kernel/io/resource.rs
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstraction for system resources.
> +//!
> +//! C header: [`include/linux/ioport.h`](../../../../include/linux/ioport.h)
Please use srctree instead of ../../../..
> +use crate::str::CStr;
> +use crate::types::Opaque;
> +
> +/// A resource abstraction.
> +#[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) unsafe fn from_ptr<'a>(ptr: *mut bindings::resource) -> &'a Self {
> + // SAFETY: Self is a transparent wrapper around `Opaque<bindings::resource>`.
> + unsafe { &mut *ptr.cast() }
Please remove the `mut` here.
> + }
> +
> + /// Returns the size of the resource.
> + pub fn size(&self) -> bindings::resource_size_t {
> + let inner = self.0.get();
> + // SAFETY: safe as per the invariants of `Resource`
If you're going to refer to the invariants of Reserve, then it should
have an "# Invariants" section.
> + unsafe { bindings::resource_size(inner) }
> + }
> +
> + /// Returns the start address of the resource.
> + pub fn start(&self) -> u64 {
> + let inner = self.0.get();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { *inner }.start
> + }
> +
> + /// Returns the name of the resource.
> + pub fn name(&self) -> &CStr {
> + let inner = self.0.get();
> + // SAFETY: safe as per the invariants of `Resource`
> + unsafe { CStr::from_char_ptr((*inner).name) }
> + }
> +}
> --
> 2.47.1
>
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 | 10 ++++
rust/kernel/io.rs | 1 +
rust/kernel/io/mem.rs | 108 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 119 insertions(+)
create mode 100644 rust/kernel/io/mem.rs
diff --git a/rust/helpers/io.c b/rust/helpers/io.c
index 3cb47bd01942..cb10060c08ae 100644
--- a/rust/helpers/io.c
+++ b/rust/helpers/io.c
@@ -106,3 +106,13 @@ 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);
+}
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 566d8b177e01..9ce3482b5ecd 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,7 @@
use crate::error::{code::EINVAL, Result};
use crate::{bindings, build_assert};
+pub mod mem;
pub mod resource;
/// Raw representation of an MMIO region.
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
new file mode 100644
index 000000000000..f2147db715bf
--- /dev/null
+++ b/rust/kernel/io/mem.rs
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic memory-mapped IO.
+
+use core::ops::Deref;
+
+use crate::io::resource::Resource;
+use crate::io::Io;
+use crate::io::IoRaw;
+use crate::prelude::*;
+
+/// 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.
+///
+/// Whether `IoMem` represents an exclusive access to the underlying memory
+/// region is determined by the caller at creation time, as overlapping access
+/// may be needed in some cases.
+///
+/// # Invariants
+///
+/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
+/// start of the I/O memory mapped region and its size.
+pub struct IoMem<const SIZE: usize = 0> {
+ io: IoRaw<SIZE>,
+ res_start: u64,
+ exclusive: bool,
+}
+
+impl<const SIZE: usize> IoMem<SIZE> {
+ /// Creates a new `IoMem` instance.
+ ///
+ /// `exclusive` determines whether the memory region should be exclusively
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that the underlying resource remains valid
+ /// throughout the `IoMem`'s lifetime. This is usually done by wrapping the
+ /// `IoMem` in a `Devres` instance, which will properly revoke the access
+ /// when the device is unbound from the matched driver.
+ pub(crate) unsafe fn new(resource: &Resource, exclusive: bool) -> Result<Self> {
+ let size = resource.size();
+ if size == 0 {
+ return Err(ENOMEM);
+ }
+
+ let res_start = resource.start();
+
+ if exclusive {
+ // SAFETY:
+ // - `res_start` and `size` are read from a presumably valid `struct resource`.
+ // - `size` is known not to be zero at this point.
+ // - `resource.name()` returns a valid C string.
+ let mem_region = unsafe {
+ bindings::request_mem_region(res_start, size, resource.name().as_char_ptr())
+ };
+
+ if mem_region.is_null() {
+ return Err(EBUSY);
+ }
+ }
+
+ // SAFETY:
+ // - `res_start` and `size` are read from a presumably valid `struct resource`.
+ // - `size` is known not to be zero at this point.
+ let addr = unsafe { bindings::ioremap(res_start, size as usize) };
+ if addr.is_null() {
+ if exclusive {
+ // SAFETY:
+ // - `res_start` and `size` are read from a presumably valid `struct resource`.
+ // - `size` is the same as the one passed to `request_mem_region`.
+ unsafe { bindings::release_mem_region(res_start, size) };
+ }
+ return Err(ENOMEM);
+ }
+
+ let io = IoRaw::new(addr as usize, size as usize)?;
+
+ Ok(IoMem {
+ io,
+ res_start,
+ exclusive,
+ })
+ }
+}
+
+impl<const SIZE: usize> Drop for IoMem<SIZE> {
+ fn drop(&mut self) {
+ if self.exclusive {
+ // SAFETY: `res_start` and `io.maxsize()` were the values passed to
+ // `request_mem_region`.
+ unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
+ }
+
+ // 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.47.1On Thu, Jan 9, 2025 at 2:32 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Add a generic iomem abstraction to safely read and write ioremapped
> regions.
>
> The reads and writes are done through IoRaw, and are thus checked either
> at compile-time, if the size of the region is known at that point, or at
> runtime otherwise.
>
> Non-exclusive access to the underlying memory region is made possible to
> cater to cases where overlapped regions are unavoidable.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> rust/helpers/io.c | 10 ++++
> rust/kernel/io.rs | 1 +
> rust/kernel/io/mem.rs | 108 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
> create mode 100644 rust/kernel/io/mem.rs
>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index 3cb47bd01942..cb10060c08ae 100644
> --- a/rust/helpers/io.c
> +++ b/rust/helpers/io.c
> @@ -106,3 +106,13 @@ 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);
> +}
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 566d8b177e01..9ce3482b5ecd 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,7 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod mem;
> pub mod resource;
>
> /// Raw representation of an MMIO region.
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> new file mode 100644
> index 000000000000..f2147db715bf
> --- /dev/null
> +++ b/rust/kernel/io/mem.rs
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic memory-mapped IO.
> +
> +use core::ops::Deref;
> +
> +use crate::io::resource::Resource;
> +use crate::io::Io;
> +use crate::io::IoRaw;
> +use crate::prelude::*;
> +
> +/// 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.
> +///
> +/// Whether `IoMem` represents an exclusive access to the underlying memory
> +/// region is determined by the caller at creation time, as overlapping access
> +/// may be needed in some cases.
> +///
> +/// # Invariants
> +///
> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> +/// start of the I/O memory mapped region and its size.
"and its size"? sentence is unclear to me ...
> +pub struct IoMem<const SIZE: usize = 0> {
> + io: IoRaw<SIZE>,
> + res_start: u64,
> + exclusive: bool,
I don't think you need this after the `new` call, so you can remove
the field from the struct.
> +}
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> + /// Creates a new `IoMem` instance.
> + ///
> + /// `exclusive` determines whether the memory region should be exclusively
Incomplete sentence?
> + /// # Safety
> + ///
> + /// The caller must ensure that the underlying resource remains valid
> + /// throughout the `IoMem`'s lifetime. This is usually done by wrapping the
> + /// `IoMem` in a `Devres` instance, which will properly revoke the access
> + /// when the device is unbound from the matched driver.
I know that this is a requirement for IoRaw, but does the
`request_mem_region` call not guarantee that the memory region stays
valid until `drop` runs?
> + pub(crate) unsafe fn new(resource: &Resource, exclusive: bool) -> Result<Self> {
> + let size = resource.size();
> + if size == 0 {
> + return Err(ENOMEM);
> + }
> +
> + let res_start = resource.start();
> +
> + if exclusive {
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + // - `resource.name()` returns a valid C string.
> + let mem_region = unsafe {
> + bindings::request_mem_region(res_start, size, resource.name().as_char_ptr())
> + };
> +
> + if mem_region.is_null() {
> + return Err(EBUSY);
> + }
> + }
> +
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + let addr = unsafe { bindings::ioremap(res_start, size as usize) };
> + if addr.is_null() {
> + if exclusive {
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is the same as the one passed to `request_mem_region`.
> + unsafe { bindings::release_mem_region(res_start, size) };
> + }
> + return Err(ENOMEM);
> + }
> +
> + let io = IoRaw::new(addr as usize, size as usize)?;
> +
> + Ok(IoMem {
> + io,
> + res_start,
> + exclusive,
> + })
> + }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> + fn drop(&mut self) {
> + if self.exclusive {
> + // SAFETY: `res_start` and `io.maxsize()` were the values passed to
> + // `request_mem_region`.
> + unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
> + }
> +
> + // 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.47.1
>
Hi Alice,
>
>> +pub struct IoMem<const SIZE: usize = 0> {
>> + io: IoRaw<SIZE>,
>> + res_start: u64,
>> + exclusive: bool,
>
> I don't think you need this after the `new` call, so you can remove
> the field from the struct.
Remove what?
Both res_start and exclusive are used in IoMem::drop().
>
>> + /// # Safety
>> + ///
>> + /// The caller must ensure that the underlying resource remains valid
>> + /// throughout the `IoMem`'s lifetime. This is usually done by wrapping the
>> + /// `IoMem` in a `Devres` instance, which will properly revoke the access
>> + /// when the device is unbound from the matched driver.
>
> I know that this is a requirement for IoRaw, but does the
> `request_mem_region` call not guarantee that the memory region stays
> valid until `drop` runs?
Hmm, I don’t think so. I’d expect the lifetime of the iomem to be tied to the lifetime
of the device, really. The mapping will probably remain, but other infrastructure will probably
be shutdown, like the clocks or regulators, so accessing the region will probably deadlock in such cases.
Other people feel free to correct me, of course ^
— Daniel
On Thu, Jan 9, 2025 at 4:34 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice,
>
> >
> >> +pub struct IoMem<const SIZE: usize = 0> {
> >> + io: IoRaw<SIZE>,
> >> + res_start: u64,
> >> + exclusive: bool,
> >
> > I don't think you need this after the `new` call, so you can remove
> > the field from the struct.
>
> Remove what?
>
> Both res_start and exclusive are used in IoMem::drop().
Ah ... I don't think exclusive should be a runtime value. I'd probably
have two types or move it to a const generic.
Or if we only need exclusive for now, then just drop exclusive.
> >> + /// # Safety
> >> + ///
> >> + /// The caller must ensure that the underlying resource remains valid
> >> + /// throughout the `IoMem`'s lifetime. This is usually done by wrapping the
> >> + /// `IoMem` in a `Devres` instance, which will properly revoke the access
> >> + /// when the device is unbound from the matched driver.
> >
> > I know that this is a requirement for IoRaw, but does the
> > `request_mem_region` call not guarantee that the memory region stays
> > valid until `drop` runs?
>
> Hmm, I don’t think so. I’d expect the lifetime of the iomem to be tied to the lifetime
> of the device, really. The mapping will probably remain, but other infrastructure will probably
> be shutdown, like the clocks or regulators, so accessing the region will probably deadlock in such cases.
>
> Other people feel free to correct me, of course ^
Ah, okay.
Alice
> > Ah ... I don't think exclusive should be a runtime value. I'd probably > have two types or move it to a const generic. > > Or if we only need exclusive for now, then just drop exclusive. That was something that Rob brought up. Rob, would you be OK with the approach above? — Daniel
Daniel Almeida <daniel.almeida@collabora.com> writes:
> 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 | 10 ++++
> rust/kernel/io.rs | 1 +
> rust/kernel/io/mem.rs | 108 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
> create mode 100644 rust/kernel/io/mem.rs
>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index 3cb47bd01942..cb10060c08ae 100644
> --- a/rust/helpers/io.c
> +++ b/rust/helpers/io.c
> @@ -106,3 +106,13 @@ 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);
> +}
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 566d8b177e01..9ce3482b5ecd 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,7 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod mem;
> pub mod resource;
>
> /// Raw representation of an MMIO region.
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> new file mode 100644
> index 000000000000..f2147db715bf
> --- /dev/null
> +++ b/rust/kernel/io/mem.rs
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic memory-mapped IO.
> +
> +use core::ops::Deref;
> +
> +use crate::io::resource::Resource;
> +use crate::io::Io;
> +use crate::io::IoRaw;
> +use crate::prelude::*;
> +
> +/// 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.
> +///
> +/// Whether `IoMem` represents an exclusive access to the underlying memory
> +/// region is determined by the caller at creation time, as overlapping access
> +/// may be needed in some cases.
> +///
> +/// # Invariants
> +///
> +/// `IoMem` always holds an `IoRaw` inststance that holds a valid pointer to the
> +/// start of the I/O memory mapped region and its size.
> +pub struct IoMem<const SIZE: usize = 0> {
> + io: IoRaw<SIZE>,
> + res_start: u64,
> + exclusive: bool,
> +}
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> + /// Creates a new `IoMem` instance.
> + ///
> + /// `exclusive` determines whether the memory region should be exclusively
> + ///
> + /// # Safety
> + ///
> + /// The caller must ensure that the underlying resource remains valid
> + /// throughout the `IoMem`'s lifetime. This is usually done by wrapping the
> + /// `IoMem` in a `Devres` instance, which will properly revoke the access
> + /// when the device is unbound from the matched driver.
> + pub(crate) unsafe fn new(resource: &Resource, exclusive: bool) -> Result<Self> {
> + let size = resource.size();
> + if size == 0 {
> + return Err(ENOMEM);
> + }
Shouldn't this be EINVAL instead? AFAIR, ENOMEM is typically used for
memory allocation failures or resource exhaustion, whereas EINVAL seems
more appropriate for signaling invalid resource configurations
> +
> + let res_start = resource.start();
> +
> + if exclusive {
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + // - `resource.name()` returns a valid C string.
> + let mem_region = unsafe {
> + bindings::request_mem_region(res_start, size, resource.name().as_char_ptr())
> + };
> +
> + if mem_region.is_null() {
> + return Err(EBUSY);
> + }
> + }
> +
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is known not to be zero at this point.
> + let addr = unsafe { bindings::ioremap(res_start, size as usize) };
> + if addr.is_null() {
> + if exclusive {
> + // SAFETY:
> + // - `res_start` and `size` are read from a presumably valid `struct resource`.
> + // - `size` is the same as the one passed to `request_mem_region`.
> + unsafe { bindings::release_mem_region(res_start, size) };
> + }
> + return Err(ENOMEM);
> + }
> +
> + let io = IoRaw::new(addr as usize, size as usize)?;
> +
> + Ok(IoMem {
> + io,
> + res_start,
> + exclusive,
> + })
> + }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> + fn drop(&mut self) {
> + if self.exclusive {
> + // SAFETY: `res_start` and `io.maxsize()` were the values passed to
> + // `request_mem_region`.
> + unsafe { bindings::release_mem_region(self.res_start, self.io.maxsize() as u64) }
> + }
> +
> + // 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) }
> + }
> +}
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 a 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 | 98 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 97 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 03287794f9d0..9575917a1b2d 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::{
- bindings, container_of, device, driver,
+ bindings, container_of, device,
+ devres::Devres,
+ driver,
error::{to_result, Result},
+ io::{mem::IoMem, resource::Resource},
of,
prelude::*,
str::CStr,
@@ -189,6 +192,99 @@ fn as_raw(&self) -> *mut bindings::platform_device {
// embedded in `struct platform_device`.
unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
}
+
+ /// Maps a platform resource through ioremap() where the size is known at
+ /// compile time.
+ ///
+ /// # Examples
+ ///
+ /// ```no_run
+ /// # use kernel::{bindings, c_str, platform};
+ ///
+ /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
+ /// let offset = 0; // Some offset.
+ ///
+ /// // If the size is known at compile time, use `ioremap_resource_sized`.
+ /// // No runtime checks will apply when reading and writing.
+ /// let resource = pdev.resource(0).ok_or(ENODEV)?;
+ /// let iomem = pdev.ioremap_resource_sized::<42>(&resource, true)?;
+ ///
+ /// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
+ /// // the `Devres` makes sure that the resource is still valid.
+ /// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
+ ///
+ /// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
+ ///
+ /// # Ok::<(), Error>(())
+ /// }
+ /// ```
+ pub fn ioremap_resource_sized<const SIZE: usize>(
+ &self,
+ resource: &Resource,
+ exclusive: bool,
+ ) -> Result<Devres<IoMem<SIZE>>> {
+ // SAFETY: We wrap the resulting `IoMem` in a `Devres`.
+ let io = unsafe { IoMem::new(resource, exclusive) }?;
+ let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
+
+ Ok(devres)
+ }
+
+ /// Maps a platform resource through ioremap().
+ ///
+ /// # Examples
+ ///
+ /// ```no_run
+ /// # use kernel::{bindings, c_str, platform};
+ ///
+ /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
+ /// let offset = 0; // Some offset.
+ ///
+ /// // Unlike `ioremap_resource_sized`, here the size of the memory region
+ /// // is not known at compile time, so only the `try_read*` and `try_write*`
+ /// // family of functions are exposed, leading to runtime checks on every
+ /// // access.
+ /// let iomem = pdev.ioremap_resource(&resource, true)?;
+ ///
+ /// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
+ ///
+ /// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
+ ///
+ /// # Ok::<(), Error>(())
+ /// }
+ /// ```
+ pub fn ioremap_resource(&self, resource: &Resource, exclusive: bool) -> Result<Devres<IoMem>> {
+ self.ioremap_resource_sized::<0>(resource, exclusive)
+ }
+
+ /// 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)
+ };
+
+ // SAFETY: `resource` is a valid pointer to a `struct resource` as
+ // returned by `platform_get_resource`.
+ (!resource.is_null()).then(|| 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(),
+ )
+ };
+
+ // SAFETY: `resource` is a valid pointer to a `struct resource` as
+ // returned by `platform_get_resource`.
+ (!resource.is_null()).then(|| unsafe { Resource::from_ptr(resource) })
+ }
}
impl AsRef<device::Device> for Device {
--
2.47.1On Thu, Jan 09, 2025 at 10:30:55AM -0300, Daniel Almeida wrote:
> 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 a 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 | 98 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 03287794f9d0..9575917a1b2d 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::{
> - bindings, container_of, device, driver,
> + bindings, container_of, device,
> + devres::Devres,
> + driver,
> error::{to_result, Result},
> + io::{mem::IoMem, resource::Resource},
> of,
> prelude::*,
> str::CStr,
> @@ -189,6 +192,99 @@ fn as_raw(&self) -> *mut bindings::platform_device {
> // embedded in `struct platform_device`.
> unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
> }
> +
> + /// Maps a platform resource through ioremap() where the size is known at
> + /// compile time.
> + ///
> + /// # Examples
> + ///
> + /// ```no_run
> + /// # use kernel::{bindings, c_str, platform};
> + ///
> + /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
> + /// let offset = 0; // Some offset.
> + ///
> + /// // If the size is known at compile time, use `ioremap_resource_sized`.
> + /// // No runtime checks will apply when reading and writing.
> + /// let resource = pdev.resource(0).ok_or(ENODEV)?;
> + /// let iomem = pdev.ioremap_resource_sized::<42>(&resource, true)?;
> + ///
> + /// // Read and write a 32-bit value at `offset`. Calling `try_access()` on
> + /// // the `Devres` makes sure that the resource is still valid.
> + /// let data = iomem.try_access().ok_or(ENODEV)?.readl(offset);
> + ///
> + /// iomem.try_access().ok_or(ENODEV)?.writel(data, offset);
> + ///
> + /// # Ok::<(), Error>(())
> + /// }
> + /// ```
> + pub fn ioremap_resource_sized<const SIZE: usize>(
> + &self,
> + resource: &Resource,
> + exclusive: bool,
> + ) -> Result<Devres<IoMem<SIZE>>> {
> + // SAFETY: We wrap the resulting `IoMem` in a `Devres`.
> + let io = unsafe { IoMem::new(resource, exclusive) }?;
If we trust `resource`, then IoMem::new() could already return a `Devres<IoMem>`
avoiding the `unsafe`.
> + let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> +
> + Ok(devres)
> + }
> +
> + /// Maps a platform resource through ioremap().
> + ///
> + /// # Examples
> + ///
> + /// ```no_run
> + /// # use kernel::{bindings, c_str, platform};
> + ///
> + /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
> + /// let offset = 0; // Some offset.
> + ///
> + /// // Unlike `ioremap_resource_sized`, here the size of the memory region
> + /// // is not known at compile time, so only the `try_read*` and `try_write*`
> + /// // family of functions are exposed, leading to runtime checks on every
> + /// // access.
> + /// let iomem = pdev.ioremap_resource(&resource, true)?;
> + ///
> + /// let data = iomem.try_access().ok_or(ENODEV)?.try_readl(offset)?;
> + ///
> + /// iomem.try_access().ok_or(ENODEV)?.try_writel(data, offset)?;
> + ///
> + /// # Ok::<(), Error>(())
> + /// }
> + /// ```
> + pub fn ioremap_resource(&self, resource: &Resource, exclusive: bool) -> Result<Devres<IoMem>> {
> + self.ioremap_resource_sized::<0>(resource, exclusive)
> + }
> +
> + /// 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)
> + };
> +
> + // SAFETY: `resource` is a valid pointer to a `struct resource` as
> + // returned by `platform_get_resource`.
> + (!resource.is_null()).then(|| 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(),
> + )
> + };
> +
> + // SAFETY: `resource` is a valid pointer to a `struct resource` as
> + // returned by `platform_get_resource`.
> + (!resource.is_null()).then(|| unsafe { Resource::from_ptr(resource) })
> + }
> }
>
> impl AsRef<device::Device> for Device {
> --
> 2.47.1
>
On Thu, Jan 9, 2025 at 2:32 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> 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 a some device first.
"a some device" typo?
> + /// ```no_run
> + /// # use kernel::{bindings, c_str, platform};
> + ///
> + /// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
When you use `#` to hide part of the example, you end up with a weird
empty line at the top of the example. Please un-hide the line or
remove the empty line.
> + /// 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)
> + };
> +
> + // SAFETY: `resource` is a valid pointer to a `struct resource` as
> + // returned by `platform_get_resource`.
> + (!resource.is_null()).then(|| unsafe { Resource::from_ptr(resource) })
This is a bit confusing. Perhaps just
if resource.is_null() {
return None;
}
Some(Resource::from_ptr(resource))
Alice
© 2016 - 2025 Red Hat, Inc.