Re: [PATCH v10 2/4] rust: io: mem: add a generic iomem abstraction

[Danilo Krummrich <dakr@kernel.org>]

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@collabora.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.