Message-Id: <1C6A62A2-B723-4066-9466-C3F0F9FB4B3F@collabora.com>
Date: Wed, 13 Nov 2024 16:42:59 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>
Cc: rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Rob Herring <robh@...nel.org>,
 Daniel Stone <daniels@...labora.com>
Subject: Re: [PATCH v2] rust: platform: add Io support

Hi all, hopefully I did not butcher anything using b4 this time.

See one question from me below.

> On 13 Nov 2024, at 16:30, Daniel Almeida <daniel.almeida@...labora.com> 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 is itself embedded in 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@...labora.com>
> ---
> This series depend on the v3 of PCI/Platform abstractions
> 
> The PCI/Platform abstractions are in-flight and can be found at:
> 
> https://lore.kernel.org/rust-for-linux/Zxili5yze1l5p5GN@pollux/T/#t
> ---
> 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.
> - Link to v1: https://lore.kernel.org/r/20241024-topic-panthor-rs-platform_io_support-v1-1-3d1addd96e30@collabora.com
> ---
> rust/bindings/bindings_helper.h |   1 +
> rust/helpers/io.c               |  22 ++++++
> rust/kernel/platform.rs         | 149 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 172 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 217c776615b9593a2fa921dee130c357dbd96761..90b2d29e7b99f33ceb313b4cc7f8232fef5eed17 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -13,6 +13,7 @@
> #include <linux/errname.h>
> #include <linux/ethtool.h>
> #include <linux/firmware.h>
> +#include <linux/ioport.h>
> #include <linux/jiffies.h>
> #include <linux/mdio.h>
> #include <linux/of_device.h>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index f9bb1bbf1fd5daedc970fc342eeacd8777a8d8ed..0d31552d5f90e69b8132725a958813d4ab8bd9fa 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>
> 
> u8 rust_helper_readb(const volatile void __iomem *addr)
> {
> @@ -89,3 +90,24 @@ 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);
> +}
> +
> +void __iomem *rust_helper_ioremap(resource_size_t addr, unsigned long size)
> +{
> + return ioremap(addr, size);
> +}
> +
> +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 addf5356f44f3cf233503aed97f1aa0d32f4f062..a6e676e0b983fea2dc2090563a138b1cc365a97d 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -4,11 +4,15 @@
> //!
> //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
> 
> +use core::ops::Deref;
> +
> use crate::{
>     bindings, container_of, device,
>     device_id::RawDeviceId,
> +    devres::Devres,
>     driver,
>     error::{to_result, Result},
> +    io::Io,
>     of,
>     prelude::*,
>     str::CStr,
> @@ -208,6 +212,47 @@ 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,
> +        num: u32,
> +        name: &CStr,
> +    ) -> Result<Devres<IoMem<SIZE>>> {
> +        // SAFETY: `res` is guaranteed to be a valid MMIO address and the size
> +        // is given by the kernel as per `self.resource_len()`.
> +        let io = unsafe { IoMem::new(self, num, name) }?;
> +        let devres = Devres::new(self.as_ref(), io, GFP_KERNEL)?;
> +
> +        Ok(devres)
> +    }
> +
> +    /// Maps a platform resource through ioremap().
> +    pub fn ioremap_resource(&self, resource: u32, name: &CStr) -> Result<Devres<IoMem>> {
> +        self.ioremap_resource_sized::<0>(resource, name)
> +    }
> +
> +    /// Returns the resource len for `resource`, if it exists.
> +    pub fn resource_len(&self, resource: u32) -> Result<bindings::resource_size_t> {
> +        match self.resource(resource) {
> +            // SAFETY: if a struct resource* is returned by the kernel, it is valid.
> +            Ok(resource) => Ok(unsafe { bindings::resource_size(resource) }),
> +            Err(e) => Err(e),
> +        }
> +    }
> +
> +    pub(crate) fn resource(&self, resource: u32) -> Result<*mut bindings::resource> {
> +        // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
> +        let resource = unsafe {
> +            bindings::platform_get_resource(self.as_raw(), bindings::IORESOURCE_MEM, resource)
> +        };
> +        if !resource.is_null() {
> +            Ok(resource)
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> }

Should this return an immutable reference instead of a pointer?

Once the pointer is known not to be null, I think it should. So far, this
is the only place in the kernel crate calling `platform_get_resource`, so
there would be no risk of breaking the reference rules and it would make
the API more ergonomic to use.

> 
> impl AsRef<device::Device> for Device {
> @@ -215,3 +260,107 @@ fn as_ref(&self) -> &device::Device {
>         &self.0
>     }
> }
> +
> +/// A I/O-mapped memory region for platform devices.
> +///
> +/// See also [`kernel::pci::Bar`].
> +///
> +/// # Examples
> +///
> +/// ```
> +/// # use kernel::{bindings, c_str, platform};
> +///
> +/// fn probe(pdev: &mut platform::Device, /* ... */) -> Result<()> {
> +///     let offset = 42; // 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, c_str!("my-device"))?;
> +///
> +///     // 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, c_str!("my-device"))?;
> +///
> +///     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<SIZE>);
> +
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    /// Creates a new `IoMem` instance.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must make sure that `paddr` is a valid MMIO address.
> +    unsafe fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
> +        let size = pdev.resource_len(num)?;
> +        if size == 0 {
> +            return Err(ENOMEM);
> +        }
> +
> +        let res = pdev.resource(num)?;
> +
> +        // SAFETY: `res` is guaranteed to be a valid pointer to a `struct
> +        // resource` as per the semantics of `bindings::platform_get_resource()`
> +        let res_start = unsafe { *res }.start;
> +
> +        // SAFETY:
> +        // - `res_start` is guaranteed to be a valid MMIO address.
> +        // - `size` is known not to be zero at this point.
> +        // - `name` is a valid C string.
> +        let mem_region =
> +            unsafe { bindings::request_mem_region(res_start, size, name.as_char_ptr()) };
> +        if mem_region.is_null() {
> +            return Err(EBUSY);
> +        }
> +
> +        // SAFETY:
> +        // - `res_start` is guaranteed to be a valid MMIO address.
> +        // - `size` is known not to be zero at this point.
> +        let addr = unsafe { bindings::ioremap(res_start, size) };
> +        if addr.is_null() {
> +            // SAFETY:
> +            // - `res_start` is guaranteed to be a valid MMIO address.
> +            // - `size` is the same as the one passed to `request_mem_region`.
> +            unsafe { bindings::release_mem_region(res_start, size) };
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
> +        // size `size`.
> +        let io = unsafe { Io::new(addr as usize, size.try_into()?)? };
> +
> +        Ok(IoMem(io))
> +    }
> +}
> +
> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::release_mem_region(self.0.base_addr() as u64, self.0.maxsize() as u64) }
> +
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.0.base_addr() as *mut core::ffi::c_void) }
> +    }
> +}
> +
> +impl<const SIZE: usize> Deref for IoMem<SIZE> {
> +    type Target = Io<SIZE>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
> 
> ---
> base-commit: 2a5c159f49a5801603af03510c7cef89ff4c9850
> change-id: 20241023-topic-panthor-rs-platform_io_support-f97aeb2ea887
> 
> Best regards,
> -- 
> Daniel Almeida <daniel.almeida@...labora.com>
> 

— Daniel

Powered by blists - more mailing lists