[PATCH 1/4] rust: dma: implement DataDirection

Danilo Krummrich posted 4 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
Add the `DataDirection` struct, a newtype wrapper around the C
`enum dma_data_direction`.

This provides a type-safe Rust interface for specifying the direction of
DMA transfers.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/dma.rs              | 41 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 0e140e07758b..c2cc52ee9945 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -47,6 +47,7 @@
 #include <linux/cpumask.h>
 #include <linux/cred.h>
 #include <linux/device/faux.h>
+#include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 2bc8ab51ec28..b0950a2768a5 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -244,6 +244,47 @@ pub mod attrs {
     pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
 }
 
+/// DMA data direction.
+///
+/// Corresponds to the C [`enum dma_data_direction`].
+///
+/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub struct DataDirection(bindings::dma_data_direction);
+
+impl DataDirection {
+    /// The DMA mapping is for bidirectional data transfer.
+    ///
+    /// This is used when the buffer can be both read from and written to by the device.
+    /// The cache for the corresponding memory region is both flushed and invalidated.
+    pub const BIDIRECTIONAL: DataDirection =
+        DataDirection(bindings::dma_data_direction_DMA_BIDIRECTIONAL);
+
+    /// The DMA mapping is for data transfer from memory to the device (write).
+    ///
+    /// The CPU has prepared data in the buffer, and the device will read it.
+    /// The cache for the corresponding memory region is flushed.
+    pub const TO_DEVICE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_TO_DEVICE);
+
+    /// The DMA mapping is for data transfer from the device to memory (read).
+    ///
+    /// The device will write data into the buffer for the CPU to read.
+    /// The cache for the corresponding memory region is invalidated before CPU access.
+    pub const FROM_DEVICE: DataDirection =
+        DataDirection(bindings::dma_data_direction_DMA_FROM_DEVICE);
+
+    /// The DMA mapping is not for data transfer.
+    ///
+    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
+    /// will not perform any cache coherency operations.
+    pub const NONE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_NONE);
+
+    /// Returns the raw representation of [`enum dma_data_direction`].
+    pub fn as_raw(self) -> bindings::dma_data_direction {
+        self.0
+    }
+}
+
 /// An abstraction of the `dma_alloc_coherent` API.
 ///
 /// This is an abstraction around the `dma_alloc_coherent` API which is used to allocate and map
-- 
2.50.1
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Alice Ryhl 1 month, 2 weeks ago
On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
> Add the `DataDirection` struct, a newtype wrapper around the C
> `enum dma_data_direction`.
> 
> This provides a type-safe Rust interface for specifying the direction of
> DMA transfers.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

> +/// DMA data direction.
> +///
> +/// Corresponds to the C [`enum dma_data_direction`].
> +///
> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> +#[derive(Copy, Clone, PartialEq, Eq)]
> +pub struct DataDirection(bindings::dma_data_direction);

Perhaps this should be a real Rust enum so that you can do an exhaustive
match?

