Message-Id: <A3F6B6C6-33B3-4522-8240-15421F240D3A@collabora.com>
Date: Wed, 11 Dec 2024 18:00:31 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Danilo Krummrich <dakr@...nel.org>
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>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] rust: platform: add Io support

Hi Danilo,

> On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@...nel.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@...labora.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@...nel.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@...labora.com>

Powered by blists - more mailing lists