Re: [PATCH v12 2/3] rust: io: mem: add a generic iomem abstraction

[Alice Ryhl <aliceryhl@google.com>]

On Fri, Jul 04, 2025 at 01:25:27PM -0300, Daniel Almeida wrote:
> Add a generic iomem abstraction to safely read and write ioremapped
> regions. This abstraction requires a previously acquired IoRequest
> instance. This makes it so that both the resource and the device match,
> or, in other words, that the resource is indeed a valid resource for a
> given bound device.
> 
> A subsequent patch will add the ability to retrieve IoRequest instances
> from platform devices.
> 
> 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>

> +    /// ```no_run
> +    /// # use kernel::{bindings, c_str, platform, of, device::Core};
> +    /// # struct SampleDriver;
> +    ///
> +    /// impl platform::Driver for SampleDriver {
> +    ///    # type IdInfo = ();
> +    ///    # const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
> +    ///
> +    ///    fn probe(

When using # to hide lines from the example, it's useful to think about
what's left. The rendered docs will have a weird empty newline at the
beginning and before `fn probe`.

So I would either remove those newlines or just not hide those lines.

> +    /// Same as [`Self::iomap_sized`] but with exclusive access to the
> +    /// underlying region.
> +    ///
> +    /// This uses the
> +    /// [`ioremap()`](https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device)
> +    /// C API.

I would probably format this like this:

/// This uses the [`ioremap()`] C API.
///
/// [`ioremap()`]: https://docs.kernel.org/driver-api/device-io.html#getting-access-to-the-device

> +/// An exclusive memory-mapped IO region.
> +///
> +/// # Invariants
> +///
> +/// - [`ExclusiveIoMem`] has exclusive access to the underlying [`IoMem`].
> +#[pin_data]
> +pub struct ExclusiveIoMem<const SIZE: usize> {

You don't need #[pin_data] if there aren't any pinned fields.

> +    /// The underlying `IoMem` instance.
> +    iomem: IoMem<SIZE>,
> +
> +    /// The region abstraction. This represents exclusive access to the
> +    /// range represented by the underlying `iomem`.
> +    ///
> +    /// This field is needed for ownership of the region.
> +    _region: Region,
> +}
> [..]
> +impl<const SIZE: usize> IoMem<SIZE> {
> +    fn ioremap(resource: &Resource) -> Result<Self> {
> +        let size = resource.size();
> +        if size == 0 {
> +            return Err(EINVAL);
> +        }
> +
> +        let res_start = resource.start();
> +
> +        let addr = if resource
> +            .flags()
> +            .contains(io::resource::flags::IORESOURCE_MEM_NONPOSTED)
> +        {
> +            // SAFETY:
> +            // - `res_start` and `size` are read from a presumably valid `struct resource`.
> +            // - `size` is known not to be zero at this point.
> +            unsafe { bindings::ioremap_np(res_start, size as usize) }

Here you cast from ResourceSize to usize. Are you sure that is correct?
I thought those types could be different.

> +impl<const SIZE: usize> Drop for IoMem<SIZE> {
> +    fn drop(&mut self) {
> +        // SAFETY: Safe as by the invariant of `Io`.
> +        unsafe { bindings::iounmap(self.io.addr() as *mut core::ffi::c_void) }

Just c_void.

Alice