> +impl DataDirection {
> +    /// The DMA mapping is for bidirectional data transfer.
> +    ///
> +    /// This is used when the buffer can be both read from and written to by the device.
> +    /// The cache for the corresponding memory region is both flushed and invalidated.
> +    pub const BIDIRECTIONAL: DataDirection =
> +        DataDirection(bindings::dma_data_direction_DMA_BIDIRECTIONAL);
> +
> +    /// The DMA mapping is for data transfer from memory to the device (write).
> +    ///
> +    /// The CPU has prepared data in the buffer, and the device will read it.
> +    /// The cache for the corresponding memory region is flushed.
> +    pub const TO_DEVICE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_TO_DEVICE);
> +
> +    /// The DMA mapping is for data transfer from the device to memory (read).
> +    ///
> +    /// The device will write data into the buffer for the CPU to read.
> +    /// The cache for the corresponding memory region is invalidated before CPU access.
> +    pub const FROM_DEVICE: DataDirection =
> +        DataDirection(bindings::dma_data_direction_DMA_FROM_DEVICE);
> +
> +    /// The DMA mapping is not for data transfer.
> +    ///
> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> +    /// will not perform any cache coherency operations.
> +    pub const NONE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_NONE);
> +
> +    /// Returns the raw representation of [`enum dma_data_direction`].
> +    pub fn as_raw(self) -> bindings::dma_data_direction {
> +        self.0
> +    }
> +}
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
> On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
>> Add the `DataDirection` struct, a newtype wrapper around the C
>> `enum dma_data_direction`.
>> 
>> This provides a type-safe Rust interface for specifying the direction of
>> DMA transfers.
>> 
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
>> +/// DMA data direction.
>> +///
>> +/// Corresponds to the C [`enum dma_data_direction`].
>> +///
>> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> +#[derive(Copy, Clone, PartialEq, Eq)]
>> +pub struct DataDirection(bindings::dma_data_direction);
>
> Perhaps this should be a real Rust enum so that you can do an exhaustive
> match?

	/// DMA data direction.
	///
	/// Corresponds to the C [`enum dma_data_direction`].
	///
	/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
	#[repr(i32)]

Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
downside that we'd have to mess with the type either in the `repr` or by casting
the variants.

	pub enum DataDirection {
	    /// The DMA mapping is for bidirectional data transfer.
	    ///
	    /// This is used when the buffer can be both read from and written to by the device.
	    /// The cache for the corresponding memory region is both flushed and invalidated.
	    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
	
	    /// The DMA mapping is for data transfer from memory to the device (write).
	    ///
	    /// The CPU has prepared data in the buffer, and the device will read it.
	    /// The cache for the corresponding memory region is flushed.
	    ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
	
	    /// The DMA mapping is for data transfer from the device to memory (read).
	    ///
	    /// The device will write data into the buffer for the CPU to read.
	    /// The cache for the corresponding memory region is invalidated before CPU access.
	    FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
	
	    /// The DMA mapping is not for data transfer.
	    ///
	    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
	    /// will not perform any cache coherency operations.
	    None = bindings::dma_data_direction_DMA_NONE,
	}
	
	impl From<DataDirection> for bindings::dma_data_direction {
	    /// Returns the raw representation of [`enum dma_data_direction`].
	    fn from(direction: DataDirection) -> Self {
	        direction as Self
	    }
	}
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Alice Ryhl 1 month, 2 weeks ago
On Mon, Aug 18, 2025 at 01:27:44PM +0200, Danilo Krummrich wrote:
> On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
> > On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
> >> Add the `DataDirection` struct, a newtype wrapper around the C
> >> `enum dma_data_direction`.
> >> 
> >> This provides a type-safe Rust interface for specifying the direction of
> >> DMA transfers.
> >> 
> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >
> >> +/// DMA data direction.
> >> +///
> >> +/// Corresponds to the C [`enum dma_data_direction`].
> >> +///
> >> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> >> +#[derive(Copy, Clone, PartialEq, Eq)]
> >> +pub struct DataDirection(bindings::dma_data_direction);
> >
> > Perhaps this should be a real Rust enum so that you can do an exhaustive
> > match?
> 
> 	/// DMA data direction.
> 	///
> 	/// Corresponds to the C [`enum dma_data_direction`].
> 	///
> 	/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> 	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> 	#[repr(i32)]
> 
> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
> downside that we'd have to mess with the type either in the `repr` or by casting
> the variants.
> 
> 	pub enum DataDirection {
> 	    /// The DMA mapping is for bidirectional data transfer.
> 	    ///
> 	    /// This is used when the buffer can be both read from and written to by the device.
> 	    /// The cache for the corresponding memory region is both flushed and invalidated.
> 	    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
> 	
> 	    /// The DMA mapping is for data transfer from memory to the device (write).
> 	    ///
> 	    /// The CPU has prepared data in the buffer, and the device will read it.
> 	    /// The cache for the corresponding memory region is flushed.
> 	    ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
> 	
> 	    /// The DMA mapping is for data transfer from the device to memory (read).
> 	    ///
> 	    /// The device will write data into the buffer for the CPU to read.
> 	    /// The cache for the corresponding memory region is invalidated before CPU access.
> 	    FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
> 	
> 	    /// The DMA mapping is not for data transfer.
> 	    ///
> 	    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> 	    /// will not perform any cache coherency operations.
> 	    None = bindings::dma_data_direction_DMA_NONE,
> 	}
> 	
> 	impl From<DataDirection> for bindings::dma_data_direction {
> 	    /// Returns the raw representation of [`enum dma_data_direction`].
> 	    fn from(direction: DataDirection) -> Self {
> 	        direction as Self
> 	    }
> 	}

