Re: [PATCH v2 07/10] rust: add `io::Io` base type

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jun 19, 2024 at 01:39:53AM +0200, Danilo Krummrich wrote:
> I/O memory is typically either mapped through direct calls to ioremap()
> or subsystem / bus specific ones such as pci_iomap().
> 
> Even though subsystem / bus specific functions to map I/O memory are
> based on ioremap() / iounmap() it is not desirable to re-implement them
> in Rust.

Why not?

> Instead, implement a base type for I/O mapped memory, which generically
> provides the corresponding accessors, such as `Io::readb` or
> `Io:try_readb`.

It provides a subset of the existing accessors, one you might want to
trim down for now, see below...

> +/* io.h */
> +u8 rust_helper_readb(const volatile void __iomem *addr)
> +{
> +	return readb(addr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_readb);

<snip>

You provide wrappers for a subset of what io.h provides, why that
specific subset?

Why not just add what you need, when you need it?  I doubt you need all
of these, and odds are you will need more.

> +u32 rust_helper_readl_relaxed(const volatile void __iomem *addr)
> +{
> +	return readl_relaxed(addr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_readl_relaxed);

I know everyone complains about wrapper functions around inline
functions, so I'll just say it again, this is horrid.  And it's going to
hurt performance, so any rust code people write is not on a level
playing field here.

Your call, but ick...

> +#ifdef CONFIG_64BIT
> +u64 rust_helper_readq_relaxed(const volatile void __iomem *addr)
> +{
> +	return readq_relaxed(addr);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_readq_relaxed);
> +#endif

Rust works on 32bit targets in the kernel now?

> +macro_rules! define_read {
> +    ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
> +        /// Read IO data from a given offset known at compile time.
> +        ///
> +        /// Bound checks are performed on compile time, hence if the offset is not known at compile
> +        /// time, the build will fail.

offsets aren't know at compile time for many implementations, as it
could be a dynamically allocated memory range.  How is this going to
work for that?  Heck, how does this work for DT-defined memory ranges
today?

thanks,

greg k-h