Re: [PATCH v3] rust: platform: add Io support

[Boqun Feng]

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@xxxxxxxxxxxxx>
> ---
> The PCI/Platform abstractions are in-flight and can be found at:
> 
> https://lore.kernel.org/rust-for-linux/20241210224947.23804-1-dakr@xxxxxxxxxx/
> ---
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
>