Re: [PATCH v6 07/16] rust: add `io::{Io, IoRaw}` base types

[Asahi Lina]

On 2/3/25 7:20 AM, Danilo Krummrich wrote:
> Hi Lina,
> 
> On Mon, Feb 03, 2025 at 06:19:57AM +0900, Asahi Lina wrote:
> 
>>
>>
>> On 12/17/24 1:20 AM, Danilo Krummrich wrote:
>>> On Thu, Dec 12, 2024 at 05:33:38PM +0100, Danilo Krummrich wrote:
>>>> +/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
>>>> +///
>>>> +/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
>>>> +/// mapping, performing an additional region request etc.
>>>> +///
>>>> +/// # Invariant
>>>> +///
>>>> +/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
>>>> +/// `maxsize`.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```no_run
>>>> +/// # use kernel::{bindings, io::{Io, IoRaw}};
>>>> +/// # use core::ops::Deref;
>>>> +///
>>>> +/// // See also [`pci::Bar`] for a real example.
>>>> +/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
>>>> +///
>>>> +/// impl<const SIZE: usize> IoMem<SIZE> {
>>>> +///     /// # Safety
>>>> +///     ///
>>>> +///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
>>>> +///     /// virtual address space.
>>>> +///     unsafe fn new(paddr: usize) -> Result<Self>{
>>>> +///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
>>>> +///         // valid for `ioremap`.
>>>> +///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE.try_into().unwrap()) };
>>
>> This is a problematic API. ioremap() does not work on some platforms
>> like Apple Silicon. Instead, you have to use ioremap_np() for most devices.
>>
>> Please add a bindings::resource abstraction and use that to construct
>> IoMem. Then, you can check the flags for
>> bindings::IORESOURCE_MEM_NONPOSTED and use the appropriate function,
>> like this:
> 
> This is just a very simplified example of how to use `IoRaw` and `Io` base
> types in the scope of an example section within a doc-comment.
> 
> There is an actual `IoMem` implementation including a struct resource
> abstraction [1] from Daniel though. Maybe you want to have a look at this
> instead.
> 
> [1] https://lore.kernel.org/rust-for-linux/20250130220529.665896-1-daniel.almeida@xxxxxxxxxxxxx/
> 

That's what I get for skimming too much... I thought this had the actual
implementation. Sorry!

>>
>> https://github.com/AsahiLinux/linux/blob/fce34c83f1dca5b10cc2c866fd8832a362de7974/rust/kernel/io_mem.rs#L152
>>
>>
>>>> +///         if addr.is_null() {
>>>> +///             return Err(ENOMEM);
>>>> +///         }
>>>> +///
>>>> +///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
>>>> +///     }
>>>> +/// }
>>>> +///
>>>> +/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
>>>> +///     fn drop(&mut self) {
>>>> +///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
>>>> +///         unsafe { bindings::iounmap(self.0.addr() as _); };
>>>> +///     }
>>>> +/// }
>>>> +///
>>>> +/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
>>>> +///    type Target = Io<SIZE>;
>>>> +///
>>>> +///    fn deref(&self) -> &Self::Target {
>>>> +///         // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
>>>> +///         unsafe { Io::from_raw(&self.0) }
>>>> +///    }
>>>> +/// }
>>>> +///
>>>> +///# fn no_run() -> Result<(), Error> {
>>>> +/// // SAFETY: Invalid usage for example purposes.
>>>> +/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
>>>> +/// iomem.writel(0x42, 0x0);
>>>> +/// assert!(iomem.try_writel(0x42, 0x0).is_ok());
>>>> +/// assert!(iomem.try_writel(0x42, 0x4).is_err());
>>>> +/// # Ok(())
>>>> +/// # }
>>>> +/// ```
>>>> +#[repr(transparent)]
>>>> +pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
> 

~~ Lina