[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aF7g2CpVhMntW7-O@pollux>
Date: Fri, 27 Jun 2025 20:20:08 +0200
From: Danilo Krummrich <dakr@...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>,
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>,
Andrew Morton <akpm@...ux-foundation.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
Ying Huang <huang.ying.caritas@...il.com>,
Benno Lossin <lossin@...nel.org>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v10 2/4] rust: io: mem: add a generic iomem abstraction
On Mon, Jun 23, 2025 at 03:00:59PM -0300, Daniel Almeida 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@...labora.com>
> ---
> rust/helpers/io.c | 5 ++
> rust/kernel/io.rs | 1 +
> rust/kernel/io/mem.rs | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 148 insertions(+)
>
> diff --git a/rust/helpers/io.c b/rust/helpers/io.c
> index 404776cf6717c8570c7600a24712ce6e4623d3c6..c475913c69e647b1042e8e7d66b9148d892947a1 100644
> --- a/rust/helpers/io.c
> +++ b/rust/helpers/io.c
> @@ -8,6 +8,11 @@ void __iomem *rust_helper_ioremap(phys_addr_t offset, size_t size)
> return ioremap(offset, size);
> }
>
> +void __iomem *rust_helper_ioremap_np(phys_addr_t offset, size_t size)
> +{
> + return ioremap_np(offset, size);
> +}
> +
> void rust_helper_iounmap(void __iomem *addr)
> {
> iounmap(addr);
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 7b70d5b5477e57d6d0f24bcd26bd8b0071721bc0..b7fc759f8b5d3c3ac6f33f5a66e9f619c58b7405 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;
>
> pub use resource::Resource;
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..2bd9cf4c4f1a59f027999a6e9a203dc99ad6c003
> --- /dev/null
> +++ b/rust/kernel/io/mem.rs
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic memory-mapped IO.
> +
> +use core::ops::Deref;
> +
> +use crate::device::Bound;
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::io;
> +use crate::io::resource::Region;
> +use crate::io::resource::Resource;
> +use crate::io::Io;
> +use crate::io::IoRaw;
> +use crate::prelude::*;
> +
> +/// An exclusive memory-mapped IO region.
> +///
> +/// # Invariants
> +///
> +/// - [`ExclusiveIoMem`] has exclusive access to the underlying [`IoMem`].
> +pub struct ExclusiveIoMem<const SIZE: usize> {
> + /// The region abstraction. This represents exclusive access to the
> + /// range represented by the underlying `iomem`.
> + ///
> + /// It's placed first to ensure that the region is released before it is
> + /// unmapped as a result of the drop order.
Isn't it more logical the other way around, i.e. first free the resource and
then release the resource guard?
Anyways, I don't think it matters too much, since drop() owns the object and
Devres guarantees that we can't race with another device trying to access the
resource.
Otherwise, I guess you could drop() the object and create a new one right away,
but that doesn't seem to be a problem either for any order?
> + /// This field is needed for ownership of the region.
> + _region: Region,
> + /// The underlying `IoMem` instance.
> + iomem: IoMem<SIZE>,
> +}
> +
> +impl<const SIZE: usize> ExclusiveIoMem<SIZE> {
> + /// Creates a new `ExclusiveIoMem` instance.
> + pub(crate) fn ioremap(resource: &Resource) -> Result<Self> {
> + let iomem = IoMem::ioremap(resource)?;
> +
> + let start = resource.start();
> + let size = resource.size();
> + let name = resource.name();
> +
> + let region = resource
> + .request_region(start, size, name, io::resource::flags::IORESOURCE_MEM)
> + .ok_or(EBUSY)?;
Same here, why do the ioremap() first? I think it logically makes more sense to
first try and get the region.
Powered by blists - more mailing lists