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

[Danilo Krummrich <dakr@kernel.org>]

On Wed, Dec 11, 2024 at 06:00:31PM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> > On 11 Dec 2024, at 15:36, Danilo Krummrich <dakr@kernel.org> wrote:
> >> +///
> >> +///     // 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?

I don't think there will be much boilerplate. I was thinking of one for
io/resource.rs and one for io/mem.rs. Does that make sense?

> 
> > 
> >> +
> >> +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` ?

I was assuming that the reason for non-exclusive access would be that a single
resource contains multiple hardware interfaces, hence multiple mappings.

If we allow partial mappings of the resource and make them exclusive instead it
would be a better solution IMHO.

But this presumes that we don't need non-exclusive access for different reasons.

If not for the reason of having multiple hardware interfaces in the same
resource, which reasons do we have (in Rust) to have multiple mappings of the
same thing?