[PATCH] rust: io: rename `io::Io` accessors

[PATCH] rust: io: rename `io::Io` accessors

[Fiona Behrens]

Rename the I/O accessors provided by `Io` to encode the type as
number instead of letter. This is in preparation for Port I/O support
to use a trait for generic accessors.

Add a `c_fn` argument to the accessor generation macro to translate
between rust and C names.

Suggested-by: Danilo Krummrich <dakr@kernel.org>
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/PIO.20support/near/499460541
Signed-off-by: Fiona Behrens <me@kloenk.dev>
---
 rust/kernel/io.rs               | 66 ++++++++++++++++++++---------------------
 samples/rust/rust_driver_pci.rs | 12 ++++----
 2 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index d4a73e52e3ee68f7b558749ed0108acde92ae5fe..72d80a6f131e3e826ecd9d2c3bcf54e89aa60cc3 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -98,9 +98,9 @@ pub fn maxsize(&self) -> usize {
 ///# 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());
+/// iomem.write32(0x42, 0x0);
+/// assert!(iomem.try_write32(0x42, 0x0).is_ok());
+/// assert!(iomem.try_write32(0x42, 0x4).is_err());
 /// # Ok(())
 /// # }
 /// ```
@@ -108,7 +108,7 @@ pub fn maxsize(&self) -> usize {
 pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
 
 macro_rules! define_read {
-    ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
+    ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn: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
@@ -119,7 +119,7 @@ pub fn $name(&self, offset: usize) -> $type_name {
             let addr = self.io_addr_assert::<$type_name>(offset);
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            unsafe { bindings::$name(addr as _) }
+            unsafe { bindings::$c_fn(addr as _) }
         }
 
         /// Read IO data from a given offset.
@@ -131,13 +131,13 @@ pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
             let addr = self.io_addr::<$type_name>(offset)?;
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            Ok(unsafe { bindings::$name(addr as _) })
+            Ok(unsafe { bindings::$c_fn(addr as _) })
         }
     };
 }
 
 macro_rules! define_write {
-    ($(#[$attr:meta])* $name:ident, $try_name:ident, $type_name:ty) => {
+    ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident <- $type_name:ty) => {
         /// Write 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
@@ -148,7 +148,7 @@ pub fn $name(&self, value: $type_name, offset: usize) {
             let addr = self.io_addr_assert::<$type_name>(offset);
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            unsafe { bindings::$name(value, addr as _, ) }
+            unsafe { bindings::$c_fn(value, addr as _, ) }
         }
 
         /// Write IO data from a given offset.
@@ -160,7 +160,7 @@ pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
             let addr = self.io_addr::<$type_name>(offset)?;
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            unsafe { bindings::$name(value, addr as _) }
+            unsafe { bindings::$c_fn(value, addr as _) }
             Ok(())
         }
     };
@@ -218,43 +218,43 @@ fn io_addr_assert<U>(&self, offset: usize) -> usize {
         self.addr() + offset
     }
 
-    define_read!(readb, try_readb, u8);
-    define_read!(readw, try_readw, u16);
-    define_read!(readl, try_readl, u32);
+    define_read!(read8, try_read8, readb -> u8);
+    define_read!(read16, try_read16, readw -> u16);
+    define_read!(read32, try_read32, readl -> u32);
     define_read!(
         #[cfg(CONFIG_64BIT)]
-        readq,
-        try_readq,
-        u64
+        read64,
+        try_read64,
+        readq -> u64
     );
 
-    define_read!(readb_relaxed, try_readb_relaxed, u8);
-    define_read!(readw_relaxed, try_readw_relaxed, u16);
-    define_read!(readl_relaxed, try_readl_relaxed, u32);
+    define_read!(read8_relaxed, try_read8_relaxed, readb_relaxed -> u8);
+    define_read!(read16_relaxed, try_read16_relaxed, readw_relaxed -> u16);
+    define_read!(read32_relaxed, try_read32_relaxed, readl_relaxed -> u32);
     define_read!(
         #[cfg(CONFIG_64BIT)]
-        readq_relaxed,
-        try_readq_relaxed,
-        u64
+        read64_relaxed,
+        try_read64_relaxed,
+        readq_relaxed -> u64
     );
 
-    define_write!(writeb, try_writeb, u8);
-    define_write!(writew, try_writew, u16);
-    define_write!(writel, try_writel, u32);
+    define_write!(write8, try_write8, writeb <- u8);
+    define_write!(write16, try_write16, writew <- u16);
+    define_write!(write32, try_write32, writel <- u32);
     define_write!(
         #[cfg(CONFIG_64BIT)]
-        writeq,
-        try_writeq,
-        u64
+        write64,
+        try_write64,
+        writeq <- u64
     );
 
-    define_write!(writeb_relaxed, try_writeb_relaxed, u8);
-    define_write!(writew_relaxed, try_writew_relaxed, u16);
-    define_write!(writel_relaxed, try_writel_relaxed, u32);
+    define_write!(write8_relaxed, try_write8_relaxed, writeb_relaxed <- u8);
+    define_write!(write16_relaxed, try_write16_relaxed, writew_relaxed <- u16);
+    define_write!(write32_relaxed, try_write32_relaxed, writel_relaxed <- u32);
     define_write!(
         #[cfg(CONFIG_64BIT)]
-        writeq_relaxed,
-        try_writeq_relaxed,
-        u64
+        write64_relaxed,
+        try_write64_relaxed,
+        writeq_relaxed <- u64
     );
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1fb6e44f33951c521c8b086a7a3a012af911cf26..ddc52db71a82a79657ec53025f9ef81d620516fc 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -43,17 +43,17 @@ struct SampleDriver {
 impl SampleDriver {
     fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
         // Select the test.
-        bar.writeb(index.0, Regs::TEST);
+        bar.write8(index.0, Regs::TEST);
 
-        let offset = u32::from_le(bar.readl(Regs::OFFSET)) as usize;
-        let data = bar.readb(Regs::DATA);
+        let offset = u32::from_le(bar.read32(Regs::OFFSET)) as usize;
+        let data = bar.read8(Regs::DATA);
 
         // Write `data` to `offset` to increase `count` by one.
         //
-        // Note that we need `try_writeb`, since `offset` can't be checked at compile-time.
-        bar.try_writeb(data, offset)?;
+        // Note that we need `try_write8`, since `offset` can't be checked at compile-time.
+        bar.try_write8(data, offset)?;
 
-        Ok(bar.readl(Regs::COUNT))
+        Ok(bar.read32(Regs::COUNT))
     }
 }
 

---
base-commit: 2408a807bfc3f738850ef5ad5e3fd59d66168996
change-id: 20250217-io-generic-rename-52d3af5b463f

Best regards,
-- 
Fiona Behrens <me@kloenk.dev>

Re: [PATCH] rust: io: rename `io::Io` accessors

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 09:58:14PM +0100, Fiona Behrens wrote:
> Rename the I/O accessors provided by `Io` to encode the type as
> number instead of letter. This is in preparation for Port I/O support
> to use a trait for generic accessors.
> 
> Add a `c_fn` argument to the accessor generation macro to translate
> between rust and C names.
> 
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/PIO.20support/near/499460541
> Signed-off-by: Fiona Behrens <me@kloenk.dev>

Acked-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH] rust: io: rename `io::Io` accessors