My suggestion is to cast on the Rust-side.

#[repr(whateveryouwant)]
enum DataDirection {
    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL as _,
}
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
On Mon Aug 18, 2025 at 2:22 PM CEST, Alice Ryhl wrote:
> On Mon, Aug 18, 2025 at 01:27:44PM +0200, Danilo Krummrich wrote:
>> On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
>> > On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
>> >> Add the `DataDirection` struct, a newtype wrapper around the C
>> >> `enum dma_data_direction`.
>> >> 
>> >> This provides a type-safe Rust interface for specifying the direction of
>> >> DMA transfers.
>> >> 
>> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >
>> >> +/// DMA data direction.
>> >> +///
>> >> +/// Corresponds to the C [`enum dma_data_direction`].
>> >> +///
>> >> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> >> +#[derive(Copy, Clone, PartialEq, Eq)]
>> >> +pub struct DataDirection(bindings::dma_data_direction);
>> >
>> > Perhaps this should be a real Rust enum so that you can do an exhaustive
>> > match?
>> 
>> 	/// DMA data direction.
>> 	///
>> 	/// Corresponds to the C [`enum dma_data_direction`].
>> 	///
>> 	/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> 	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
>> 	#[repr(i32)]
>> 
>> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
>> downside that we'd have to mess with the type either in the `repr` or by casting
>> the variants.
>> 
>> 	pub enum DataDirection {
>> 	    /// The DMA mapping is for bidirectional data transfer.
>> 	    ///
>> 	    /// This is used when the buffer can be both read from and written to by the device.
>> 	    /// The cache for the corresponding memory region is both flushed and invalidated.
>> 	    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
>> 	
>> 	    /// The DMA mapping is for data transfer from memory to the device (write).
>> 	    ///
>> 	    /// The CPU has prepared data in the buffer, and the device will read it.
>> 	    /// The cache for the corresponding memory region is flushed.
>> 	    ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
>> 	
>> 	    /// The DMA mapping is for data transfer from the device to memory (read).
>> 	    ///
>> 	    /// The device will write data into the buffer for the CPU to read.
>> 	    /// The cache for the corresponding memory region is invalidated before CPU access.
>> 	    FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
>> 	
>> 	    /// The DMA mapping is not for data transfer.
>> 	    ///
>> 	    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
>> 	    /// will not perform any cache coherency operations.
>> 	    None = bindings::dma_data_direction_DMA_NONE,
>> 	}
>> 	
>> 	impl From<DataDirection> for bindings::dma_data_direction {
>> 	    /// Returns the raw representation of [`enum dma_data_direction`].
>> 	    fn from(direction: DataDirection) -> Self {
>> 	        direction as Self
>> 	    }
>> 	}
>
> My suggestion is to cast on the Rust-side.
>
> #[repr(whateveryouwant)]

What's your suggestion for whateveryouwant?

And I mean this in general, not only for this case of dma::DataDirection. If we
pick u32 than things break if a negative enum variant is added on the C side. If
we pick i32, it can happen that we overflow.

