Message-ID: <CAL_JsqKfUNdS+iPMfb6is9hA-U-CD+4iG2LYuk7=p8eVH5CoMw@mail.gmail.com>
Date: Tue, 26 Nov 2024 14:13:13 -0600
From: Rob Herring <robh@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: 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>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Boris Brezillon <boris.brezillon@...labora.com>, Daniel Stone <daniels@...labora.com>
Subject: Re: [PATCH v2] rust: platform: add Io support

On Wed, Nov 13, 2024 at 1:32 PM 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>> {

For DT, a resource is retrieved by index or name (matching
'reg-names'). Both ways are equally used. That's not how you are using
name here, but I think you should. I think both name and resource
should be optional (but one must be present).

> +        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)
> +        }
> +    }
>  }
>
>  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"))?;

"my-device" is just some made up name. I think we want this to be
either a firmware defined name or nothing. We don't need driver
authors making up more names (they already make up the firmware names
when they are present).

> +///
> +///     // 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);

Shouldn't this fail? If the size is 42 bytes, then reading from offset
42 is out of bounds. It is also unaligned which wouldn't work on some
architectures (Arm).

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

How is that possible? The address comes from firmware and could be
complete garbage.

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

This will fail on some DT systems. I can't tell you which ones though
as that information was passed down to me on stone tablets. The
problem is there can be 2 nodes with the same addresses and the 2nd
request will fail. That is discouraged, but there are no tools to find
and detect this. That said, the combination of nodes with duplicate
addresses and a rust driver may be rare enough we can just ignore that
problem. OTOH, many, many drivers just don't ever call
request_mem_region().

I would just use res->name for the name here. For DT, that's either
the 'reg-names' value or the node name.

Rob

Powered by blists - more mailing lists