[Daniel Almeida]

Hi Fiona,

> On 17 Feb 2025, at 17:58, Fiona Behrens <me@kloenk.dev> wrote:
> 
> Rename the I/O accessors provided by `Io` to encode the type as
> number instead of letter. This is in preparation for Port I/O support
> to use a trait for generic accessors.
> 
> Add a `c_fn` argument to the accessor generation macro to translate
> between rust and C names.
> 
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/PIO.20support/near/499460541
> Signed-off-by: Fiona Behrens <me@kloenk.dev>

IMHO, the `writel` and `readl` nomenclature is extremely widespread, so it’s a bit confusing to see this renamed.

On the other hand, I do understand that such a change may be needed for ergonomic reasons when introducing
PIO, so I guess we will have to live with it :)

Acked-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH] rust: io: rename `io::Io` accessors

[Miguel Ojeda]

On Mon, Feb 17, 2025 at 10:37 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> IMHO, the `writel` and `readl` nomenclature is extremely widespread, so it’s a bit confusing to see this renamed.

We could generate `#[doc(alias = "...")]`s if that helps -- it would
appear if you type `writel` in the search, for instance.

(I guess the rename could be justified (just a bit) by the fact that
in Rust we are using fixed-width integers and so on more...)

Cheers,
Miguel

Re: [PATCH] rust: io: rename `io::Io` accessors

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 10:59:58PM +0100, Miguel Ojeda wrote:
> On Mon, Feb 17, 2025 at 10:37 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
> >
> > IMHO, the `writel` and `readl` nomenclature is extremely widespread, so it’s a bit confusing to see this renamed.
> 
> We could generate `#[doc(alias = "...")]`s if that helps -- it would
> appear if you type `writel` in the search, for instance.

This is a good idea, I think.

> 
> (I guess the rename could be justified (just a bit) by the fact that
> in Rust we are using fixed-width integers and so on more...)

This is a good reason too. My main reason for proposing this was that if we back
the trait with both MMIO and PIO `writel` and `readl` nomenclature might falsely
be read as if it is limited to MMIO.

Re: [PATCH] rust: io: rename `io::Io` accessors

[Greg Kroah-Hartman]

On Mon, Feb 17, 2025 at 09:58:14PM +0100, Fiona Behrens wrote:
> Rename the I/O accessors provided by `Io` to encode the type as
> number instead of letter. This is in preparation for Port I/O support
> to use a trait for generic accessors.
> 
> Add a `c_fn` argument to the accessor generation macro to translate
> between rust and C names.
> 
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/PIO.20support/near/499460541
> Signed-off-by: Fiona Behrens <me@kloenk.dev>
> ---
>  rust/kernel/io.rs               | 66 ++++++++++++++++++++---------------------
>  samples/rust/rust_driver_pci.rs | 12 ++++----
>  2 files changed, 39 insertions(+), 39 deletions(-)

This is good, thanks!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Want me to take this through my tree?

thanks,

greg k-h

Re: [PATCH] rust: io: rename `io::Io` accessors

[Miguel Ojeda]

On Tue, Feb 18, 2025 at 8:57 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> Want me to take this through my tree?

Sure, as you prefer! Happy either way. Thanks!

(The alias thing can be improved later)

Cheers,
Miguel

Re: [PATCH] rust: io: rename `io::Io` accessors

[Greg Kroah-Hartman]

On Sat, Feb 22, 2025 at 02:54:25PM +0100, Miguel Ojeda wrote:
> On Tue, Feb 18, 2025 at 8:57 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > Want me to take this through my tree?
> 
> Sure, as you prefer! Happy either way. Thanks!

Ok, I took it now, thanks!

greg k-h