> enum DataDirection {
>     Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL as _,

We have a clippy lint that warns about `as _` casts, but I guess you meant
`as whateveryouwant`. However, if bindgen picks whateveryouwant too, we also get
a warning.

Ultimately, we'd need to suppress a warning.

> }
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Alice Ryhl 1 month, 2 weeks ago
On Mon, Aug 18, 2025 at 2:57 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Aug 18, 2025 at 2:22 PM CEST, Alice Ryhl wrote:
> > On Mon, Aug 18, 2025 at 01:27:44PM +0200, Danilo Krummrich wrote:
> >> On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
> >> > On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
> >> >> Add the `DataDirection` struct, a newtype wrapper around the C
> >> >> `enum dma_data_direction`.
> >> >>
> >> >> This provides a type-safe Rust interface for specifying the direction of
> >> >> DMA transfers.
> >> >>
> >> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> >
> >> >> +/// DMA data direction.
> >> >> +///
> >> >> +/// Corresponds to the C [`enum dma_data_direction`].
> >> >> +///
> >> >> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> >> >> +#[derive(Copy, Clone, PartialEq, Eq)]
> >> >> +pub struct DataDirection(bindings::dma_data_direction);
> >> >
> >> > Perhaps this should be a real Rust enum so that you can do an exhaustive
> >> > match?
> >>
> >>      /// DMA data direction.
> >>      ///
> >>      /// Corresponds to the C [`enum dma_data_direction`].
> >>      ///
> >>      /// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> >>      #[derive(Copy, Clone, PartialEq, Eq, Debug)]
> >>      #[repr(i32)]
> >>
> >> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
> >> downside that we'd have to mess with the type either in the `repr` or by casting
> >> the variants.
> >>
> >>      pub enum DataDirection {
> >>          /// The DMA mapping is for bidirectional data transfer.
> >>          ///
> >>          /// This is used when the buffer can be both read from and written to by the device.
> >>          /// The cache for the corresponding memory region is both flushed and invalidated.
> >>          Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
> >>
> >>          /// The DMA mapping is for data transfer from memory to the device (write).
> >>          ///
> >>          /// The CPU has prepared data in the buffer, and the device will read it.
> >>          /// The cache for the corresponding memory region is flushed.
> >>          ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
> >>
> >>          /// The DMA mapping is for data transfer from the device to memory (read).
> >>          ///
> >>          /// The device will write data into the buffer for the CPU to read.
> >>          /// The cache for the corresponding memory region is invalidated before CPU access.
> >>          FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
> >>
> >>          /// The DMA mapping is not for data transfer.
> >>          ///
> >>          /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> >>          /// will not perform any cache coherency operations.
> >>          None = bindings::dma_data_direction_DMA_NONE,
> >>      }
> >>
> >>      impl From<DataDirection> for bindings::dma_data_direction {
> >>          /// Returns the raw representation of [`enum dma_data_direction`].
> >>          fn from(direction: DataDirection) -> Self {
> >>              direction as Self
> >>          }
> >>      }
> >
> > My suggestion is to cast on the Rust-side.
> >
> > #[repr(whateveryouwant)]
>
> What's your suggestion for whateveryouwant?
>
> And I mean this in general, not only for this case of dma::DataDirection. If we
> pick u32 than things break if a negative enum variant is added on the C side. If
> we pick i32, it can happen that we overflow.
>
> > enum DataDirection {
> >     Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL as _,
>
> We have a clippy lint that warns about `as _` casts, but I guess you meant
> `as whateveryouwant`. However, if bindgen picks whateveryouwant too, we also get
> a warning.
>
> Ultimately, we'd need to suppress a warning.

In general, I think we want some sort of helper function to cast
between arbitrary integer types in const-evaluation that panics if the
cast is out of bounds. Both here and many other places.

Alice
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
On Mon Aug 18, 2025 at 4:00 PM CEST, Alice Ryhl wrote:
> In general, I think we want some sort of helper function to cast
> between arbitrary integer types in const-evaluation that panics if the
> cast is out of bounds. Both here and many other places.

What exactly do you have in mind?
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Alice Ryhl 1 month, 2 weeks ago
On Mon, Aug 18, 2025 at 7:23 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Aug 18, 2025 at 4:00 PM CEST, Alice Ryhl wrote:
> > In general, I think we want some sort of helper function to cast
> > between arbitrary integer types in const-evaluation that panics if the
> > cast is out of bounds. Both here and many other places.
>
> What exactly do you have in mind?

        pub enum DataDirection {
            /// The DMA mapping is for bidirectional data transfer.
            ///
            /// This is used when the buffer can be both read from and
written to by the device.
            /// The cache for the corresponding memory region is both
flushed and invalidated.
            Bidirectional =
const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),

with no warnings and build-failure if out-of-bounds.

Alice
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
> with no warnings and build-failure if out-of-bounds.

+/// DMA data direction.
+///
+/// Corresponds to the C [`enum dma_data_direction`].
+///
+/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[repr(u32)]
+pub enum DataDirection {
+    /// The DMA mapping is for bidirectional data transfer.
+    ///
+    /// This is used when the buffer can be both read from and written to by the device.
+    /// The cache for the corresponding memory region is both flushed and invalidated.
+    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
+
+    /// The DMA mapping is for data transfer from memory to the device (write).
+    ///
+    /// The CPU has prepared data in the buffer, and the device will read it.
+    /// The cache for the corresponding memory region is flushed.
+    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
+
+    /// The DMA mapping is for data transfer from the device to memory (read).
+    ///
+    /// The device will write data into the buffer for the CPU to read.
+    /// The cache for the corresponding memory region is invalidated before CPU access.
+    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
+
+    /// The DMA mapping is not for data transfer.
+    ///
+    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
+    /// will not perform any cache coherency operations.
+    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
+}
+
+impl DataDirection {
+    /// Casts the bindgen-generated enum type to a `u32` at compile time.
+    ///
+    /// This function will cause a compile-time error if the underlying value of the
+    /// C enum is out of bounds for `u32`.
+    const fn const_cast(val: bindings::dma_data_direction) -> u32 {
+        // CAST: The C standard allows compilers to choose different integer types for enums.
+        // To safely check the value, we cast it to a wide signed integer type (`i128`)
+        // which can hold any standard C integer enum type without truncation.
+        let wide_val = val as i128;
+
+        // Check if the value is outside the valid range for the target type `u32`.
+        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
+        if wide_val < 0 || wide_val > u32::MAX as i128 {
+            // Trigger a compile-time error in a const context.
+            panic!("C enum value is out of bounds for the target type `u32`.");
+        }
+
+        // CAST: This cast is valid because the check above guarantees that `wide_val`
+        // is within the representable range of `u32`.
+        wide_val as u32
+    }
+}
+
+impl From<DataDirection> for bindings::dma_data_direction {
+    /// Returns the raw representation of [`enum dma_data_direction`].
+    fn from(direction: DataDirection) -> Self {
+        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
+        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
+        // with the enum variants of `DataDirection`, which is a valid assumption given our
+        // compile-time checks.
+        direction as u32 as Self
+    }
+}
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Daniel Almeida 1 month, 2 weeks ago

> On 18 Aug 2025, at 18:03, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
>> with no warnings and build-failure if out-of-bounds.
> 
> +/// DMA data direction.
> +///
> +/// Corresponds to the C [`enum dma_data_direction`].
> +///
> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +#[repr(u32)]
> +pub enum DataDirection {
> +    /// The DMA mapping is for bidirectional data transfer.
> +    ///
> +    /// This is used when the buffer can be both read from and written to by the device.
> +    /// The cache for the corresponding memory region is both flushed and invalidated.
> +    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
> +
> +    /// The DMA mapping is for data transfer from memory to the device (write).
> +    ///
> +    /// The CPU has prepared data in the buffer, and the device will read it.
> +    /// The cache for the corresponding memory region is flushed.
> +    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
> +
> +    /// The DMA mapping is for data transfer from the device to memory (read).
> +    ///
> +    /// The device will write data into the buffer for the CPU to read.
> +    /// The cache for the corresponding memory region is invalidated before CPU access.
> +    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
> +
> +    /// The DMA mapping is not for data transfer.
> +    ///
> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> +    /// will not perform any cache coherency operations.
> +    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
> +}
> +
> +impl DataDirection {
> +    /// Casts the bindgen-generated enum type to a `u32` at compile time.
> +    ///
> +    /// This function will cause a compile-time error if the underlying value of the
> +    /// C enum is out of bounds for `u32`.
> +    const fn const_cast(val: bindings::dma_data_direction) -> u32 {

This should be its own generic helper for similar enums, IMHO

> +        // CAST: The C standard allows compilers to choose different integer types for enums.
> +        // To safely check the value, we cast it to a wide signed integer type (`i128`)
> +        // which can hold any standard C integer enum type without truncation.
> +        let wide_val = val as i128;
> +
> +        // Check if the value is outside the valid range for the target type `u32`.
> +        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
> +        if wide_val < 0 || wide_val > u32::MAX as i128 {
> +            // Trigger a compile-time error in a const context.
> +            panic!("C enum value is out of bounds for the target type `u32`.");
> +        }
> +
> +        // CAST: This cast is valid because the check above guarantees that `wide_val`
> +        // is within the representable range of `u32`.
> +        wide_val as u32
> +    }
> +}
> +
> +impl From<DataDirection> for bindings::dma_data_direction {
> +    /// Returns the raw representation of [`enum dma_data_direction`].
> +    fn from(direction: DataDirection) -> Self {
> +        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
> +        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
> +        // with the enum variants of `DataDirection`, which is a valid assumption given our
> +        // compile-time checks.
> +        direction as u32 as Self
> +    }
> +}
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
On Wed Aug 20, 2025 at 3:17 PM CEST, Daniel Almeida wrote:
>
>
>> On 18 Aug 2025, at 18:03, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
>>> with no warnings and build-failure if out-of-bounds.
>> 
>> +/// DMA data direction.
>> +///
>> +/// Corresponds to the C [`enum dma_data_direction`].
>> +///
>> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
>> +#[repr(u32)]
>> +pub enum DataDirection {
>> +    /// The DMA mapping is for bidirectional data transfer.
>> +    ///
>> +    /// This is used when the buffer can be both read from and written to by the device.
>> +    /// The cache for the corresponding memory region is both flushed and invalidated.
>> +    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
>> +
>> +    /// The DMA mapping is for data transfer from memory to the device (write).
>> +    ///
>> +    /// The CPU has prepared data in the buffer, and the device will read it.
>> +    /// The cache for the corresponding memory region is flushed.
>> +    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
>> +
>> +    /// The DMA mapping is for data transfer from the device to memory (read).
>> +    ///
>> +    /// The device will write data into the buffer for the CPU to read.
>> +    /// The cache for the corresponding memory region is invalidated before CPU access.
>> +    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
>> +
>> +    /// The DMA mapping is not for data transfer.
>> +    ///
>> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
>> +    /// will not perform any cache coherency operations.
>> +    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
>> +}
>> +
>> +impl DataDirection {
>> +    /// Casts the bindgen-generated enum type to a `u32` at compile time.
>> +    ///
>> +    /// This function will cause a compile-time error if the underlying value of the
>> +    /// C enum is out of bounds for `u32`.
>> +    const fn const_cast(val: bindings::dma_data_direction) -> u32 {
>
> This should be its own generic helper for similar enums, IMHO

In general, I agree, but it may not be exactly straight forward (considering
that it still needs to work from const context).

The function relies on the fact that both the argument and return type are
numeric primitives, which need to fit into an i128. We could probably define a
marker trait for such types, etc.

Yet, I don't know how we could do the casts from one generic type I to another
generic type O. I think we'd need to implement traits for that as well. However,
this would require the unstable const_trait_impl feature.

Hence, working this out (if even possible currently) and improving existing enum
abstractions should be done independent from this series.

>> +        // CAST: The C standard allows compilers to choose different integer types for enums.
>> +        // To safely check the value, we cast it to a wide signed integer type (`i128`)
>> +        // which can hold any standard C integer enum type without truncation.
>> +        let wide_val = val as i128;
>> +
>> +        // Check if the value is outside the valid range for the target type `u32`.
>> +        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
>> +        if wide_val < 0 || wide_val > u32::MAX as i128 {
>> +            // Trigger a compile-time error in a const context.
>> +            panic!("C enum value is out of bounds for the target type `u32`.");
>> +        }
>> +
>> +        // CAST: This cast is valid because the check above guarantees that `wide_val`
>> +        // is within the representable range of `u32`.
>> +        wide_val as u32
>> +    }
>> +}
>> +
>> +impl From<DataDirection> for bindings::dma_data_direction {
>> +    /// Returns the raw representation of [`enum dma_data_direction`].
>> +    fn from(direction: DataDirection) -> Self {
>> +        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
>> +        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
>> +        // with the enum variants of `DataDirection`, which is a valid assumption given our
>> +        // compile-time checks.
>> +        direction as u32 as Self
>> +    }
>> +}
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Miguel Ojeda 1 month, 2 weeks ago
On Mon, Aug 18, 2025 at 1:27 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
> downside that we'd have to mess with the type either in the `repr` or by casting
> the variants.

Yes, it can easily pick `u32` -- the variants are always `int` in C,
but the actual enum can be a type compatible with int, unsigned or
char; and `bindgen` generates each variant with a type alias to the
underlying type of the `enum`, not `int`.

So e.g. for

    enum uint_enum { uint_enum_a, uint_enum_b };
    enum int_enum { int_enum_a = -1, int_enum_b };

    _Static_assert(_Generic(uint_enum_a, int: 1, default: 0), "");
    _Static_assert(_Generic(uint_enum_b, int: 1, default: 0), "");
    _Static_assert(_Generic(int_enum_a, int: 1, default: 0), "");
    _Static_assert(_Generic(int_enum_b, int: 1, default: 0), "");

    _Static_assert(_Generic((enum uint_enum)0, unsigned: 1, default: 0), "");
    _Static_assert(_Generic((enum int_enum)0, int: 1, default: 0), "");

you get:

    pub const uint_enum_uint_enum_a: uint_enum = 0;
    pub const uint_enum_uint_enum_b: uint_enum = 1;
    pub type uint_enum = ffi::c_uint;

    pub const int_enum_int_enum_a: int_enum = -1;
    pub const int_enum_int_enum_b: int_enum = 0;
    pub type int_enum = ffi::c_int;

Then there is this issue I reported to be careful about, which can
change it back to `i32` if there is a forward reference:

    https://github.com/rust-lang/rust-bindgen/issues/3179

I hope that helps.

Cheers,
Miguel
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Danilo Krummrich 1 month, 2 weeks ago
On 8/18/25 1:56 PM, Miguel Ojeda wrote:
> I hope that helps.

It does -- thanks for writing this up!
Re: [PATCH 1/4] rust: dma: implement DataDirection
Posted by Miguel Ojeda 1 month, 2 weeks ago
On Mon, Aug 18, 2025 at 1:56 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Yes, it can easily pick `u32` -- the variants are always `int` in C,

And, for completeness, if the kernel eventually uses C23 enums (I
don't see any, from a quick grep), i.e. with an underlying type, then
in C the variants will actually have that type, not `int`:

    enum c23_enum : short { c23_enum_a, c23_enum_b };

    _Static_assert(_Generic(c23_enum_a, short: 1, default: 0), "");
    _Static_assert(_Generic(c23_enum_b, short: 1, default: 0), "");

    _Static_assert(_Generic((enum c23_enum)0, short: 1, default: 0), "");

And `bindgen` will have the underlying type too, so everything matches
in that case:

    pub const c23_enum_c23_enum_a: c23_enum = 0;
    pub const c23_enum_c23_enum_b: c23_enum = 1;
    pub type c23_enum = ffi::c_short;

So, in a way, what `bindgen` tries to do (for the normal ones) is
behave a bit like these new ones.

But I always wondered if it is good or not differing there.

Cheers,
Miguel